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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

7002: Too large graphs will freeze JMC #193

Closed
wants to merge 6 commits into from

Conversation

@cimi
Copy link
Contributor

@cimi cimi commented Jan 10, 2021

This PR adds a control to limit the maximum number of nodes that can be rendered in the JMC graph view.

At the moment, rendering very large graphs makes the application unresponsive so we want to guard against this by setting a limit on the maximum number of nodes that will be rendered. When this limit is exceeded, we don't try to render our StacktraceGraphModel into a graphiz dot string and instead we generate a simple dot string with one node and a message informing the user that they are trying to render too many nodes.

I spent a couple of hours fighting with SWT to get the basic dropdown functionality working in the component toolbar. I took some inspiration from the FlameView component's toolbar actions and I found some (not great) examples with Google. I'm happy to use other components - this seemed to be the simplest from what I found. Please let me know if there's anything better. 馃槂


Screenshot 2021-01-10 at 18 14 39

Screenshot 2021-01-10 at 18 15 00

We will also display a similar message when the graph model has no information (no nodes):

Screenshot 2021-01-10 at 18 14 20


Other things we've considered:

  • add an icon instead of the text in the toolbar - we'll leave the text label for now
  • create text labels instead of the hardcoded strings I'm using now - we decided not to do localisation while this view is experimental
  • I was on the fence about adding a 'no limit' option in the dropdown and decided against it. I guess it's useful in development when trying out pruning strategies, but it might surprise people unfamiliar with how it works and freeze their apps. In development, we can just remove the limit when we want to try things out.

Progress

  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JMC-7002: Too large graphs will freeze JMC

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jmc pull/193/head:pull/193
$ git checkout pull/193

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 10, 2021

馃憢 Welcome back cimi! 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.

@thegreystone
Copy link
Member

@thegreystone thegreystone commented Jan 10, 2021

Looks good to me. I can try my hand at an icon. :)

@thegreystone
Copy link
Member

@thegreystone thegreystone commented Jan 10, 2021

I think, perhaps a less on-the-nose-message for when there is no data would be nice. :)

@cimi
Copy link
Contributor Author

@cimi cimi commented Jan 10, 2021

I've changed the empty graph display to a simple label with a small font size. It seems to work ok and doesn't require other changes to the html or javascript 馃槂 With this method, it could become very small if the graph view window size is small. Nevertheless, I prefer this way since hacking html/css without touching the javascript is complicated: the SVG fills the page even when we have empty graphs, so I can't just overlay the graph over the text.

It looks like this now:

Screenshot 2021-01-10 at 20 10 26

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

@mlbridge mlbridge bot commented Jan 12, 2021

Webrevs

StringBuilder builder = new StringBuilder(2048);
String graphName = getConf(configuration, ConfigurationKey.Name, DEFAULT_NAME);
builder.append(String.format("digraph \"%s\" {%n", graphName));

int nodeCount = model.getNodes().size();
Copy link
Member

@thegreystone thegreystone Jan 13, 2021

I'm not sure I like the idea of special messages like this being passed as a "special" di-graph. I'd prefer this special check to be done outside of the toDot serializer, but I think it's okay since the graph view is sort of early access. It's certainly not worth holding up the release for.

Copy link
Contributor Author

@cimi cimi Jan 13, 2021

Agreed, this is a slight abuse of the serialiser 馃槂 but having it this way keeps the code simpler. We won't need to have special conditions before serialisation (we'd want to avoid if there are too many nodes), avoids adding extra behaviour in Java for passing messages and avoids adding functionality in the JS to do the toggle between displaying messages and showing the graph. Doing all this is not too difficult, but this way is simpler and doesn't introduce any new components.

We also do a similar thing for the flame graph: we display 'No stacktrace information available' as a flame graph with one root frame.

Related, doing the toggle with pure CSS (i.e. no JS) is not straightforward because graphviz resizes to cover the entire area. It might be possible to hack something in CSS that produces different display depending on the structure of the SVG generated - but given the state of the browser running inside our application, I don't think this is a good idea even if we get it working.

@openjdk
Copy link

@openjdk openjdk bot commented Jan 13, 2021

@cimi 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:

7002: Too large graphs will freeze JMC

Reviewed-by: hirt

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 4 new commits pushed to the master branch:

  • deb32e7: 7081: Update Copyright year to 2021, Validate and update license attributes
  • 549cba0: 7079: Numeric values in automated analysis summary is not resolved
  • 1e7cd87: 7076: Rules and Analysis code has exceptions during uitests
  • a21771c: 7032: The banner is missing from help page

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 (@thegreystone) 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 13, 2021
@cimi
Copy link
Contributor Author

@cimi cimi commented Jan 13, 2021

/integrate

@openjdk openjdk bot added the sponsor label Jan 13, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 13, 2021

@cimi
Your change (at version 67aec35) is now ready to be sponsored by a Committer.

@thegreystone
Copy link
Member

@thegreystone thegreystone commented Jan 13, 2021

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented Jan 13, 2021

@thegreystone @cimi Since your change was applied there have been 4 commits pushed to the master branch:

  • deb32e7: 7081: Update Copyright year to 2021, Validate and update license attributes
  • 549cba0: 7079: Numeric values in automated analysis summary is not resolved
  • 1e7cd87: 7076: Rules and Analysis code has exceptions during uitests
  • a21771c: 7032: The banner is missing from help page

Your commit was automatically rebased without conflicts.

Pushed as commit b6fe7ca.

馃挕 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
2 participants