RFR: 8254078: DataOutputStream is very slow post-disabling of Biased Locking [v4]

[Aleksey Shipilev]

On Thu, 8 Oct 2020 09:36:01 GMT, Andrew Haley <aph at openjdk.org> wrote:

>> DataOutputStream is very slow post-disabling of Biased Locking. This
>> was discovered when benchmarking a transaction library, which showed
>> significant performance loss when moving to JDK 15. WIth some small
>> changes to DataOutputStream we can get the performance back. There's a
>> JMH benchmark at
>> http://cr.openjdk.java.net/~aph/JDK-8254078/jmh-tests.tar
>> 
>> Some Stream classes use very fine-grained locking.
>> 
>> In particular, writeInt is defined like this:
>> 
>>         out.write((v >>> 24) & 0xFF);
>>         out.write((v >>> 16) & 0xFF);
>>         out.write((v >>> 8) & 0xFF);
>>         out.write((v >>> 0) & 0xFF);
>>         incCount(4);
>> 
>> Unfortunately, ByteArrayOutputStream.write(byte) is defined like this:
>> 
>>     public synchronized void write(int b) {
>>         ensureCapacity(count + 1);
>>         buf[count] = (byte) b;
>>         count += 1;
>>     }
>> 
>> so we acquire and release a lock for every byte that is output.
>> 
>>  For example, writing 4kb of ints goes from 17.3 us/op to 53.9 us/op when biased locking is disabled:
>> 
>> 
>> +UseBiasedLocking DataOutputStreamTest.dataOutputStreamOverByteArray avgt 6 53.895 ± 5.126 us/op
>> -UseBiasedLocking DataOutputStreamTest.dataOutputStreamOverByteArray avgt 6 17.291 ± 4.430 us/op
>> 
>> There are refactorings of DataOutputStream we can do to mitigate this.
>
> Andrew Haley has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8254078: DataOutputStream is very slow post-disabling of Biased Locking

I have comments about the benchmark code style :)

test/micro/org/openjdk/bench/java/io/DataOutputStreamTest.java line 41:

> 39:
> 40:     @State(Scope.Benchmark)
> 41:     public static class BenchmarkState {

You don't need a separate nested class to carry `@State`, AFAICS. Move everything to `DataOutputStreamTest` body, and
put the `@State` annotation near other class annotations (`@Warmup` and friends).

test/micro/org/openjdk/bench/java/io/DataOutputStreamTest.java line 43:

> 41:     public static class BenchmarkState {
> 42:         @Param({"4096"}) int SIZE;
> 43:         @Param({"char", "short", "int", /*"string"*/}) String BASIC_TYPE;

- Do we use `"string"` case or not?
- Java style: all caps are reserved for `static final` fields, these should be `size` and `basicType`
- JMH supports `@Param` over `Enum`-s, so this `String` dance does not seem necessary.

test/micro/org/openjdk/bench/java/io/DataOutputStreamTest.java line 66:

> 64:                 };
> 65:                 outputString = new String(new byte[SIZE]);
> 66:             } catch (Exception e) {

`@Setup` is allowed to declare `throws Exception`, which would allow removing this `try-catch` block.

test/micro/org/openjdk/bench/java/io/DataOutputStreamTest.java line 117:

> 115:             case STRING: writeString(state, dataOutput); break;
> 116:         }
> 117:     }

So, the way I see it, this can be replaced by:

    public void write(DataOutput dataOutput) {
        switch (basicType) {
            case CHAR:
                for (int i = 0; i < SIZE; i += 2) {
                    dataOutput.writeChar(i);
                }
                break;
            case SHORT:
                for (int i = 0; i < SIZE; i += 2) {
                    dataOutput.writeShort(i);
                }
                break;
            case INT:
                for (int i = 0; i < SIZE; i += 4) {
                    dataOutput.writeInt(i);
                }
                break;
            case STRING:
                dataOutput.writeChars(state.outputString);
                break;
        }
    }

...thus keeping all the code in one place, and simplifying the benchmark.

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

Changes requested by shade (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/542