RFR: 8302738: IGV: refine 'Simplify graph' filter [v2]

[Roberto Castañeda Lozano]

On Tue, 28 Mar 2023 08:42:53 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> Roberto Castañeda Lozano has updated the pull request incrementally with five additional commits since the last revision:
>> 
>>  - Increase the bold text line factor slightly
>>  - Add extra horizontal margin for long labels and let them overflow within the node
>>  - Select slots as well
>>  - Remove code that is commented out
>>  - Assert inputLabel is non-null
>
> Thanks Roberto for your detailed answers!
> 
>> > When selecting a CallStaticJava node, the custom node info is sometimes cut depending on the zoom level (sometimes more, sometimes less)
>> 
>> Good catch, @chhagedorn! This is an existing issue in mainline IGV, you can reproduce it e.g. by showing a long property such as `dump_spec` in the node text. The issue just becomes more visible with the addition of custom node info in this changeset. As far as I understand, the node width is computed assuming it is selected (i.e. bold text) at 100% zoom level, and scaled proportionally to the selected zoom level. This assumes label fonts scale perfectly with the zoom level, which is not the case. As a result, very long node labels can overflow at different zoom levels than 100%. I don't see a better solution than multiplying the computed node width with a factor (`Figure::BOLD_LINE_FACTOR`) to account for the worst-case text overflow at any zoom level. This will not change the width of most nodes since this tends to be dominated by the input slots anyway, only for those nodes with long labels. I selected this factor experimentally to be of 6% of the total width. Hope this new ver
 sion fixes the issue you observed. If not, please try out and suggest a more appropriate factor.
> 
> That looks much better now! 6% seems to be a good value to go with. Thanks for fixing this general issue. 
> 
>> > Selecting an inlined node with the condensed graph filter does not work when searching for it. For example, I can search for 165 Bool node in the search field. It finds it but when clicking on it, it shows me an empty graph. I would have expected to see the following graph with the "outer" node being selected which includes 165 Bool.
>> 
>> Selecting, highlighting, centering, synchronizing etc. inlined and combined nodes ("slots" in IGV speak) has not been possible at all before this changeset. You can reproduce similar issues when using the "Simplify graph" filter in mainline IGV.
> 
> I see, I've never used the "Simplify graph" filter before. That's why I've only noticed this now.
> 
>> I included some basic (admittedly half-baked) support for this in this changeset (enhanced searching and parts of selecting, but not highlighting, centering, or synchronizing among tabs), but implementing full support would require a rather deep refactoring of IGV. I will not have time to work on such a refactoring in the coming weeks, so I propose to simply remove the partial support for slot interaction implemented provided by this changeset, so that we leave IGV in the same consistent state as before, and create a RFE for adding proper support in the future. @chhagedorn, @tobiasholenstein what do you think?
> 
> I agree with your suggestion to remove the partial implementation and try to fully support it later in a separate RFE. That might be the cleanest solution for now. And we could still take your current code as a starting point for that RFE .
> 
>> > Maybe the node info can be improved further in a future RFE, for example for CountedLoop nodes to also show if it is a pre/main/post loop or to add the stride.
>> 
>> Good suggestion! I agree that there is room for further exploiting custom node info in the future, loop nodes are excellent candidates :)
> 
> Great! :-) Can you file an RFE for that?
> 
> Thanks,
> Christian

I just reverted the filter ordering changes from this changeset and merged from master, incorporating the changes from JDK-8302644 instead ("IGV: Apply filters per graph tab and not globally"). Testing passes as before. @chhagedorn, @tobiasholenstein would you like to have another look or am I free to integrate?

-------------

PR Comment: https://git.openjdk.org/jdk/pull/12955#issuecomment-1490597196