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

[Alan Bateman]

On Wed, 7 Oct 2020 14:09:46 GMT, Andrew Haley <aph at openjdk.org> wrote:

>> There is a pre-existing race condition on use of the writeBuffer.
>> It is allocated per DataOutputStream but is unsynchronized.
>> Only the write(byte) and write(byte[], off, len) methods are synchronized.
>> Will that be/become a problem?
>
>> There is a pre-existing race condition on use of the writeBuffer.
>> It is allocated per DataOutputStream but is unsynchronized.
>> Only the write(byte) and write(byte[], off, len) methods are synchronized.
>> Will that be/become a problem?
> 
> Hard to say. There isn't a guarantee in the specification. After my patch, writeInt is
> 
>     public final void writeInt(int v) throws IOException {
>         writeBuffer[0] = (byte)(v >>> 24);
>         writeBuffer[1] = (byte)(v >>> 16);
>         writeBuffer[2] = (byte)(v >>>  8);
>         writeBuffer[3] = (byte)(v >>>  0);
>         out.write(writeBuffer, 0, 4);
>         incCount(4);
>     }
> 
> whereas writeLong() is
>     public final void writeLong(long v) throws IOException {
>         writeBuffer[0] = (byte)(v >>> 56);
>         writeBuffer[1] = (byte)(v >>> 48);
>         writeBuffer[2] = (byte)(v >>> 40);
>         writeBuffer[3] = (byte)(v >>> 32);
>         writeBuffer[4] = (byte)(v >>> 24);
>         writeBuffer[5] = (byte)(v >>> 16);
>         writeBuffer[6] = (byte)(v >>>  8);
>         writeBuffer[7] = (byte)(v >>>  0);
>         out.write(writeBuffer, 0, 8);
>         incCount(8);
>     }
> 
> I know there's a history of Java programmers writing to the implementation, not the specification, but ISTM that if
> writeLong() can be written this way so can writeInt().

DataOutputStream is a JDK 1.0 era class that doesn't specify whether it is safe for use by multiple threads or not.
Some methods are synchronized, some are not. The writeBuffer used by writeLong means it is not thread safe. The
writeShort and writeInt methods aren't thread safe so changing them to use the writeBuffer doesn't make things any
worse. Other examples are the "bytearr" used by writeUTF, the writeChars and writeBytes methods, and the protected
"written" field that sub-classes can use to monitor the number of bytes written.

It might be helpful to add a statement to the DataOutputStream class description to say that it not safe for by
concurrent threads and it needs appropriate synchronization to coordinate multiple writers. This issue or another issue
doesn't matter.

Can the benchmark be turned into a microbenchmark for test/micro?

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

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