Skip to content

IonSystemBuilder.copy() allocates and discards three default sub-builders on every call #1161

Description

@anuragdy

What happens

IonSystemBuilder initializes its three sub-builder fields with field initializers:

IonTextWriterBuilder textWriterBuilder = IonTextWriterBuilder.standard().withCharsetAscii();
IonBinaryWriterBuilder binaryWriterBuilder = IonBinaryWriterBuilder.standard();
IonReaderBuilder readerBuilder = IonReaderBuilder.standard();

IonTextWriterBuilder textWriterBuilder = IonTextWriterBuilder.standard().withCharsetAscii();
IonBinaryWriterBuilder binaryWriterBuilder = IonBinaryWriterBuilder.standard();
IonReaderBuilder readerBuilder = IonReaderBuilder.standard();

Java compiles instance field initializers into every constructor (JLS 12.5), including
the copy constructor — whose body then overwrites all three fields:

private IonSystemBuilder(IonSystemBuilder that)
{
this.myCatalog = that.myCatalog;
this.myStreamCopyOptimized = that.myStreamCopyOptimized;
this.textWriterBuilder = that.textWriterBuilder;
this.binaryWriterBuilder = that.binaryWriterBuilder;
this.readerBuilder = that.readerBuilder;
}

private IonSystemBuilder(IonSystemBuilder that)
{
    this.myCatalog = that.myCatalog;
    this.myStreamCopyOptimized = that.myStreamCopyOptimized;
    this.textWriterBuilder = that.textWriterBuilder;      // overwrites the initializer
    this.binaryWriterBuilder = that.binaryWriterBuilder;  // overwrites the initializer
    this.readerBuilder = that.readerBuilder;              // overwrites the initializer
}

So the three default builders are constructed and then thrown away on every copy. The copy
constructor is the only path behind copy(), and therefore also behind mutable(),
immutable(), and every withXxx() method — i.e. essentially all IonSystemBuilder use
other than standard().build().

javap -p -c on the copy constructor confirms the dead work: 74 bytes of bytecode, of
which offsets 9..32 (three invokestatic calls plus one invokevirtual) produce values
that are never read.

Impact

Measured with the JMH benchmark added in the PR below (Corretto 17.0.20, x86-64,
-prof gc, 3×1 s warmup / 5×1 s measurement, 1 fork):

benchmark before after
standard().copy() 77.76 ± 0.73 ns/op, 288 B/op 4.93 ± 0.08 ns/op, 32 B/op
standard().withCatalog(c) 80.73 ± 1.30 ns/op, 288 B/op 5.44 ± 0.15 ns/op, 32 B/op
standard().withCatalog(c).build() 336.7 ± 20.1 ns/op, 784 B/op 277.7 ± 92.8 ns/op, 528 B/op

Allocation is exact (gc.alloc.rate.norm reports ± 0.001 B/op on every row): 256 B/op of
pure waste on every copy, and the same 256 B on the build path. That is the discarded text
writer builder (72 B), binary writer builder and its catalog (152 B), and reader builder
(32 B); the surviving 32 B is the IonSystemBuilder.Mutable itself.

The wall-clock win on copy() is larger than the byte count suggests:
_Private_IonManagedBinaryWriterBuilder declares ten volatile fields, so constructing the
discarded binary writer builder emits a series of memory fences. (The build() timing is
noisy — the allocation delta is the reliable signal there.)

A production CPU profile of a large internal service attributes 0.66% of total service CPU
to IonSystemBuilder.copy.

Suggested fix

Declare the three fields without initializers and assign the defaults in the no-argument
constructor, so the copy constructor does no redundant work. standard() is unaffected —
its bytecode is identical before and after — and copies keep sharing the same sub-builder
instances by reference, exactly as today.

An alternative — hoisting the defaults into private static final constants — looks
tempting but is a silent trap: STANDARD is initialized at line 75, textually before
where those constants would be declared, so under JLS 12.4.2 it would capture null for
all three (JLS 8.3.3's illegal-forward-reference rule does not catch it, so javac accepts
it silently) and build() would NPE.

I have a PR ready for the constructor fix.

Provenance

The field initializers were introduced in #781 (commit f807803, 2024-04-09), which replaced
lazy defaulting inside build() with eager initialization so that the new
getIonTextWriterBuilder() / getIonBinaryWriterBuilder() / getReaderBuilder() getters
would never return null. That goal is preserved by assigning in the constructor.

Note: IonValueLite/IonSymbolLite contain what looks like the same pattern but is the
inverse — their copy constructors deliberately do not reassign the initialized fields, so
the initializer is the live value on the clone path. They must not be "fixed" the same way.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions