fix: Reduced garbage on InsightsReport's serialization

[franz1981]

Got a couple of questions before proceeding (adding tests and/or other changes).

It appears that in the multi client case each client serialize the same report many times, but they have the option (for each one) to decorate it:
There is a way to safely assume it won't be changed (by decoration?) and just serialize it once?

I refer to this

insights-java-client/api/src/main/java/com/redhat/insights/http/InsightsMultiClient.java

Line 43 in 21aca0c

client.sendInsightsReport(filename, report);

code path.
It would be nice to pass through the previously serialized report, during the iteration, if we an assume it to not be changed.

[franz1981]

@ehsavoie too

[franz1981]

Another thing that could be fixed in the original code is to address the double copy (from heap ByteBuffer to direct one) hidden in the file writes, hitting https://github.com/openjdk/jdk/blob/9f98136c3a00ca24d59ffefd58308603b58110c7/src/java.base/share/classes/sun/nio/ch/IOUtil.java#L94

which cause unwanted and unpleasant creation of pooled (ie a proper leak) direct Buffer which would impact on EAP off-heap/direct memory footprint, see https://github.com/openjdk/jdk/blob/33011ea19bb29e88ce18a138a8fa8b34f8c97407/src/java.base/share/classes/sun/nio/ch/Util.java#L77 which show that by default, such temporary direct buffers are (thread-local) cached without any limits, per buffer.

In this regard, RandomAccessFile which can perform stack allocation or a temporary off-heap allocation (via new/free, really), seems a much better choice, to save any impact on long running applications like EAP: see https://github.com/openjdk/jdk/blob/33011ea19bb29e88ce18a138a8fa8b34f8c97407/src/java.base/share/native/libjava/io_util.c#L167
TLDR for reports which are > 8192, RandomAccessFile woudl create a new buffer (via malloc/free) to hold the array content and perform the actual file write (freeing up it immediately).
For reports smaller than that, instead, it would create a stack allocated buffer, without interacting with the native allocator.
@kittylyst wdyt?

[franz1981]

@kittylyst I've send an additional (optional) commit to address the comment at #110 (comment)

InsightsFileWritingClientTest::testWriteToTmp has become invalid, given that
it doesn't use serialize to produce an intermediate byte[].
What's the best way to test with mocks this ?

[kittylyst]

Hi folks. Thanks for the detailed work on this PR.

I am quite lost in all the comments and so forth, and as a result I'm not clear on the design intent here. Can we put a call in next week to discuss @franz1981 ? Also - can you link to the test cases that indicate the issue that we're trying to solve by this change?

[franz1981]

I am quite lost in all the comments and so forth, and as a result I'm not clear on the design intent here.

Yep, let me summarize the main points here:

1. the current usage of Jackson is prone to creating multiple copies of the same serialized data, which could be improved by using the right API (which make uses of an internal byte[] own by Jackson itself): as a positive consequence of this, I have removed the need to encode again a String into its UTF-8 byte[] encoded form, saving others additional copy
2. using Files::write hides (for OpenJDK, at least) a subtle off-heap direct leak due to using FileChannel::write which stores a thread local direct buffer to speed up subsequent writes performed by the same Thread. This doesn't work well with long running applications like Wildfly/EAP (and depends by the frequency by which reports are produced vs up time and user limits for direct memory allocations)
3. The previous 2 changes has allowed me to use the Jackson API to directly stream the JSON over the RandomAccessFile provided, saving another additional copy and (String + byte[]) allocations.

We can go through these with some profiling data during the call plus the other topics that @ehsavoie want to talk about, thanks!

[jponge]

Nevermind the failure on the latest conventional commit checks, it's because the original PR didn't have a matching title (happened to me before)

[jponge]

Superseded by #117