Reduce memory pressure in FormattingTestReporter

[ash211]

Before this PR

I saw OOMs on an internal project that I think were contributed to by the code in this class's writeAllOutput() method. That method holds the output multiple times in memory at once:

1. accumulated into StringWriter
2. flatted into the String contents
3. As an array of substrings, in contents.split("\n")

For the OOM I observed, the failed allocation was in this stacktrace:

Java frames: (J=compiled Java code, j=interpreted, Vv=VM code)
v  ~RuntimeStub::_new_array_nozero_Java 0x0000714dc3c619a7
J 4916 c2 java.lang.String.<init>(Ljava/lang/AbstractStringBuilder;Ljava/lang/Void;)V [email protected] (99 bytes) @ 0x0000714dc4360070 [0x0000714dc435fb60+0x0000000000000510]
J 31391 c1 java.lang.StringBuffer.toString()Ljava/lang/String; [email protected] (34 bytes) @ 0x0000714dbd95eb34 [0x0000714dbd95e9e0+0x0000000000000154]
J 30067 c1 java.io.StringWriter.toString()Ljava/lang/String; [email protected] (8 bytes) @ 0x0000714dbe3750ac [0x0000714dbe375040+0x000000000000006c]
j  com.palantir.witchcraft.java.logging.gradle.testreport.FormattingTestReporter$FormattingTestResultsProvider.writeAllOutput(JLorg/gradle/api/tasks/testing/TestOutputEvent$Destination;Ljava/io/Writer;)V+38
j  org.gradle.api.internal.tasks.testing.report.ClassPageRenderer$2.doExecute(Lorg/gradle/internal/html/SimpleHtmlWriter;)V+39
j  org.gradle.api.internal.tasks.testing.report.ClassPageRenderer$2.doExecute(Ljava/lang/Object;)V+5
j  org.gradle.internal.ErroringAction.execute(Ljava/lang/Object;)V+2
j  org.gradle.api.internal.tasks.testing.report.PageRenderer$1.render(Lorg/gradle/api/internal/tasks/testing/report/CompositeTestResults;Lorg/gradle/internal/html/SimpleHtmlWriter;)V+5

After this PR

==COMMIT_MSG==
Reduce memory pressure in FormattingTestReporter
==COMMIT_MSG==

This PR refactors the FormattingTestReporter to hold the contents in memory fewer times. Instead of accumulating all the lines into one StringWriter stringWriter, we accumulate characters until we find a newline. Then process that line and continue. And instead of writing the line into an intermedia String and then that String into the delegate, we write directly into the delegate.

Possible downsides?

• by working at the character level and checking for newlines char by char, we lose batching and mechanical sympathy
• there is existing test coverage in TestReportFormattingPluginIntegrationSpec that verifies the results, but there isn't test coverage for performance, which may have regressed. And this PR doesn't currently include test coverage asserting that large amounts of console output can now be processed even with small heaps (difficult to add tests for)

[changelog-app]

Generate changelog in changelog/@unreleased

Type (Select exactly one)

• Feature (Adding new functionality)
• Improvement (Improving existing functionality)
• Fix (Fixing an issue with existing functionality)
• Break (Creating a new major version by breaking public APIs)
• Deprecation (Removing functionality in a non-breaking way)
• Migration (Automatically moving data/functionality to a new system)

Description

Reduce memory pressure in FormattingTestReporter

Check the box to generate changelog(s)

• Generate changelog entry

[changelog-app]

✅ Successfully generated changelog entry!

What happened?

Your changelog entries have been stored in the database as part of our migration to ChangelogV3.

Need to regenerate?

Simply interact with the changelog bot comment again to regenerate these entries.

[schlosna]

Do we need to synchronize on the writer to ensure this is written atomically?

Suggested change

	lineProcessor.accept(line, delegate);
	delegate.write("\n");
	synchronized (this) {
	lineProcessor.accept(line, delegate);
	delegate.write('\n');
	}

[ash211]

if we're going to make this LineProcessingWriter thread-safe, then I think we need to protect the lineBuffer from concurrent writes, and maybe then add synchronized keyword to the write() and close() methods.

I'm not sure we need it to be thread-safe though... even multiple calls to the writeAllOutput method here would create new LineProcessingWriters

[schlosna]

java.io.Writer exposes a protected lock field for synchronization to ensure that output (e.g. line with new line) is written as an atomic unit. I think something like #763 would help here in cases where there might be concurrent things writing to stdout/stderr. I think we can also provide an optimized write(String str, int off, int len) that uses vectorized String#indexOf and bulk char array copy from String to StringBuilder.

[stale]

This PR has been automatically marked as stale because it has not been touched in the last 14 days. If you'd like to keep it open, please leave a comment or add the 'long-lived' label, otherwise it'll be closed in 7 days.

[autorelease3]

Released 2.23.0