Re: RFR: 8330748: ByteArrayOutputStream.writeTo(OutputStream) pins carrier [v2]
On Wed, 24 Apr 2024 14:54:34 GMT, Viktor Klang wrote:
>> Currently we have
>>
>> public void writeTo(OutputStream out) throws IOException {
>> if (Thread.currentThread().isVirtual()) {
>> out.write(toByteArray());
>> } else synchronized (this) {
>> out.write(buf, 0, count);
>> }
>> }
>>
>> where `toByteArray()` is `synchronized`, but here I would think that we'd
>> want to replace it with simply `Arrays.copyOf(buf, count)` without the
>> `synchronized`, no?
>
> @bplb My interpretation was that we didn't want to hold the monitor *during*
> out.write().
Please see 8076291fb3d097ef67d0b59b9be3c8b762aad7cf.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18901#discussion_r1578116805
Re: RFR: 8330748: ByteArrayOutputStream.writeTo(OutputStream) pins carrier [v2]
On Wed, 24 Apr 2024 14:52:45 GMT, Brian Burkhalter wrote:
>> Using a subclass to count the number of invocations of toByteArray seems a
>> bit strange but in general it is more robust to not rely on a method that
>> may be overridden by a subclass. So I think the suggestion is good.
>
> Currently we have
>
> public void writeTo(OutputStream out) throws IOException {
> if (Thread.currentThread().isVirtual()) {
> out.write(toByteArray());
> } else synchronized (this) {
> out.write(buf, 0, count);
> }
> }
>
> where `toByteArray()` is `synchronized`, but here I would think that we'd
> want to replace it with simply `Arrays.copyOf(buf, count)` without the
> `synchronized`, no?
@bplb My interpretation was that we didn't want to hold the monitor *during*
out.write().
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18901#discussion_r1578034781
Re: RFR: 8330748: ByteArrayOutputStream.writeTo(OutputStream) pins carrier [v2]
On Wed, 24 Apr 2024 07:08:20 GMT, Alan Bateman wrote:
>> So do we think it better not to invoke `toByteArray` here?
>
> Using a subclass to count the number of invocations of toByteArray seems a
> bit strange but in general it is more robust to not rely on a method that may
> be overridden by a subclass. So I think the suggestion is good.
Currently we have
public void writeTo(OutputStream out) throws IOException {
if (Thread.currentThread().isVirtual()) {
out.write(toByteArray());
} else synchronized (this) {
out.write(buf, 0, count);
}
}
where `toByteArray()` is `synchronized`, but here I would think that we'd want
to replace it with simply `Arrays.copyOf(buf, count)` without the
`synchronized`, no?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18901#discussion_r1578031656
Re: RFR: 8330748: ByteArrayOutputStream.writeTo(OutputStream) pins carrier [v2]
On Tue, 23 Apr 2024 18:08:34 GMT, Brian Burkhalter wrote: >> I was thinking more of a subclass that counted invocations to public methods >> or metering which would cause subclass to double the counts when calling via >> virtual thread. > > So do we think it better not to invoke `toByteArray` here? Using a subclass to count the number of invocations of toByteArray seems a bit strange but in general it is more robust to not rely on a method that may be overridden by a subclass. So I think the suggestion is good. - PR Review Comment: https://git.openjdk.org/jdk/pull/18901#discussion_r1577388594
Re: RFR: 8330748: ByteArrayOutputStream.writeTo(OutputStream) pins carrier [v2]
On Tue, 23 Apr 2024 06:20:47 GMT, Alan Bateman wrote:
>> Brian Burkhalter has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Correct ID in test @bug tag
>
> test/jdk/java/io/ByteArrayOutputStream/WriteToReleasesCarrier.java line 78:
>
>> 76: }
>> 77: } finally {
>> 78: LockSupport.unpark(vthread1);
>
> It might be clearer if you add vthread1.join() after the unpark. It's not
> strictly needed here as scheduler::close will block until the carrier
> terminates so that will guarantee that the virtual thread has unmounted.
Added `vthread1.join()` after the unpark in
1dd59b7bf7aa2087845ad7806a5afbed2b5ea1b5.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18901#discussion_r1577076109
Re: RFR: 8330748: ByteArrayOutputStream.writeTo(OutputStream) pins carrier [v2]
On Tue, 23 Apr 2024 17:35:42 GMT, Jason Mehrens wrote: >> A good question. The buf/count fields are protected so the subclass has >> direct access to the bytes. So while it could Arrays.copy the bytes, it >> doesn't help with a buggy subclass that is changing bytes without >> synchronization. > > I was thinking more of a subclass that counted invocations to public methods > or metering which would cause subclass to double the counts when calling via > virtual thread. So do we think it better not to invoke `toByteArray` here? - PR Review Comment: https://git.openjdk.org/jdk/pull/18901#discussion_r1576686390
Re: RFR: 8330748: ByteArrayOutputStream.writeTo(OutputStream) pins carrier [v2]
On Tue, 23 Apr 2024 16:08:13 GMT, Alan Bateman wrote:
>> src/java.base/share/classes/java/io/ByteArrayOutputStream.java line 164:
>>
>>> 162: public void writeTo(OutputStream out) throws IOException {
>>> 163: if (Thread.currentThread().isVirtual()) {
>>> 164: out.write(toByteArray());
>>
>> Would it be better to avoid calling a public method `toByteArray` encase
>> subclass is overriding it?
>
> A good question. The buf/count fields are protected so the subclass has
> direct access to the bytes. So while it could Arrays.copy the bytes, it
> doesn't help with a buggy subclass that is changing bytes while
> synchronization.
I was thinking more of a subclass that counted invocations to public methods or
metering which would cause subclass to double the counts when calling via
virtual thread.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18901#discussion_r1576637177
Re: RFR: 8330748: ByteArrayOutputStream.writeTo(OutputStream) pins carrier [v2]
On Tue, 23 Apr 2024 11:16:01 GMT, Jason Mehrens wrote:
>> Brian Burkhalter has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Correct ID in test @bug tag
>
> src/java.base/share/classes/java/io/ByteArrayOutputStream.java line 164:
>
>> 162: public void writeTo(OutputStream out) throws IOException {
>> 163: if (Thread.currentThread().isVirtual()) {
>> 164: out.write(toByteArray());
>
> Would it be better to avoid calling a public method `toByteArray` encase
> subclass is overriding it?
A good question. The buf/count fields are protected so the subclass has direct
access to the bytes. So while it could Arrays.copy the bytes, it doesn't help
with a buggy subclass that is changing bytes while synchronization.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18901#discussion_r1576523112
Re: RFR: 8330748: ByteArrayOutputStream.writeTo(OutputStream) pins carrier [v2]
On Mon, 22 Apr 2024 19:49:44 GMT, Brian Burkhalter wrote:
>> Prevent blocking due to a carrier thread not being released when
>> `ByteArrayOutputStream.writeTo` is invoked from a virtual thread.
>
> Brian Burkhalter has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Correct ID in test @bug tag
src/java.base/share/classes/java/io/ByteArrayOutputStream.java line 164:
> 162: public void writeTo(OutputStream out) throws IOException {
> 163: if (Thread.currentThread().isVirtual()) {
> 164: out.write(toByteArray());
Would it be better to avoid calling a public method `toByteArray` encase
subclass is overriding it?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18901#discussion_r1576078358
Re: RFR: 8330748: ByteArrayOutputStream.writeTo(OutputStream) pins carrier [v2]
On Tue, 23 Apr 2024 07:27:02 GMT, Alan Bateman wrote: > > Has the alternative of moving to a j.u.c.Lock (Needs Reentrant or not?) > > been explored/benchmarked? > > Yes, decided not to do because it's just guarding access to a byte[] and any > changes can influence the inlining. Plus, it would need to continue to use > monitors when extended as the subclass may assume synchronization in the > super class. Limiting it to just the problematic writeTo method seem the > least risky. That's true, good points across the board. - PR Comment: https://git.openjdk.org/jdk/pull/18901#issuecomment-2071869067
Re: RFR: 8330748: ByteArrayOutputStream.writeTo(OutputStream) pins carrier [v2]
On Tue, 23 Apr 2024 07:11:34 GMT, Viktor Klang wrote: > Has the alternative of moving to a j.u.c.Lock (Needs Reentrant or not?) been > explored/benchmarked? Yes, decided not to do because it's just guarding access to a byte[] and any changes can influence the inlining. Plus, it would need to continue to use monitors when extended as the subclass may assume synchronization in the super class. Limiting it to just the problematic writeTo method seem the least risky. - PR Comment: https://git.openjdk.org/jdk/pull/18901#issuecomment-2071613230
Re: RFR: 8330748: ByteArrayOutputStream.writeTo(OutputStream) pins carrier [v2]
On Mon, 22 Apr 2024 19:49:44 GMT, Brian Burkhalter wrote: >> Prevent blocking due to a carrier thread not being released when >> `ByteArrayOutputStream.writeTo` is invoked from a virtual thread. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > Correct ID in test @bug tag Has the alternative of moving to a j.u.c.Lock (Needs Reentrant or not?) been explored/benchmarked? - PR Comment: https://git.openjdk.org/jdk/pull/18901#issuecomment-2071589121
Re: RFR: 8330748: ByteArrayOutputStream.writeTo(OutputStream) pins carrier [v2]
On Mon, 22 Apr 2024 19:49:44 GMT, Brian Burkhalter wrote:
>> Prevent blocking due to a carrier thread not being released when
>> `ByteArrayOutputStream.writeTo` is invoked from a virtual thread.
>
> Brian Burkhalter has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Correct ID in test @bug tag
As a temporary workaround then I think this is okay. It does mean there needs
to a heap spec for the snapshot. Aside from that, the behavioral difference
should only be observable to something that is relying on unspecified behavior.
BAOS does not document anything about thread safety or that its methods are
synchronized.
test/jdk/java/io/ByteArrayOutputStream/WriteToReleasesCarrier.java line 78:
> 76: }
> 77: } finally {
> 78: LockSupport.unpark(vthread1);
It might be clearer if you add vthread1.join() after the unpark. It's not
strictly needed here as scheduler::close will block until the carrier
terminates so that will guarantee that the virtual thread has unmounted.
test/jdk/java/io/ByteArrayOutputStream/WriteToReleasesCarrier.java line 124:
> 122: * Returns a builder to create virtual threads that use the given
> scheduler.
> 123: */
> 124: static Thread.Builder.OfVirtual virtualThreadBuilder(Executor
> scheduler) throws Exception {
At some point we will move some of the infrastructure for tests like this to a
shared location in the test tree.
-
Marked as reviewed by alanb (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18901#pullrequestreview-2016331774
PR Review Comment: https://git.openjdk.org/jdk/pull/18901#discussion_r1575700125
PR Review Comment: https://git.openjdk.org/jdk/pull/18901#discussion_r1575700737
Re: RFR: 8330748: ByteArrayOutputStream.writeTo(OutputStream) pins carrier [v2]
On Mon, 22 Apr 2024 19:49:44 GMT, Brian Burkhalter wrote: >> Prevent blocking due to a carrier thread not being released when >> `ByteArrayOutputStream.writeTo` is invoked from a virtual thread. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > Correct ID in test @bug tag Tiers 1-3 passed on the usual platforms. - PR Comment: https://git.openjdk.org/jdk/pull/18901#issuecomment-2071221654
Re: RFR: 8330748: ByteArrayOutputStream.writeTo(OutputStream) pins carrier [v2]
> Prevent blocking due to a carrier thread not being released when > `ByteArrayOutputStream.writeTo` is invoked from a virtual thread. Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision: Correct ID in test @bug tag - Changes: - all: https://git.openjdk.org/jdk/pull/18901/files - new: https://git.openjdk.org/jdk/pull/18901/files/08bb9cb6..57e4917b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18901&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18901&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18901.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18901/head:pull/18901 PR: https://git.openjdk.org/jdk/pull/18901
