Re: RFR: 8332154: Memory leak in SynchronousQueue [v3]
On Mon, 20 May 2024 14:49:13 GMT, Alan Bateman wrote:
>> Viktor Klang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Moving the memory leak test for SynchronousQueue into its own test and
>> runs only for JDK20+, using VirtualThreads
>
> Looks okay, I just wonder how reliable assertDoesntLeak with concurrent
> agents, debug builds, and VM options such as a Xcomp that will challenge the
> await(10s). Minimally the timed awaits should be untimed. Another idea is
> try an ES like this:
>
> try (var executor = Executors.newVirtualThreadPerTaskExecutor()) {
> for (int i = 0; i < NUMBER_OF_ITEMS; i++) {
> executor.submit( .. producer .. );
> executor.submit(.. consumer ..);
> }
> }
>
> No need for the count down latches or the need to retry on
> InterruptedException. The close method waits for the 400 virtual threads to
> finish so you can assert that the queue is empty.
>
> The loop that waits for survivors to empty can be unbounded too. If there is
> a leak then the test will timeout.
@AlanBateman Great suggestions, Alan. I've updated the PR to reflect your
improvements.
-
PR Comment: https://git.openjdk.org/jdk/pull/19271#issuecomment-2120665477
Re: RFR: 8332154: Memory leak in SynchronousQueue [v3]
On Fri, 17 May 2024 13:19:19 GMT, Viktor Klang wrote:
>> Local testing seems to indicate that this fix (which mirrors what's done in
>> the FIFO mode) addresses the problem. Regression test added for JDK20+
>
> Viktor Klang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Moving the memory leak test for SynchronousQueue into its own test and runs
> only for JDK20+, using VirtualThreads
Looks okay, I just wonder how reliable assertDoesntLeak will concurrent agents,
debug builds, and VM options such as a Xcomp that will challenge the
await(10s). Minimally the timed awaits should be untimed. Another idea is try
an ES like this:
try (var executor = Executors.newVirtualThreadPerTaskExecutor()) {
for (int i = 0; i < NUMBER_OF_ITEMS; i++) {
executor.submit( .. producer .. );
executor.submit(.. consumer ..);
}
}
No need for the count down latches or the need to retry on
InterruptedException. The close method waits for the 400 virtual threads to
finish so you can assert that the queue is empty.
The loop that waits for survivors to empty can be unbounded too. If there is a
leak then the test will timeout.
-
PR Comment: https://git.openjdk.org/jdk/pull/19271#issuecomment-2120616967
Re: RFR: 8332154: Memory leak in SynchronousQueue [v3]
On Fri, 17 May 2024 13:49:44 GMT, Doug Lea wrote: >> Viktor Klang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Moving the memory leak test for SynchronousQueue into its own test and >> runs only for JDK20+, using VirtualThreads > > Sure. Seems reasonable. @DougLea Perfect, thanks for reviewing! - PR Comment: https://git.openjdk.org/jdk/pull/19271#issuecomment-2118065006
Re: RFR: 8332154: Memory leak in SynchronousQueue [v3]
On Fri, 17 May 2024 13:19:19 GMT, Viktor Klang wrote: >> Local testing seems to indicate that this fix (which mirrors what's done in >> the FIFO mode) addresses the problem. >> >> But with that said, I haven't come up with a decent way of adding some form >> of regression test. Suggestions are most welcome. /cc @DougLea > > Viktor Klang has updated the pull request incrementally with one additional > commit since the last revision: > > Moving the memory leak test for SynchronousQueue into its own test and runs > only for JDK20+, using VirtualThreads Sure. Seems reasonable. - PR Comment: https://git.openjdk.org/jdk/pull/19271#issuecomment-2117653564
Re: RFR: 8332154: Memory leak in SynchronousQueue [v3]
> Local testing seems to indicate that this fix (which mirrors what's done in > the FIFO mode) addresses the problem. > > But with that said, I haven't come up with a decent way of adding some form > of regression test. Suggestions are most welcome. /cc @DougLea Viktor Klang has updated the pull request incrementally with one additional commit since the last revision: Moving the memory leak test for SynchronousQueue into its own test and runs only for JDK20+, using VirtualThreads - Changes: - all: https://git.openjdk.org/jdk/pull/19271/files - new: https://git.openjdk.org/jdk/pull/19271/files/f365c5f0..cc0a2014 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19271=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19271=01-02 Stats: 181 lines in 3 files changed: 116 ins; 65 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19271.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19271/head:pull/19271 PR: https://git.openjdk.org/jdk/pull/19271
