RFR: 7885: Graphical rendering of dependency view fails due to heap memory drain

[Brice Dutheil]

On Wed, 2 Aug 2023 06:08:22 GMT, Virag Purnam <vpurnam at openjdk.org> wrote:

> When multiple views are enabled in JMC mainly Dependency View and Heatmap View. (Issue was with Flame graph as well but it got fixed as the implementation is now based on swing component)
> 
> For the Dependency View and Heatmap View still the implementation is based on javascript and for one particular scenario JMC gives "**java.lang.OutOfMemoryError: Java heap space**". 
> 
> **Scenario:**
> For each selection in a table, views get rendered. For each click on table, 4 threads call method "**toJsonString(IItemCollection items)**" present in "**IItemCollectionJsonSerializer**" class. Within method it appends the items to a StringBuilder and then it writes it to a StringWriter. When we have multiple JFR files open in editor, or we select the table contents very fast, multiple threads call the method at the same time and all try to append and write at the same time. This results in the "**java.lang.OutOfMemoryError: Java heap space**". 
> 
> ![image](https://github.com/openjdk/jmc/assets/97600378/ae24614c-c640-4dc0-9c4c-7f70ee2f164f)
> 
> ![image](https://github.com/openjdk/jmc/assets/97600378/a41434d7-7bb1-47a0-bb7f-6a8b8e17af30)
> 
> 
> **Possible solution:** Making method  "**toJsonString(IItemCollection items)**" synchronized. I have done the changes and created a PR. 
> 
> Could you please review and provide your comments on this? If there are better way to solve this issue, could you please suggest me the same?

I believe the change shouldn't be applied on the core libraries.

core/org.openjdk.jmc.flightrecorder.serializers/src/main/java/org/openjdk/jmc/flightrecorder/serializers/json/IItemCollectionJsonSerializer.java line 62:

> 60: 	private final static Logger LOGGER = Logger.getLogger("org.openjdk.jmc.flightrecorder.json");
> 61: 
> 62: 	public synchronized static String toJsonString(IItemCollection items) {

I'm not sure putting `synchronized` on this method in core libraries is the right call. Every consumer of this method will now pay the price for a JMC issue.

I think it's better to fix the issue where the web based views use this methods.

----

Also on the topic of synchronization I believe it's better to synchronize in the method body rather than the method.

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

PR Review: https://git.openjdk.org/jmc/pull/511#pullrequestreview-1560845018
PR Review Comment: https://git.openjdk.org/jmc/pull/511#discussion_r1283038535