Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8260581: IGV: enhance node search #2285

Closed
wants to merge 11 commits into from
Closed

Conversation

@robcasloz
Copy link
Contributor

@robcasloz robcasloz commented Jan 28, 2021

Apply several enhancements to the quick node search functionality:

  • Allow users to search by node id or name by default (i.e. when no property is specified) instead of name only.
  • Show partial matches when searching for a specific property (e.g. so that searching "type=con" lists all "control"-type nodes).
  • Avoid showing the "All N matching nodes" entry if there is a single match, or the user is searching a numeric value.
  • Rank matches so that full matches are listed first, followed by matches at the beginning of the partially matched value, followed by the rest of matches in increasing size of the partially matched value. Numeric matches with the same rank are sorted increasingly. For example, searching "5" on a set of nodes with labels {"5 AddI", "25 AddL", "253 AddL", "554 MulI"} should list the matches as follows:
    1. 5 AddI
    2. 554 MulI
    3. 25 AddL
    4. 253 AddL

As an illustration of some of these enhancements, this screenshot shows the behavior of the quick node search functionality before the changes:

search-before

and after:

search-after

Tested manually on small and large (~10000 nodes) graphs. Thanks to Christian Hagedorn for feedback on several iterations of the enhancements.

As part of the review, please evaluate not just the code changes but also the usability.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

Reviewers

Contributors

  • Christian Hagedorn <chagedorn@openjdk.org>

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2285/head:pull/2285
$ git checkout pull/2285

Change the default quick search property to "label", to allow users to search by
node id or name by default, instead of name only. Re-compute the "label"
property every time node labels are updated.
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 28, 2021

👋 Welcome back rcastanedalo! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

@openjdk openjdk bot commented Jan 28, 2021

@robcasloz The following label will be automatically applied to this pull request:

  • hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@robcasloz
Copy link
Contributor Author

@robcasloz robcasloz commented Jan 28, 2021

/summary
Change the default quick search property to "label", to allow users to search by
node id or name by default, instead of name only. Re-compute the "label"
property every time node labels are updated.

@openjdk
Copy link

@openjdk openjdk bot commented Jan 28, 2021

@robcasloz Setting summary to:

Change the default quick search property to "label", to allow users to search by
node id or name by default, instead of name only. Re-compute the "label"
property every time node labels are updated.

@robcasloz robcasloz changed the title 8260581: IGV: search on node labels by default instead of name only 8260581: IGV: enhance node search Jan 29, 2021
robcasloz added 3 commits Jan 29, 2021
Adding an explicit "Recent Searches" category rather than the default, implicit
one triggers two issues:

- IGV does not show the recent searches when clicking into the search bar.

- IGV selects random nodes in the list of matches instead of the top one.
@robcasloz
Copy link
Contributor Author

@robcasloz robcasloz commented Jan 29, 2021

/summary
Allow users to search by node id or name by default, show partial matches when
searching for a specific property, show 'All N matching nodes' entry only if
relevant, and rank results by level of matching.

@openjdk
Copy link

@openjdk openjdk bot commented Jan 29, 2021

@robcasloz Updating existing summary to:

Allow users to search by node id or name by default, show partial matches when
searching for a specific property, show 'All N matching nodes' entry only if
relevant, and rank results by level of matching.

@robcasloz robcasloz marked this pull request as ready for review Jan 29, 2021
@openjdk openjdk bot added the rfr label Jan 29, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 29, 2021

Copy link
Member

@chhagedorn chhagedorn left a comment

Otherwise, it looks good to me.

I applied your patch and tested the new features in the IGV. They work as expected. Good to see the search being improved!

@openjdk
Copy link

@openjdk openjdk bot commented Jan 29, 2021

@robcasloz This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8260581: IGV: enhance node search

Allow users to search by node id or name by default, show partial matches when
searching for a specific property, show 'All N matching nodes' entry only if
relevant, and rank results by level of matching.

