RFR: 8251464: make Node::dump(int depth) support indent

[Tobias Hartmann]

Hi Xin,

thanks for making these changes. That looks good to me.
But lets wait for some more opinions from other reviewers.

Best regards,
Tobias

On 04.09.20 06:33, Liu, Xin wrote:
> Hi, Tobias, 
> 
> Thank you to review the patch.
> 
> Yes, My change does affect PrintIdeal. Indeed, if line-wrapping happens, the readability will drop a lot. 
> Taking your advice, I introduce a new c2 option PrintIdealIndentThreshold=5. if users attempt to dump an ideal graph 
> deeper than that level, the indention function will be automatically disable. 
> 
> -XX:+PrintIdeal is root()->dump(9999) under the hook. Of course it doesn't indent. Indention still happens if users call "node->dump(-3)"  in a debugger. 
> 
> Making a beautiful formation is a surprisingly hard task. Previously, I would like to treat Node::_idx as a line number in vim.  That's why I make them align on left side. 
> In this new revision, I indent a few whitespaces in first place. The result looks like this.  What do you think? 
> https://bugs.openjdk.java.net/secure/attachment/90041/indent_with_idx.log
> 
> here is the new revison: 
> http://cr.openjdk.java.net/~xliu/8251464/01/webrev/
> 
> thanks, 
> --lx
> 
>  
> ________________________________________
> From: Tobias Hartmann <tobias.hartmann at oracle.com>
> Sent: Thursday, September 3, 2020 5:30 AM
> To: Liu, Xin; 'hotspot-compiler-dev at openjdk.java.net'
> Subject: RE: [EXTERNAL] RFR: 8251464: make Node::dump(int depth) support indent
> 
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> Hi Xin,
> 
> I'm concerned that the output quickly becomes unreadable when dumping a large graph.
> For example, isn't -XX:+PrintIdeal affected by this as well?
> 
> Also, looking at the example output you've posted [1], shouldn't the node id be indented as well?
> 
> This might be helpful when dumping small parts of the graph but then it should be optional (i.e. can
> be turned on via a flag/argument if needed).
> 
> Best regards,
> Tobias
> 
> [1] https://bugs.openjdk.java.net/secure/attachment/89800/dump2.txt
> 
> On 29.08.20 22:08, Liu, Xin wrote:
>> hi, Reviewers,
>>
>>
>> Could you review this patch?
>>
>> JBS:https://bugs.openjdk.java.net/browse/JDK-8251464
>>
>> Webrev:
>>
>> http://cr.openjdk.java.net/~xliu/8251464/00/webrev/
>>
>>
>> This patch attempts to improve the formation of nodes when developers try to dump an ideal graph or snippet of a graph.  In practice, I found it's pretty handy if Node::dump(int d) can support indent.
>>
>> The basic idea is to support indention for the utility function:
>>
>> collect_nodes_i(GrowableArray<Node*>* queue, const Node* start, int direction, uint depth, bool include_start, bool only_ctrl, bool only_data)
>>
>> It only affects Node::dump family and -XX::PrintIdeal.  It won't impact the output for igv.
>> This can help developers who try to inspect a cluster of nodes in gdb.
>>
>> Another change is naming. collect_nodes_i uses breadth-first search. the container is used in fifo way instead of filo.
>> I think the name "queue" serve better.
>>
>> TEST:
>> hotspot:tier1  and gtest.
>> mach-5
>>
>> thanks,
>> --lx
>>
>>