RFR: 7002: Too large graphs will freeze JMC

[Alex Ciminian]

On Wed, 13 Jan 2021 16:21:21 GMT, Marcus Hirt <hirt at openjdk.org> wrote:

>> 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](https://user-images.githubusercontent.com/348973/104131749-4987a100-5370-11eb-8ae1-b9ee8064921f.png)
>> 
>> ![Screenshot 2021-01-10 at 18 15 00](https://user-images.githubusercontent.com/348973/104131756-5c01da80-5370-11eb-9fe3-67e357d0fa00.png)
>> 
>> We will also display a similar message when the graph model has no information (no nodes):
>> 
>> ![Screenshot 2021-01-10 at 18 14 20](https://user-images.githubusercontent.com/348973/104131741-3c6ab200-5370-11eb-8d6f-1901371ab21f.png)
>> 
>> ---
>> 
>> 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.
>
> application/org.openjdk.jmc.flightrecorder.graphview/src/main/java/org/openjdk/jmc/flightrecorder/ext/graphview/graph/DotGenerator.java line 269:
> 
>> 267: 		String graphName = getConf(configuration, ConfigurationKey.Name, DEFAULT_NAME);
>> 268: 		builder.append(String.format("digraph \"%s\" {%n", graphName));
>> 269: 		int nodeCount = model.getNodes().size();
> 
> 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.

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.

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

PR: https://git.openjdk.java.net/jmc/pull/193