Roman, just FYI. In https://github.com/openjdk/jdk/pull/10590 I see you mention code which has heavy single-threaded use of synchronized JRE types.

You mention Vector and StringBuffer as examples, maybe this bug gives another example of such code. Here we have use of the sychronized BufferedInputStream.read() to compare 2 input streams, likely written like this since the code is very trivial to write (and due to using buffered streams, the general expectation is that read() would not perform much worse than read(byte[], int, int)).

Hi Simeon,

> Here we have use of the sychronized BufferedInputStream.read() to compare 2 input streams, likely written like this since the code is very trivial to write (and due to using buffered streams, the general expectation is that read() would not perform much worse than read(byte[], int, int)).

Yes, that really should be a reasonable expectation of a buffered stream. Unfortunately, the expectation is invalidated because a call to read() implies synchronization. This is a known and unfortunate problem with the way that the stream APIs were designed many years ago. It is also, as you mention, a problem that was to a large degree hidden by biased locking (very much like Vector and StringBuffer) but has now appeared again. The OpenJDK project is aware of this problem and is considering remedies but there is no immediate plan to provide an alternative API or a JVM change to help this specific use case. The best option for clients of BufferedInputStream is to read data in bulk where possible.

To add to what Andrew Dinn says.

Changing to bulk operations is the proper remedy to this. Fixing `BufferedInputStream` on JDK side would be very hard, if possible at all. The underlying problem with locks in this scenario is atomic CAS on lock acquisition, and that is a bare minimum we have to do. Even if we manage to avoid explicit lock synchronization there, we would still need to use atomics to do safe buffered reads, which suffers from the same problem. Biased locking managed to avoid atomics on lock acquisition with intricate JVM cooperation; attempting to reproduce this in pure Java would likely fail.

Assuming the reproducer is similar to the real code that customer has, there is `Files.mismatch` since JDK 12 that should be good performance-wise. That method already does "bulk" reads and comparisons. If the real code just needs to deal with buffered read for some other purpose, I'd recommend `InputStream.readNBytes` convenience method introduced in JDK 9.

AFAICS, it is techinically possible to side-step this problem by "hacking in" the subclass of `BufferedInputStream` which overrides `read()` with unsynchronized version. This would require extensive verification that all `BufferedInputStream` instances are confined to a single thread. Then something like this would work:

```
class UnsyncBufferedInputStream extends BufferedInputStream {
    public UnsyncBufferedInputStream(InputStream in) {
        super(in);
    }

    @Override
    public int read() throws IOException {
        if (pos >= count) {
            // Let superclass handle buffers
            return super.read();
        }
        return buf[pos++] & 0xFF;
    }
}
```

Then again, if user modifies the source code, it would be more future-proof to just avoid doing per-byte operations in favor of bulk operations.

Some performance data for your amusement:
 https://gist.github.com/shipilev/71fd881f7cbf3f87028bf87385e84889

I.e. we are to not expect improvements in the JDK here?

We've already added a workaround in our code, to  prevent the performance degradation. We'll also likely have to advise users to adjust their code, if they too have code that runs into the degradation.

> I.e. we are to not expect improvements in the JDK here?

Yes, I do not see an easy way out of this degradation at the JDK side. 
Changing the user code is (unfortunately) the pragmatic way to go here.

OK, please close this as wont fix then. Thanks!

Hi Simeon.

The only JVM change that might affect this is the work Roman Kennke is doing on locking. However, I doubt that is going to do achieve the improvement provided by biased locking.

I raised this upstream on the OpenJDK hotspot-dev list and discussion did suggest the possibility of a different workaround to the one offered by Aleksey.

The difficulty here is that the inner loop is continually locking and unlocking each of the two input streams in turn. That makes it hard to optimize the code as is. However, one thing you might try is synchronizing on each stream outside the loop

        try {
            newContent = new BufferedInputStream(new FileInputStream(file));
            oldContent = new BufferedInputStream(new FileInputStream(file));
            synchronized(oldContent) {
                synchronized(newContent) {
                    int newByte = newContent.read();
                    int oldByte = oldContent.read();
                    while (newByte != -1 && oldByte != -1 && newByte == oldByte) {
                        newByte = newContent.read();
                        oldByte = oldContent.read();
                    }
                }
            }
            contentChanged = newByte != oldByte;
         . . .

The compiler may be able to elide the synchronizations associated with the inner reads when it inlines them because it knows the associated object is already locked in an outer block (this is called lock coarsening). This would be suitable for cases where the code really does need to proceed by reading one byte at a time.

If the compiler cannot perform lock coarsening wit this version it may help to move the initial reads outside the synchronizations.

Alright, thanks for the suggested workaround. I'll document it as well, under the list of suggested workarounds for users.

(In reply to Simeon Andreev from comment #6)
> OK, please close this as wont fix then. Thanks!

Thanks, did so (hopefully correctly).