Co-authored-by: Christian Hagedorn <chagedorn@openjdk.org>
Reviewed-by: chagedorn, vlivanov, xliu

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 80 new commits pushed to the master branch:

  • 9037615: 8222850: jshell tool: Misleading cascade compiler error in switch expression with undefined vars
  • 91e6c75: 8260928: InitArrayShortSize constraint func should print a helpful error message
  • cb127a4: 8198343: Test java/awt/print/PrinterJob/TestPgfmtSetMPA.java may fail w/o printer
  • c008410: 8197825: [Test] Intermittent timeout with javax/swing JColorChooser Test
  • b9d4211: 8260193: Remove JVM_GetInterfaceVersion() and JVM_DTraceXXX
  • 98a7692: 8076313: GraphicsEnvironment does not detect changes in count of monitors on Linux OS
  • a47befc: 8260878: com/sun/jdi/JdbOptions.java fails without jfr
  • d423d36: 8258508: Merge G1RedirtyCardsQueue into qset
  • bec6043: 8259570: (macos) tools/jpackage tests fails with 'hdiutil: couldn't eject "disk2" - Resource busy'
  • ffbcf1b: 8260471: Change SystemDictionary::X_klass calls to vmClasses::X_klass
  • ... and 70 more: https://git.openjdk.java.net/jdk/compare/a97aedff9f622ba6816b7550588f1d429bff2483...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@chhagedorn, @iwanowww) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Jan 29, 2021
Copy link

@iwanowww iwanowww left a comment

Awesome! Finally!

@robcasloz
Copy link
Contributor Author

@robcasloz robcasloz commented Feb 1, 2021

Thanks Christian and Vladimir for reviewing! I will wait a couple more days in case anyone else has comments.

Sort otherwise equally relevant matches by node id, which is by default the
first word in node labels. Thanks to Christian Hagedorn for the suggestion and
(slightly adapted) patch.
@robcasloz
Copy link
Contributor Author

@robcasloz robcasloz commented Feb 1, 2021

/contributor add @chhagedorn

@openjdk
Copy link

@openjdk openjdk bot commented Feb 1, 2021

@robcasloz
Contributor Christian Hagedorn <chagedorn@openjdk.org> successfully added.

@robcasloz
Copy link
Contributor Author

@robcasloz robcasloz commented Feb 1, 2021

Commit 981e903 adds a match sorting refinement contributed by @chhagedorn (numeric sorting of same-rank matches), please re-review.

Copy link
Member

@chhagedorn chhagedorn left a comment

Thanks for adding the additional sorting suggestion! Looks good to me.

I applied the patch again and everything works as expected in the IGV.

Copy link
Member

@navyxliu navyxliu left a comment

LGTM. I ran it with your patch. it works as expected.

@robcasloz
Copy link
Contributor Author

@robcasloz robcasloz commented Feb 2, 2021

Thanks for adding the additional sorting suggestion! Looks good to me.

I applied the patch again and everything works as expected in the IGV.

Thanks again, Christian!

@robcasloz
Copy link
Contributor Author

@robcasloz robcasloz commented Feb 2, 2021

LGTM. I ran it with your patch. it works as expected.

Thanks for reviewing, Xin!

@robcasloz
Copy link
Contributor Author

@robcasloz robcasloz commented Feb 3, 2021

/integrate

@openjdk openjdk bot added the sponsor label Feb 3, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 3, 2021

@robcasloz
Your change (at version 981e903) is now ready to be sponsored by a Committer.

@robcasloz
Copy link
Contributor Author

@robcasloz robcasloz commented Feb 3, 2021

Please sponsor.

@iwanowww
Copy link

@iwanowww iwanowww commented Feb 3, 2021

/sponsor

@openjdk openjdk bot closed this Feb 3, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 3, 2021

@iwanowww @robcasloz Since your change was applied there have been 80 commits pushed to the master branch:

  • 9037615: 8222850: jshell tool: Misleading cascade compiler error in switch expression with undefined vars
  • 91e6c75: 8260928: InitArrayShortSize constraint func should print a helpful error message
  • cb127a4: 8198343: Test java/awt/print/PrinterJob/TestPgfmtSetMPA.java may fail w/o printer
  • c008410: 8197825: [Test] Intermittent timeout with javax/swing JColorChooser Test
  • b9d4211: 8260193: Remove JVM_GetInterfaceVersion() and JVM_DTraceXXX
  • 98a7692: 8076313: GraphicsEnvironment does not detect changes in count of monitors on Linux OS
  • a47befc: 8260878: com/sun/jdi/JdbOptions.java fails without jfr
  • d423d36: 8258508: Merge G1RedirtyCardsQueue into qset
  • bec6043: 8259570: (macos) tools/jpackage tests fails with 'hdiutil: couldn't eject "disk2" - Resource busy'
  • ffbcf1b: 8260471: Change SystemDictionary::X_klass calls to vmClasses::X_klass
  • ... and 70 more: https://git.openjdk.java.net/jdk/compare/a97aedff9f622ba6816b7550588f1d429bff2483...master

Your commit was automatically rebased without conflicts.

Pushed as commit ae2c5f0.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants