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

[Liu, Xin]

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
>
>