AVRO-3987 [Java] Concurrency improvement for ReflectData FieldAccessors

[ashley-taylor]

What is the purpose of the change

Improve the performance by removing synchronisation calls.

Running the ReflectRecordTest with 8 threads on a M2 Macbook pro

before change

    Benchmark                  Mode  Cnt        Score        Error  Units
    ReflectRecordTest.decode  thrpt    3  4,157,551.230 ± 831123.819  ops/s
    ReflectRecordTest.encode  thrpt    3  4,926,092.511 ± 455798.533  ops/s

after

    Benchmark                  Mode  Cnt         Score         Error  Units
    ReflectRecordTest.decode  thrpt    3  12,221,568.529 ± 1308392.259  ops/s
    ReflectRecordTest.encode  thrpt    3  12,569,201.802 ± 1898124.234  ops/s

Verifying this change

This change is already covered by existing tests, such as (please describe tests).

org.apache.avo.reflect already tests this code

Documentation

• Does this pull request introduce a new feature? no
• If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

[ashley-taylor]

to recreate the benchmark, you need to change the threads value here to be greater than 1

avro/lang/java/perf/src/main/java/org/apache/avro/perf/Perf.java

Line 75 in 33668bf

.warmupIterations(warmupIterations).measurementIterations(measurementIterations).forks(1).threads(1)

[martin-g]

Without synchronization this.bySchema could be modified several times by now. Would that be a problem ?

[ashley-taylor]

Since the object is derived It won't cause an issue. There could be duplicated generation across threads. Causing some wasted compute at the start of processing. But since this.bySchema is always replaced with a new map with no future modifications concurrent read are safe.

[KalleOlaviNiemitalo]

I'm not really a Java programmer but the this.bySchema = bySchema assignment seems OK, assuming that the volatile causes a memory barrier that ensures the effects of bySchema.put(schema, result) will be seen by other threads that read this.bySchema after the assignment.

However, I wonder about bySchema.get(schema). Is WeakHashMap.get formally documented to be safe for concurrent calls? Each time WeakHashMap.get is called, WeakHashMap.expungeStaleEntries can delete some entries, and although WeakHashMap.expungeStaleEntries synchronizes those deletions against each other, it doesn't seem to synchronize them against any reads that WeakHashMap.get may be doing on other threads.

[ashley-taylor]

@martin-g anything you need to get this merged?

[martin-g]

@martin-g anything you need to get this merged?

I'll leave it to a Java developer because I am not that familiar with the Java SDK.