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

[Brice Dutheil]

On Fri, 4 Aug 2023 15:25:11 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?
>
> Virag Purnam has updated the pull request incrementally with one additional commit since the last revision:
> 
>   7885: Graphical rendering of dependency view fails due to heap memory drain

Changing the Json format might be a breaking change, even if pooling constants is a very good improvement in my opinion, it could a format option ?

I was thinking about a **possible** alternative by introducing `IItemCollectionJsonSerializer::toJsonCharSequence` variants (and deprecating `IItemCollectionJsonSerializer::toJsonString` methods). 

<details><summary>Code suggestion</summary>


	@Deprecated
	public static String toJsonString(IItemCollection items) {
		return toJsonCharSequence(items).toString();
	}

	@Deprecated
	public static String toJsonString(Iterable<IItem> items) {
		return toJsonCharSequence(items).toString();
	}

	public static CharSequence toJsonCharSequence(IItemCollection items) {
		StringWriter sw = new StringWriter();
		IItemCollectionJsonSerializer marshaller = new IItemCollectionJsonSerializer(sw);
		try {
			marshaller.writeRecording(items);
		} catch (IOException e) {
			LOGGER.log(Level.SEVERE, "Failed to serialize recording to JSON", e);
		}
		return sw.getBuffer();
	}

	public static CharSequence toJsonCharSequence(Iterable<IItem> items) {
		StringWriter sw = new StringWriter();
		IItemCollectionJsonSerializer marshaller = new IItemCollectionJsonSerializer(sw);
		try {
			marshaller.writeEvents(items);
		} catch (IOException e) {
			LOGGER.log(Level.SEVERE, "Failed to serialize items to JSON", e);
		}
		return sw.getBuffer();
	}


</details>


The existing methods, actually build the payload using a `StringWriter` under the hood, notice that the methods build the `String` from a `StringBuffer` which is a `CharSequence`.

https://github.com/openjdk/jmc/blob/6003efc56f0b43bf2a2392d800394438c8e0a967/core/org.openjdk.jmc.flightrecorder.serializers/src/main/java/org/openjdk/jmc/flightrecorder/serializers/json/IItemCollectionJsonSerializer.java#L62-L83

The idea would be to convert the `CharSequence`, very late, i.e. when actually passing the payload. Actually `String.format` will do that for us, the other changes would be the change the declared type `String` -> `CharSequence`.

https://github.com/openjdk/jmc/blob/6003efc56f0b43bf2a2392d800394438c8e0a967/application/org.openjdk.jmc.flightrecorder.heatmap/src/main/java/org/openjdk/jmc/flightrecorder/heatmap/views/HeatmapView.java#L216-L216

Unfortunately the `org.eclipse.swt.browser.Browser` API only work with `String`s so at some point this gigantic object has to be created. And the effort to pool objects might be interesting then.

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

PR Comment: https://git.openjdk.org/jmc/pull/511#issuecomment-1691459563