Re: RFR: 8255072: [TESTBUG] com/sun/jdi/EATests.java should not fail if expected VMOutOfMemoryException is not thrown [v3]
On Thu, 22 Oct 2020 20:35:29 GMT, Richard Reingruber wrote: >> The following test cases try to provoke VMOutOfMemoryException during object >> reallocation: >> >> EAPopFrameNotInlinedReallocFailure >> EAPopInlinedMethodWithScalarReplacedObjectsReallocFailure >> EAForceEarlyReturnOfInlinedMethodWithScalarReplacedObjectsReallocFailure >> >> This is not 100% reliable therefore it should not be treated as test failure >> if no oom was raised. > > Richard Reingruber has updated the pull request with a new target base due to > a merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains four additional > commits since the last revision: > > - Skip test cases expecting OOMEs if running with ZGC. > - Merge branch 'master' into JDK-8255072-eatests-oom-not-thrown > - Make OOME more reliable and skip test cases if it is not expected because > scalar replacement is disabled > - 8255072: [TESTBUG] com/sun/jdi/EATests.java should not fail if expected > VMOutOfMemoryException is not thrown Looks good. - Marked as reviewed by cjplummer (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/775
Re: RFR: 8255072: [TESTBUG] com/sun/jdi/EATests.java should not fail if expected VMOutOfMemoryException is not thrown
On Thu, 22 Oct 2020 14:29:28 GMT, Chris Plummer wrote: >> You mentioned the possibility that the OOME is not thrown because it is >> another thread that consumes all memory than the one calling >> forceEarlyReturn() which is supposed to fail with OOME. TLAB could be an >> issue then. In general GC could have a heuristic in place to raise OOME in >> greedy threads when another thread would still be able to allocate >> successfully. I think I could change the debugger part to call >> consumeAllMemory() in the debuggee. This should be executed in the same jdwp >> agent thread as the later forceEarlyReturn. >> >> But honestly I don't think it is worth it and I cannot even test if it fixes >> the issues. I'd prefer to skip the 3 test cases if running with ZGC. Please >> let me know what you prefer. > >> But honestly I don't think it is worth it and I cannot even test if it fixes >> the issues. I'd prefer to skip the 3 test cases if running with ZGC. Please >> let me know what you prefer. > > That's one option if this only happens with ZGC. You also mentioned doing > retries. I would only suggest that if you think you can limit the chances of > the OOME not happening to be so unlikely that we are no likely to ever see it > happen. With the last commit the combined change is - The 3 problematic test cases are skipped if ZGC is selected. - They are also skipped if no OOME during object reallocation can be expected because allocations are not eliminated. - In consumeAllMemory, as a last step, empty LinkedList nodes are created without long array to fill up small blocks of free memory. - EATests.java is removed from the problem list for ZGC. - PR: https://git.openjdk.java.net/jdk/pull/775
Re: RFR: 8255072: [TESTBUG] com/sun/jdi/EATests.java should not fail if expected VMOutOfMemoryException is not thrown [v3]
> The following test cases try to provoke VMOutOfMemoryException during object > reallocation: > > EAPopFrameNotInlinedReallocFailure > EAPopInlinedMethodWithScalarReplacedObjectsReallocFailure > EAForceEarlyReturnOfInlinedMethodWithScalarReplacedObjectsReallocFailure > > This is not 100% reliable therefore it should not be treated as test failure > if no oom was raised. Richard Reingruber has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision: - Skip test cases expecting OOMEs if running with ZGC. - Merge branch 'master' into JDK-8255072-eatests-oom-not-thrown - Make OOME more reliable and skip test cases if it is not expected because scalar replacement is disabled - 8255072: [TESTBUG] com/sun/jdi/EATests.java should not fail if expected VMOutOfMemoryException is not thrown - Changes: - all: https://git.openjdk.java.net/jdk/pull/775/files - new: https://git.openjdk.java.net/jdk/pull/775/files/4196a421..33ceb741 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=775=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=775=01-02 Stats: 28553 lines in 409 files changed: 23836 ins; 3073 del; 1644 mod Patch: https://git.openjdk.java.net/jdk/pull/775.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/775/head:pull/775 PR: https://git.openjdk.java.net/jdk/pull/775
Integrated: JDK-8255262: Remove use of legacy custom @spec tag
On Thu, 22 Oct 2020 17:16:23 GMT, Jonathan Gibbons wrote: > The change is (just) to remove legacy usages of a JDK-private custom tag. This pull request has now been integrated. Changeset: 0aa3c925 Author:Jonathan Gibbons URL: https://git.openjdk.java.net/jdk/commit/0aa3c925 Stats: 209 lines in 69 files changed: 0 ins; 209 del; 0 mod 8255262: Remove use of legacy custom @spec tag Reviewed-by: lancea, mr, iris, alanb, darcy, mchung - PR: https://git.openjdk.java.net/jdk/pull/814
Re: RFR: JDK-8255262: Remove use of legacy custom @spec tag
On Thu, 22 Oct 2020 17:16:23 GMT, Jonathan Gibbons wrote: > The change is (just) to remove legacy usages of a JDK-private custom tag. Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/814
Re: RFR: JDK-8255262: Remove use of legacy custom @spec tag
On Thu, 22 Oct 2020 17:16:23 GMT, Jonathan Gibbons wrote: > The change is (just) to remove legacy usages of a JDK-private custom tag. Marked as reviewed by darcy (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/814
Re: RFR: JDK-8255262: Remove use of legacy custom @spec tag
On Thu, 22 Oct 2020 17:16:23 GMT, Jonathan Gibbons wrote: > The change is (just) to remove legacy usages of a JDK-private custom tag. As the creator of these tags many moons ago, I approve this change. - Marked as reviewed by mr (Lead). PR: https://git.openjdk.java.net/jdk/pull/814
Re: RFR: JDK-8255262: Remove use of legacy custom @spec tag
On Thu, 22 Oct 2020 17:16:23 GMT, Jonathan Gibbons wrote: > The change is (just) to remove legacy usages of a JDK-private custom tag. Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/814
Re: RFR: JDK-8255262: Remove use of legacy custom @spec tag
On Thu, 22 Oct 2020 17:16:23 GMT, Jonathan Gibbons wrote: > The change is (just) to remove legacy usages of a JDK-private custom tag. Nice clean-up. - Marked as reviewed by iris (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/814
Re: RFR: JDK-8255262: Remove use of legacy custom @spec tag
On Thu, 22 Oct 2020 17:16:23 GMT, Jonathan Gibbons wrote: > The change is (just) to remove legacy usages of a JDK-private custom tag. looks fine - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/814
RFR: JDK-8255262: Remove use of legacy custom @spec tag
The change is (just) to remove legacy usages of a JDK-private custom tag. - Commit messages: - JDK-8255262: Remove use of legacy custom @spec tag Changes: https://git.openjdk.java.net/jdk/pull/814/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=814=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255262 Stats: 209 lines in 69 files changed: 0 ins; 209 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/814.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/814/head:pull/814 PR: https://git.openjdk.java.net/jdk/pull/814
Re: RFR: 8254861: reformat code in vmTestbase/nsk/jdb [v2]
On Thu, 22 Oct 2020 08:48:20 GMT, Serguei Spitsyn wrote:
>> File test/hotspot/jtreg/vmTestbase/nsk/jdb/kill/kill001/kill001.java has
>> this change:
>>
>> for (int i = 0; i < threads.length; i++) {
>> reply = jdb.receiveReplyForWithMessageWait(JdbCommand.kill +
>> threads[i] + " " +
>> - DEBUGGEE_EXCEPTIONS
>> + "[" + i + "]",
>> - "killed");
>> +DEBUGGEE_EXCEPTIONS + "[" + i + "]",
>> +"killed");
>> }
>> I think, the second line "killed"); has to be aligned with the previous one.
>> Also, I feels like this change makes the code to be less readable:
>> reply = jdb.receiveReplyForWithMessageWait(JdbCommand.eval +
>> DEBUGGEE_RESULT,
>> - DEBUGGEE_RESULT + " =");
>> +DEBUGGEE_RESULT + " =");
>
> Hi Igor,
>
> Overall, it is great. Your formatting tool seems to be AI.
> This update fixes a lot of formatting issues.
> I have no more comments so far.
Regarding the "killed" formatting, I think the reason for it is the 8 space
"line continuation rule". The 2nd line is a actually a line continuation of a
line continuation, so it gets indented and extra 16 spaces. The 3rd line is
just a single line continuation, so gets indented just an extra 8. I think what
would help to clean this up is to move the first argument onto a newline, which
would just be indented an extra 8. That way it won't be broken up over multiple
lines, and will also be inline with the "killed" argument.
For the 2nd case above, I also think this is less readable than the original. I
personally like it if you align all arguments with the first one, even if it
leads to a non-standard indentation. If you want subsequent arguments to use
the 8 space line continuation rule, then I suggest whenever arguments are on
multiple lines that you always place the first argument on a new line so all
arguments are aligned together.
-
PR: https://git.openjdk.java.net/jdk/pull/689
Integrated: 8223312: Utilize handshakes instead of is_thread_fully_suspended
On Mon, 19 Oct 2020 09:59:34 GMT, Robbin Ehn wrote: > The main point of this change-set is to make it easier to implement S/R on > top of handshakes. > Which is a prerequisite for removing _suspend_flag (which duplicates the > handshake functionality). > But we also remove some complicated S/R methods. > > We basically just put in everything in the handshake closure, so the diff > just looks much worse than what it is. > > TraceSuspendDebugBits have an ifdef, but in both cases it now just returns. > But I was unsure if I should remove now or when is_ext_suspend_completed() is > removed. > > Passes multiple t1-5 runs, locally it passes many > jck:vm/nsk_jvmti/nsk_jdi/jdk-jdi runs. This pull request has now been integrated. Changeset: 4634dbef Author:Robbin Ehn URL: https://git.openjdk.java.net/jdk/commit/4634dbef Stats: 611 lines in 6 files changed: 174 ins; 376 del; 61 mod 8223312: Utilize handshakes instead of is_thread_fully_suspended Reviewed-by: dholmes, rrich, dcubed, eosterlund - PR: https://git.openjdk.java.net/jdk/pull/729
Re: RFR: 8223312: Utilize handshakes instead of is_thread_fully_suspended [v3]
On Thu, 22 Oct 2020 10:44:21 GMT, Robbin Ehn wrote: >> Looks good. Awesome fix IMO. > > Passes my local testing: open/test/jdk/com/sun/jdi/EATests.java, nsk_jvmti, > nsk_jdi, jdk_jdi, jck:vm. > Still running t1-t5 in test system. > > I will be integrating later today, so the ZGC/EscapeBarrier issue can be > resolved (which is semi-dependent on this). > > Thanks @dholmes-ora, @dcubed-ojdk, @reinrich, @fisk ! T1-5 looked good. - PR: https://git.openjdk.java.net/jdk/pull/729
Re: RFR: 8255072: [TESTBUG] com/sun/jdi/EATests.java should not fail if expected VMOutOfMemoryException is not thrown
On Thu, 22 Oct 2020 13:07:32 GMT, Richard Reingruber wrote: > But honestly I don't think it is worth it and I cannot even test if it fixes > the issues. I'd prefer to skip the 3 test cases if running with ZGC. Please > let me know what you prefer. That's one option if this only happens with ZGC. You also mentioned doing retries. I would only suggest that if you think you can limit the chances of the OOME not happening to be so unlikely that we are no likely to ever see it happen. - PR: https://git.openjdk.java.net/jdk/pull/775
Re: RFR: 8255072: [TESTBUG] com/sun/jdi/EATests.java should not fail if expected VMOutOfMemoryException is not thrown
On Thu, 22 Oct 2020 07:57:32 GMT, Richard Reingruber wrote: >>> The new LinkedListOfLongArrays is created by renaming LinkedList to >>> LinkedListOfLongArrays. The new LinkedList is a list node without payload, >>> so it >>> is smaller than a LinkedListOfLongArrays node. I try to fill the last free >>> blocks with these. But yeah, this won't make that much of a difference. >> >> Ok. However, since you can't reproduce the initial problem it's hard to say >> if this will fix it. You would need to remove it from the problem list with >> this PR and see what happens. >> >> Earlier you said: >> >>> Note also that the OOME is successfully generated during object >>> reallocation a couple of times before (search "run args" in attachments >>> to the JBS issue). >> >> So I suppose in that case it's ok if this one test case allows for no OOME >> just as long as other test cases still require it. > >> Earlier you said: >> >> > Note also that the OOME is successfully generated during object >> > reallocation a couple of times before (search "run args" in attachments >> > to the JBS issue). >> >> So I suppose in that case it's ok if this one test case allows for no OOME >> just as long as other test cases still require it. > > With that I wanted to state that the probability to get the OOME is not too > bad > for meaningful testing. It should be the same for the 3 test cases. > > What about repeating a test case if the expected OOME is not raised and let it > fail after 10x retries? You mentioned the possibility that the OOME is not thrown because it is another thread that consumes all memory than the one calling forceEarlyReturn() which is supposed to fail with OOME. TLAB could be an issue then. In general GC could have a heuristic in place to raise OOME in greedy threads when another thread would still be able to allocate successfully. I think I could change the debugger part to call consumeAllMemory() in the debuggee. This should be executed in the same jdwp agent thread as the later forceEarlyReturn. But honestly I don't think it is worth it and I cannot even test if it fixes the issues. I'd prefer to skip the 3 test cases if running with ZGC. Please let me know what you prefer. - PR: https://git.openjdk.java.net/jdk/pull/775
Re: RFR: 8223312: Utilize handshakes instead of is_thread_fully_suspended [v3]
On Thu, 22 Oct 2020 10:04:48 GMT, Erik Österlund wrote: >> Robbin Ehn has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains seven commits: >> >> - Fixed merge miss >> - Merge branch 'master' into >> 8223312-Utilize-handshakes-instead-of-is_thread_fully_suspended >> - Merge fix from Richard >> - Merge branch 'master' into >> 8223312-Utilize-handshakes-instead-of-is_thread_fully_suspended >> - Removed TraceSuspendDebugBits >> - Removed unused method is_ext_suspend_completed_with_lock >> - Utilize handshakes instead of is_thread_fully_suspended > > Looks good. Awesome fix IMO. Passes my local testing: open/test/jdk/com/sun/jdi/EATests.java, nsk_jvmti, nsk_jdi, jdk_jdi, jck:vm. Still running t1-t5 in test system. I will be integrating later today, so the ZGC/EscapeBarrier issue can be resolved (which is semi-dependent on this). Thanks @dholmes-ora, @dcubed-ojdk, @reinrich, @fisk ! - PR: https://git.openjdk.java.net/jdk/pull/729
Re: RFR: 8223312: Utilize handshakes instead of is_thread_fully_suspended [v3]
On Thu, 22 Oct 2020 08:50:54 GMT, Richard Reingruber wrote: >> Depth 1 means top frame and its caller. In >> UpdateForPopTopFrameClosure::doit() line 1606(?) the 2 top frames are >> deoptimized. Reallocating objects while a frame pop request is processed >> does not work if reallocation fails therefore we use an EscapeBarrier to >> eagerly reallocate objects beforehand. > > @fisk for PopFrame the top frame needs to be deoptimized (if compiled) to be > able to actually remove it when the thread is resumed. Its caller needs to be > deoptimized to be able restart the call. For ForceEarlyReturn it is not > necessary to restart. The target can return to a compiled caller and continue > executing compiled code. So the caller frame is not deoptimized. > > @robehn nothing is messed up. Thanks again for doing it. Great! - PR: https://git.openjdk.java.net/jdk/pull/729
Re: RFR: 8223312: Utilize handshakes instead of is_thread_fully_suspended [v3]
On Thu, 22 Oct 2020 10:04:01 GMT, Erik Österlund wrote: >> @robehn you haven't messed up. Hope I havn't either. I've tested >> >> == >> Test summary >> == >>TEST TOTAL PASS FAIL ERROR >> >>jtreg:test/hotspot/jtreg:hotspot_serviceability 197 197 0 0 >> >>jtreg:test/jdk:jdk_svc 1176 1176 0 0 >> >>jtreg:test/jdk:jdk_jdi 174 174 0 0 >> >>jtreg:test/hotspot/jtreg:vmTestbase_nsk_jdi1141 1141 0 0 >> >>jtreg:test/hotspot/jtreg:vmTestbase_nsk_jvmti 648 648 0 0 >> >>jtreg:test/hotspot/jtreg:vmTestbase_nsk_jdwp113 113 0 0 >> >> == >> TEST SUCCESS >> jdk_jdi now includes jdk/com/sun/jdi/EATests.java which tests >> PopFrame/ForceEarlyReturn with object reallocation with and without >> reallocation failures. > > Ah. I see now the loop uses <= instead of <. So my reasoning was right but > off by 1. Passing in 0 really means deopt 1 frame. Which means everything is > fine and working the way I expect it to. Great - PR: https://git.openjdk.java.net/jdk/pull/729
Re: RFR: 8223312: Utilize handshakes instead of is_thread_fully_suspended [v4]
> The main point of this change-set is to make it easier to implement S/R on > top of handshakes. > Which is a prerequisite for removing _suspend_flag (which duplicates the > handshake functionality). > But we also remove some complicated S/R methods. > > We basically just put in everything in the handshake closure, so the diff > just looks much worse than what it is. > > TraceSuspendDebugBits have an ifdef, but in both cases it now just returns. > But I was unsure if I should remove now or when is_ext_suspend_completed() is > removed. > > Passes multiple t1-5 runs, locally it passes many > jck:vm/nsk_jvmti/nsk_jdi/jdk-jdi runs. Robbin Ehn has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains ten commits: - Merge - Review updates - Fixed merge miss - Merge branch 'master' into 8223312-Utilize-handshakes-instead-of-is_thread_fully_suspended - Merge fix from Richard - Merge branch 'master' into 8223312-Utilize-handshakes-instead-of-is_thread_fully_suspended - Removed TraceSuspendDebugBits - Removed unused method is_ext_suspend_completed_with_lock - Utilize handshakes instead of is_thread_fully_suspended - Changes: https://git.openjdk.java.net/jdk/pull/729/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=729=03 Stats: 611 lines in 6 files changed: 174 ins; 376 del; 61 mod Patch: https://git.openjdk.java.net/jdk/pull/729.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/729/head:pull/729 PR: https://git.openjdk.java.net/jdk/pull/729
Re: RFR: 8223312: Utilize handshakes instead of is_thread_fully_suspended [v3]
On Thu, 22 Oct 2020 08:42:40 GMT, Richard Reingruber wrote: >> Stack frames are counted beginning at 0. The top frame has depth 0. So >> object deoptimization happens in the top frame. >> >> Still the used method is not optimal because it assumes that objects of >> frames within the given depth are accessed and their escape state is >> changed. But potentially caller methods optimized on the escape state >> therefore it searches for caller frames passing ArgEscape objects and >> deoptimizes these too. With ForceEarlyReturn no objects are accessed but it >> is so uncommon that I did not bother optimizing this. Should I? > > @robehn you haven't messed up. Hope I havn't either. I've tested > > == > Test summary > == >TEST TOTAL PASS FAIL ERROR > >jtreg:test/hotspot/jtreg:hotspot_serviceability 197 197 0 0 > >jtreg:test/jdk:jdk_svc 1176 1176 0 0 > >jtreg:test/jdk:jdk_jdi 174 174 0 0 > >jtreg:test/hotspot/jtreg:vmTestbase_nsk_jdi1141 1141 0 0 > >jtreg:test/hotspot/jtreg:vmTestbase_nsk_jvmti 648 648 0 0 > >jtreg:test/hotspot/jtreg:vmTestbase_nsk_jdwp113 113 0 0 > > == > TEST SUCCESS > jdk_jdi now includes jdk/com/sun/jdi/EATests.java which tests > PopFrame/ForceEarlyReturn with object reallocation with and without > reallocation failures. Ah. I see now the loop uses <= instead of <. So my reasoning was right but off by 1. Passing in 0 really means deopt 1 frame. Which means everything is fine and working the way I expect it to. - PR: https://git.openjdk.java.net/jdk/pull/729
Re: RFR: 8223312: Utilize handshakes instead of is_thread_fully_suspended [v3]
On Wed, 21 Oct 2020 08:40:47 GMT, Robbin Ehn wrote: >> The main point of this change-set is to make it easier to implement S/R on >> top of handshakes. >> Which is a prerequisite for removing _suspend_flag (which duplicates the >> handshake functionality). >> But we also remove some complicated S/R methods. >> >> We basically just put in everything in the handshake closure, so the diff >> just looks much worse than what it is. >> >> TraceSuspendDebugBits have an ifdef, but in both cases it now just returns. >> But I was unsure if I should remove now or when is_ext_suspend_completed() >> is removed. >> >> Passes multiple t1-5 runs, locally it passes many >> jck:vm/nsk_jvmti/nsk_jdi/jdk-jdi runs. > > Robbin Ehn has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains seven commits: > > - Fixed merge miss > - Merge branch 'master' into > 8223312-Utilize-handshakes-instead-of-is_thread_fully_suspended > - Merge fix from Richard > - Merge branch 'master' into > 8223312-Utilize-handshakes-instead-of-is_thread_fully_suspended > - Removed TraceSuspendDebugBits > - Removed unused method is_ext_suspend_completed_with_lock > - Utilize handshakes instead of is_thread_fully_suspended Looks good. Awesome fix IMO. - Marked as reviewed by eosterlund (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/729
Re: RFR: 8252103: Parallel heap inspection for ParallelScavengeHeap [v4]
On Mon, 19 Oct 2020 13:09:34 GMT, Lin Zang wrote: >> - Parallel heap iteration support for PSS >> - JBS: https://bugs.openjdk.java.net/browse/JDK-8252103 > > Lin Zang has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. Took a second look today and found two additional problems. I would also be interested in what kind of testing you've done with this patch. src/hotspot/share/gc/parallel/psOldGen.cpp line 199: > 197: > 198: // iterate objects in block. > 199: HeapWord* end = MIN2(top, begin + _iterate_block_size); `_iterate_block_size` is a size in bytes (or at least you use it like a size in bytes when calculating the number of blocks), so before you can add it you need to convert it to a size in words like this: Suggestion: size_t block_word_size = _iterate_block_size / HeapWordSize; HeapWord* begin = bottom + block_index * block_word_size; assert((block_word_size % (ObjectStartArray::block_size)) == 0, "BLOCK SIZE not a multiple of start_array block"); // iterate objects in block. HeapWord* end = MIN2(top, begin + block_word_size); - Changes requested by sjohanss (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/25
Re: RFR: 8252103: Parallel heap inspection for ParallelScavengeHeap [v4]
On Wed, 21 Oct 2020 19:49:03 GMT, Stefan Johansson wrote: >> Lin Zang has refreshed the contents of this pull request, and previous >> commits have been removed. The incremental views will show differences >> compared to the previous content of the PR. > > src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp line 565: > >> 563: size_t old_gen_used = >> ParallelScavengeHeap::heap()->old_gen()->used_in_bytes(); >> 564: size_t block_size = >> ParallelScavengeHeap::heap()->old_gen()->iterate_block_size(); >> 565: uint n_blocks_in_old = old_gen_used / block_size + 1; > > Instead of doing this calculation here, what do you think about making > `iterate_block_size()` a constant in `PSOldGen` and instead adding a function > that returns the number of blocks available, something like: > `iterable_blocks()` One additional thing here, the `n_blocks_in_old` calculation will return one extra block when old_gen_used is a multiple of block_size. : Here's an alternative: uint n_blocks_in_old = (old_gen_used + block_size - 1) / block_size; - PR: https://git.openjdk.java.net/jdk/pull/25
Re: RFR: 8223312: Utilize handshakes instead of is_thread_fully_suspended [v3]
On Thu, 22 Oct 2020 08:23:38 GMT, Richard Reingruber wrote: >> @reinrich did I mess something up when merging this in? > > Depth 1 means top frame and its caller. In > UpdateForPopTopFrameClosure::doit() line 1606(?) the 2 top frames are > deoptimized. Reallocating objects while a frame pop request is processed does > not work if reallocation fails therefore we use an EscapeBarrier to eagerly > reallocate objects beforehand. @fisk for PopFrame the top frame needs to be deoptimized (if compiled) to be able to actually remove it when the thread is resumed. Its caller needs to be deoptimized to be able restart the call. For ForceEarlyReturn it is not necessary to restart. The target can return to a compiled caller and continue executing compiled code. So the caller frame is not deoptimized. @robehn nothing is messed up. Thanks again for doing it. - PR: https://git.openjdk.java.net/jdk/pull/729
Re: RFR: 8254861: reformat code in vmTestbase/nsk/jdb [v2]
On Thu, 22 Oct 2020 08:03:15 GMT, Serguei Spitsyn wrote:
>> due to the same reasons in the case w/ `fields001`, these lines have 3 unit
>> indentation, 1st for `hc001` class, 2nd for `testInvalidCommands` method,
>> 3rd for `invClassNames` array initialization, so they have 3x4 = 12 spaces.
>
> File test/hotspot/jtreg/vmTestbase/nsk/jdb/kill/kill001/kill001.java has this
> change:
>
> for (int i = 0; i < threads.length; i++) {
> reply = jdb.receiveReplyForWithMessageWait(JdbCommand.kill +
> threads[i] + " " +
> - DEBUGGEE_EXCEPTIONS +
> "[" + i + "]",
> - "killed");
> +DEBUGGEE_EXCEPTIONS + "[" + i + "]",
> +"killed");
> }
> I think, the second line "killed"); has to be aligned with the previous one.
> Also, I feels like this change makes the code to be less readable:
> reply = jdb.receiveReplyForWithMessageWait(JdbCommand.eval +
> DEBUGGEE_RESULT,
> - DEBUGGEE_RESULT + " =");
> +DEBUGGEE_RESULT + " =");
Hi Igor,
Overall, it is great. Your formatting tool seems to be AI.
This update fixes a lot of formatting issues.
I have no more comments so far.
-
PR: https://git.openjdk.java.net/jdk/pull/689
Re: RFR: 8223312: Utilize handshakes instead of is_thread_fully_suspended [v3]
On Thu, 22 Oct 2020 08:14:47 GMT, Richard Reingruber wrote: >> @reinrich did I mess something up when merging this in? > > Stack frames are counted beginning at 0. The top frame has depth 0. So object > deoptimization happens in the top frame. > > Still the used method is not optimal because it assumes that objects of > frames within the given depth are accessed and their escape state is changed. > But potentially caller methods optimized on the escape state therefore it > searches for caller frames passing ArgEscape objects and deoptimizes these > too. With ForceEarlyReturn no objects are accessed but it is so uncommon that > I did not bother optimizing this. Should I? @robehn you haven't messed up. Hope I havn't either. I've tested == Test summary == TEST TOTAL PASS FAIL ERROR jtreg:test/hotspot/jtreg:hotspot_serviceability 197 197 0 0 jtreg:test/jdk:jdk_svc 1176 1176 0 0 jtreg:test/jdk:jdk_jdi 174 174 0 0 jtreg:test/hotspot/jtreg:vmTestbase_nsk_jdi1141 1141 0 0 jtreg:test/hotspot/jtreg:vmTestbase_nsk_jvmti 648 648 0 0 jtreg:test/hotspot/jtreg:vmTestbase_nsk_jdwp113 113 0 0 == TEST SUCCESS jdk_jdi now includes jdk/com/sun/jdi/EATests.java which tests PopFrame/ForceEarlyReturn with object reallocation with and without reallocation failures. - PR: https://git.openjdk.java.net/jdk/pull/729
Re: RFR: 8223312: Utilize handshakes instead of is_thread_fully_suspended [v3]
On Thu, 22 Oct 2020 08:04:48 GMT, Robbin Ehn wrote:
>> src/hotspot/share/prims/jvmtiEnv.cpp line 1663:
>>
>>> 1661: return JVMTI_ERROR_OUT_OF_MEMORY;
>>> 1662: }
>>> 1663: if (!eb.deoptimize_objects(1)) {
>>
>> Oh and why is the depth 1 here, when two frames are deoptimized? Maybe I
>> missed something.
>
> @reinrich did I mess something up when merging this in?
Depth 1 means top frame and its caller. In UpdateForPopTopFrameClosure::doit()
line 1606(?) the 2 top frames are deoptimized. Reallocating objects while a
frame pop request is processed does not work if reallocation fails therefore we
use an EscapeBarrier to eagerly reallocate objects beforehand.
-
PR: https://git.openjdk.java.net/jdk/pull/729
Re: RFR: 8223312: Utilize handshakes instead of is_thread_fully_suspended [v3]
On Thu, 22 Oct 2020 08:04:44 GMT, Robbin Ehn wrote:
>> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1390:
>>
>>> 1388: return JVMTI_ERROR_OUT_OF_MEMORY;
>>> 1389: }
>>> 1390: if (!eb.deoptimize_objects(0)) {
>>
>> Why is the depth 0 here? That makes no sense to me. My understanding of this
>> is that we have extracted the object deopt that would "normally" (since last
>> week?) be done in JvmtiEnvBase::check_top_frame. And it is walking 1 frame,
>> so shouldn't the depth be 1?
>
> @reinrich did I mess something up when merging this in?
Stack frames are counted beginning at 0. The top frame has depth 0. So object
deoptimization happens in the top frame.
Still the used method is not optimal because it assumes that objects of frames
within the given depth are accessed and their escape state is changed. But
potentially caller methods optimized on the escape state therefore it searches
for caller frames passing ArgEscape objects and deoptimizes these too. With
ForceEarlyReturn no objects are accessed but it is so uncommon that I did not
bother optimizing this. Should I?
-
PR: https://git.openjdk.java.net/jdk/pull/729
Re: RFR: 8223312: Utilize handshakes instead of is_thread_fully_suspended [v3]
On Wed, 21 Oct 2020 14:20:21 GMT, Richard Reingruber wrote:
>> src/hotspot/share/runtime/deoptimization.cpp line 1771:
>>
>>> 1769: Deoptimization::deoptimize_frame_internal(thread, id, reason);
>>> 1770: } else {
>>> 1771: VM_DeoptimizeFrame deopt(thread, id, reason);
>>
>> I guess VM_DeoptimizeFrame can be replaced with a handshake too now.
>
> Not in this pr of course :)
I think so.
-
PR: https://git.openjdk.java.net/jdk/pull/729
Re: RFR: 8223312: Utilize handshakes instead of is_thread_fully_suspended [v3]
On Thu, 22 Oct 2020 06:50:37 GMT, Richard Reingruber wrote:
>> Agreed. @sspitsyn - This makes me wonder if the lack of
>> synchronization is the cause of some instability in the
>> JVM/TI ForceEarlyReturn() testing.
>>
>> Update: The old code only made the updates if the thread was fully
>> suspended so you won't have a race between the requesting thread
>> and the target thread in that case.
>
> Yes, I meant synchronization between racing agent threads. Surely a corner
> case.
Since we do not hold Threads_lock nor SR_lock nothing is stopping the resume at
this point AFAICT.
Now this might be illegal, but it can happen if you are not really careful,
specially in test like Kitchensink where two modules might use S/R on the same
thread.
Also if two threads calling this, the second thread might have passed:
if (_state->is_earlyret_pending()) {
When we do the setting:
_state->set_earlyret_pending();
But now it's protected, even if this never manifested as bug, now we sure it
will not.
-
PR: https://git.openjdk.java.net/jdk/pull/729
Re: RFR: 8223312: Utilize handshakes instead of is_thread_fully_suspended [v3]
On Wed, 21 Oct 2020 22:57:59 GMT, David Holmes wrote:
>> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1661:
>>
>>> 1659: assert(vf->frame_pointer() != NULL, "frame pointer mustn't be
>>> NULL");
>>> 1660: if (java_thread->is_exiting() || java_thread->threadObj() == NULL) {
>>> 1661: return;
>>
>> What's the `_result` value if this `return` executes?
>
> The default "not alive" value.
Added comment.
-
PR: https://git.openjdk.java.net/jdk/pull/729
Re: RFR: 8223312: Utilize handshakes instead of is_thread_fully_suspended [v3]
On Wed, 21 Oct 2020 20:31:27 GMT, Erik Österlund wrote:
>> Robbin Ehn has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains seven commits:
>>
>> - Fixed merge miss
>> - Merge branch 'master' into
>> 8223312-Utilize-handshakes-instead-of-is_thread_fully_suspended
>> - Merge fix from Richard
>> - Merge branch 'master' into
>> 8223312-Utilize-handshakes-instead-of-is_thread_fully_suspended
>> - Removed TraceSuspendDebugBits
>> - Removed unused method is_ext_suspend_completed_with_lock
>> - Utilize handshakes instead of is_thread_fully_suspended
>
> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1390:
>
>> 1388: return JVMTI_ERROR_OUT_OF_MEMORY;
>> 1389: }
>> 1390: if (!eb.deoptimize_objects(0)) {
>
> Why is the depth 0 here? That makes no sense to me. My understanding of this
> is that we have extracted the object deopt that would "normally" (since last
> week?) be done in JvmtiEnvBase::check_top_frame. And it is walking 1 frame,
> so shouldn't the depth be 1?
@reinrich did I mess something up when merging this in?
> src/hotspot/share/prims/jvmtiEnv.cpp line 1663:
>
>> 1661: return JVMTI_ERROR_OUT_OF_MEMORY;
>> 1662: }
>> 1663: if (!eb.deoptimize_objects(1)) {
>
> Oh and why is the depth 1 here, when two frames are deoptimized? Maybe I
> missed something.
@reinrich did I mess something up when merging this in?
-
PR: https://git.openjdk.java.net/jdk/pull/729
Re: RFR: 8223312: Utilize handshakes instead of is_thread_fully_suspended [v3]
On Wed, 21 Oct 2020 16:54:48 GMT, Daniel D. Daugherty
wrote:
>> Robbin Ehn has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains seven commits:
>>
>> - Fixed merge miss
>> - Merge branch 'master' into
>> 8223312-Utilize-handshakes-instead-of-is_thread_fully_suspended
>> - Merge fix from Richard
>> - Merge branch 'master' into
>> 8223312-Utilize-handshakes-instead-of-is_thread_fully_suspended
>> - Removed TraceSuspendDebugBits
>> - Removed unused method is_ext_suspend_completed_with_lock
>> - Utilize handshakes instead of is_thread_fully_suspended
>
> src/hotspot/share/prims/jvmtiEnv.cpp line 1718:
>
>> 1716: MutexLocker mu(JvmtiThreadState_lock);
>> 1717: if (java_thread == JavaThread::current()) {
>> 1718: op.doit(java_thread, true);
>
> Please add a comment after the `true` parameter to
> indicate the name of the doit() function's parameter,
> e.g., `true /* self */`.
Fixed
> src/hotspot/share/prims/jvmtiEnvBase.cpp line 56:
>
>> 54: #include "runtime/threadSMR.hpp"
>> 55: #include "runtime/vframe.hpp"
>> 56: #include "runtime/vframe.inline.hpp"
>
> When you add `foo.inline.hpp` you delete `foo.hpp` because
> the `foo.inline.hpp` file always includes the `foo.hpp` file.
Fixed
> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1311:
>
>> 1309: // It is to keep a ret_ob_h handle alive after return to the caller.
>> 1310: jvmtiError
>> 1311: JvmtiEnvBase::check_top_frame(Thread* current_thread, JavaThread*
>> java_thread,
>
> Again, it is not clear why these changes to `check_top_frame` are here since
> they
> appear to be related to @reinrich's work.
Long story:
Before async handshakes was integrate I had a patch which does the same as this.
This change was accidentally slipped into async handshakes change-set and was
integrated.
Richard notice this, I told him he could revert it in his change-set for EB, so
he did.
But now we need this change, so here it comes once more!
VM thread is allowed to execute these handshakes, thus when calling
check_top_frame() from SetForceEarlyReturn::doit() it's just a Thread*.
> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1398:
>
>> 1396: SetForceEarlyReturn op(state, value, tos);
>> 1397: if (java_thread == current_thread) {
>> 1398: op.doit(java_thread, true);
>
> Please add a comment after the true parameter to
> indicate the name of the doit() function's parameter,
> e.g., `true /* self */`.
Fixed
> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1543:
>
>> 1541: HandleMark hm(current_thread);
>> 1542: JavaThread* java_thread = target->as_Java_thread();
>> 1543:
>
> This would be useful here:
>
> `assert(_state->get_thread() == java_thread, "Must be");`
Fixed.
> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1570:
>
>> 1568:
>> 1569: ResourceMark rm(current_thread);
>> 1570: // Check if there are more than one Java frame in this thread, that
>> the top two frames
>
> typo: s/are more/is more/
Fixed
> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1642:
>
>> 1640:
>> 1641: if (!self) {
>> 1642: if (!java_thread->is_external_suspend()) {
>
> You could join these two if-statements with `&&` and have
> one less indenting level...
Fixed
> src/hotspot/share/prims/jvmtiEnvBase.hpp line 361:
>
>> 359: _tos(tos) {}
>> 360: void do_thread(Thread *target) {
>> 361: doit(target, false);
>
> Please add a comment after the true parameter to
> indicate the name of the doit() function's parameter,
> e.g., `false /* self */`.
Fixed
> src/hotspot/share/prims/jvmtiEnvBase.hpp line 395:
>
>> 393: _depth(depth) {}
>> 394: void do_thread(Thread *target) {
>> 395: doit(target, false);
>
> Please add a comment after the true parameter to
> indicate the name of the doit() function's parameter,
> e.g., `false /* self */`.
Fixed
> src/hotspot/share/runtime/deoptimization.cpp line 1755:
>
>> 1753: thread->is_handshake_safe_for(Thread::current()) ||
>> 1754: SafepointSynchronize::is_at_safepoint(),
>> 1755: "can only deoptimize other thread at a safepoint");
>
> Should that now be: `safepoint/handshake`??
Fixed
> src/hotspot/share/runtime/thread.cpp line 698:
>
>> 696: RememberProcessedThread rpt(this);
>> 697: oops_do_no_frames(f, cf);
>> 698: oops_do_frames(f, cf);
>
> In the comment above:
> // ... If we were
> // called by wait_for_ext_suspend_completion(), then it
> // will be doing the retries so we don't have to.
> `wait_for_ext_suspend_completion()` has been deleted so the
> comment needs work.
I just delete the no longer relevant parts.
-
PR: https://git.openjdk.java.net/jdk/pull/729
Re: RFR: 8223312: Utilize handshakes instead of is_thread_fully_suspended [v3]
On Wed, 21 Oct 2020 16:47:39 GMT, Daniel D. Daugherty
wrote:
>> src/hotspot/share/prims/jvmtiEnv.cpp line 1808:
>>
>>> 1806: }
>>> 1807: if (java_lang_Class::is_primitive(k_mirror)) {
>>> 1808: return JVMTI_ERROR_NONE;
>>
>> The call of JvmtiSuspendControl::print() seems to be eliminated. Ok for me.
>
> It's not clear to me why the `JvmtiSuspendControl::print()` is being
> eliminated. Please explain. The `TraceJVMTICalls` support is so that
> someone can diagnose what JVM/TI calls are being made, including
> context in some cases, so it seems wrong to delete this call.
TraceJVMTICalls is a define local to the file jvmtiEnv.cpp set to false which
added some more logging for three of the JVM TI functions.
You must first read the code to see if TraceJVMTICalls affects the functions
you have an issue with and then change it to true if your lucky it's on of
those three. And then you need to recompile. Before commit you need to set to
false again.
Why not just temporary add the JvmtiSuspendControl::print()/relevant logging
instead ?
Which you still need to do if it's not one of those three functions.
Since this code is not in jvmtiEnv.cpp, we also would need to move
TraceJVMTICalls to global scope in some header.
Turning TraceJVMTICalls into UL is good idea I guess, but not in scope of this
:)
>> src/hotspot/share/runtime/thread.cpp line 537:
>>
>>> 535: // cancelled). Returns true if the thread is externally suspended and
>>> 536: // false otherwise.
>>> 537: bool JavaThread::is_ext_suspend_completed() {
>>
>> I'd think `JavaThread::is_ext_suspend_completed` can be removed also (as a
>> separate enhancement). It also duplicates code of the handshake mechanism.
>> Just replace VM_ThreadSuspend with a handshake.
>
> `is_ext_suspend_completed()` includes code that detects that a thread
> that is in `_thread_in_native_trans` and does not yet have a walkable
> stack has not completed suspension and we will do some retries in
> this function until the target thread gets stable. We have to make sure
> that the handshake mechanism has a similar stability guarantee or a
> stack walker may fail intermittently.
Handshake can only be executed at safepoint poll site, which means if the stack
is walkable in all safepoints it is also true for handshakes. And we would be
in so much trouble if it were not walkable in all safepoints :)
-
PR: https://git.openjdk.java.net/jdk/pull/729
Re: RFR: 8223312: Utilize handshakes instead of is_thread_fully_suspended [v3]
On Wed, 21 Oct 2020 14:02:59 GMT, Richard Reingruber wrote: >> Robbin Ehn has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains seven commits: >> >> - Fixed merge miss >> - Merge branch 'master' into >> 8223312-Utilize-handshakes-instead-of-is_thread_fully_suspended >> - Merge fix from Richard >> - Merge branch 'master' into >> 8223312-Utilize-handshakes-instead-of-is_thread_fully_suspended >> - Removed TraceSuspendDebugBits >> - Removed unused method is_ext_suspend_completed_with_lock >> - Utilize handshakes instead of is_thread_fully_suspended > > src/hotspot/share/prims/jvmtiEnvBase.hpp line 310: > >> 308: >> GrowableArray *owned_monitors_list); >> 309: static jvmtiError check_top_frame(Thread* current_thread, JavaThread* >> java_thread, >> 310: jvalue value, TosState tos, Handle* >> ret_ob_h); > > Maybe fix indentation? Fixed - PR: https://git.openjdk.java.net/jdk/pull/729
Re: RFR: 8223312: Utilize handshakes instead of is_thread_fully_suspended [v3]
On Wed, 21 Oct 2020 22:57:26 GMT, David Holmes wrote: >> Especially if an assert() is added above on L1543. > > Agreed - this code has become confused about what thread variables are > present and their relationship. Fixed and moved assert. - PR: https://git.openjdk.java.net/jdk/pull/729
Re: RFR: 8254861: reformat code in vmTestbase/nsk/jdb [v2]
On Mon, 19 Oct 2020 20:57:03 GMT, Igor Ignatyev wrote:
>> test/hotspot/jtreg/vmTestbase/nsk/jdb/hidden_class/hc001/hc001.java line 323:
>>
>>> 321: "xx.yyy/0x111/0x222",
>>> 322: "xx./0x111.0x222",
>>> 323: "xx.yyy.zzz/"
>>
>> Why are these indented 8 instead of 4?
>
> due to the same reasons in the case w/ `fields001`, these lines have 3 unit
> indentation, 1st for `hc001` class, 2nd for `testInvalidCommands` method, 3rd
> for `invClassNames` array initialization, so they have 3x4 = 12 spaces.
File test/hotspot/jtreg/vmTestbase/nsk/jdb/kill/kill001/kill001.java has this
change:
for (int i = 0; i < threads.length; i++) {
reply = jdb.receiveReplyForWithMessageWait(JdbCommand.kill +
threads[i] + " " +
- DEBUGGEE_EXCEPTIONS +
"[" + i + "]",
- "killed");
+DEBUGGEE_EXCEPTIONS + "[" + i + "]",
+"killed");
}
I think, the second line "killed"); has to be aligned with the previous one.
-
PR: https://git.openjdk.java.net/jdk/pull/689
Re: RFR: 8255072: [TESTBUG] com/sun/jdi/EATests.java should not fail if expected VMOutOfMemoryException is not thrown
On Thu, 22 Oct 2020 05:06:43 GMT, Chris Plummer wrote: > Earlier you said: > > > Note also that the OOME is successfully generated during object > > reallocation a couple of times before (search "run args" in attachments > > to the JBS issue). > > So I suppose in that case it's ok if this one test case allows for no OOME > just as long as other test cases still require it. With that I wanted to state that the probability to get the OOME is not too bad for meaningful testing. It should be the same for the 3 test cases. What about repeating a test case if the expected OOME is not raised and let it fail after 10x retries? - PR: https://git.openjdk.java.net/jdk/pull/775
Re: RFR: 8223312: Utilize handshakes instead of is_thread_fully_suspended [v3]
On Wed, 21 Oct 2020 08:40:47 GMT, Robbin Ehn wrote:
>> The main point of this change-set is to make it easier to implement S/R on
>> top of handshakes.
>> Which is a prerequisite for removing _suspend_flag (which duplicates the
>> handshake functionality).
>> But we also remove some complicated S/R methods.
>>
>> We basically just put in everything in the handshake closure, so the diff
>> just looks much worse than what it is.
>>
>> TraceSuspendDebugBits have an ifdef, but in both cases it now just returns.
>> But I was unsure if I should remove now or when is_ext_suspend_completed()
>> is removed.
>>
>> Passes multiple t1-5 runs, locally it passes many
>> jck:vm/nsk_jvmti/nsk_jdi/jdk-jdi runs.
>
> Robbin Ehn has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains seven commits:
>
> - Fixed merge miss
> - Merge branch 'master' into
> 8223312-Utilize-handshakes-instead-of-is_thread_fully_suspended
> - Merge fix from Richard
> - Merge branch 'master' into
> 8223312-Utilize-handshakes-instead-of-is_thread_fully_suspended
> - Removed TraceSuspendDebugBits
> - Removed unused method is_ext_suspend_completed_with_lock
> - Utilize handshakes instead of is_thread_fully_suspended
src/hotspot/share/prims/jvmtiEnv.cpp line 1663:
> 1661: return JVMTI_ERROR_OUT_OF_MEMORY;
> 1662: }
> 1663: if (!eb.deoptimize_objects(1)) {
Oh and why is the depth 1 here, when two frames are deoptimized? Maybe I missed
something.
-
PR: https://git.openjdk.java.net/jdk/pull/729
Re: RFR: 8223312: Utilize handshakes instead of is_thread_fully_suspended [v3]
On Wed, 21 Oct 2020 08:40:47 GMT, Robbin Ehn wrote:
>> The main point of this change-set is to make it easier to implement S/R on
>> top of handshakes.
>> Which is a prerequisite for removing _suspend_flag (which duplicates the
>> handshake functionality).
>> But we also remove some complicated S/R methods.
>>
>> We basically just put in everything in the handshake closure, so the diff
>> just looks much worse than what it is.
>>
>> TraceSuspendDebugBits have an ifdef, but in both cases it now just returns.
>> But I was unsure if I should remove now or when is_ext_suspend_completed()
>> is removed.
>>
>> Passes multiple t1-5 runs, locally it passes many
>> jck:vm/nsk_jvmti/nsk_jdi/jdk-jdi runs.
>
> Robbin Ehn has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains seven commits:
>
> - Fixed merge miss
> - Merge branch 'master' into
> 8223312-Utilize-handshakes-instead-of-is_thread_fully_suspended
> - Merge fix from Richard
> - Merge branch 'master' into
> 8223312-Utilize-handshakes-instead-of-is_thread_fully_suspended
> - Removed TraceSuspendDebugBits
> - Removed unused method is_ext_suspend_completed_with_lock
> - Utilize handshakes instead of is_thread_fully_suspended
Just wondering why the escape barrier for force early return uses a stack depth
is 0. Either that is wrong, or the escape barrier is not needed in the first
place here. I think.
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1390:
> 1388: return JVMTI_ERROR_OUT_OF_MEMORY;
> 1389: }
> 1390: if (!eb.deoptimize_objects(0)) {
Why is the depth 0 here? That makes no sense to me. My understanding of this is
that we have extracted the object deopt that would "normally" (since last
week?) be done in JvmtiEnvBase::check_top_frame. And it is walking 1 frame, so
shouldn't the depth be 1?
-
PR: https://git.openjdk.java.net/jdk/pull/729
Re: RFR: 8223312: Utilize handshakes instead of is_thread_fully_suspended [v3]
On Wed, 21 Oct 2020 17:03:45 GMT, Daniel D. Daugherty wrote: >> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1454: >> >>> 1452: _state->set_earlyret_pending(); >>> 1453: _state->set_earlyret_oop(ret_ob_h()); >>> 1454: _state->set_earlyret_value(_value, _tos); >> >> Good that these updates are done with a handshake now. Maybe I'm missing >> s.th. but I don't see synchronization in the older version. > > Agreed. @sspitsyn - This makes me wonder if the lack of > synchronization is the cause of some instability in the > JVM/TI ForceEarlyReturn() testing. > > Update: The old code only made the updates if the thread was fully > suspended so you won't have a race between the requesting thread > and the target thread in that case. Yes, I meant synchronization between racing agent threads. Surely a corner case. - PR: https://git.openjdk.java.net/jdk/pull/729
Re: RFR: 8223312: Utilize handshakes instead of is_thread_fully_suspended [v3]
On Wed, 21 Oct 2020 16:45:53 GMT, Daniel D. Daugherty
wrote:
>> Robbin Ehn has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains seven commits:
>>
>> - Fixed merge miss
>> - Merge branch 'master' into
>> 8223312-Utilize-handshakes-instead-of-is_thread_fully_suspended
>> - Merge fix from Richard
>> - Merge branch 'master' into
>> 8223312-Utilize-handshakes-instead-of-is_thread_fully_suspended
>> - Removed TraceSuspendDebugBits
>> - Removed unused method is_ext_suspend_completed_with_lock
>> - Utilize handshakes instead of is_thread_fully_suspended
>
> src/hotspot/share/prims/jvmtiEnv.cpp line 1646:
>
>> 1644: // java_thread - pre-checked
>> 1645: jvmtiError
>> 1646: JvmtiEnv::PopFrame(JavaThread* java_thread) {
>
> So I'm a bit confused why I'm seeing PopFrame() changes here that are
> related to @reinrich's EscapeBarrier work. I've seen mention of picking
> up a patch during this review from @reinrich so may that's why. I don't
> see anything wrong with the changes, but I am confused why they are
> here in this review.
This change moves code with EscapeBarriers (integrated with #119) into a
handshake. That does not work because object reallocation can safepoint. So the
EBs are pulled out of the handshake.
-
PR: https://git.openjdk.java.net/jdk/pull/729
Re: RFR: 8254723: add diagnostic command to write Linux perf map file [v2]
On Thu, 22 Oct 2020 05:00:10 GMT, Thomas Stuefe wrote: > > 1. Like Alexey, I would really wish for an print-at-exit switch. The > common naming seems to be xxxAtExit (so not, OnExit). "PrintXxx" seems to be > printing stuff out to tty, "DumpXxxx" for writing separate files (e.g. CDS > map). So I would name it DumpPerfMapAtExit. > OK, makes sense. > 2. Dumping to /tmp is unexpected for me, I would prefer if the default > were dumping to the current directory. That seems to be the default for other > files too (cds map, hs-err file etc). > > 3. Not necessary but nice would be a an option to specify location of the > dump file. > The `/tmp/perf-.map` is hardcoded into perf though ([see here](https://github.com/torvalds/linux/blob/master/tools/perf/util/map.c#L155)), so I don't think it's useful for the user to be able to change the location. - PR: https://git.openjdk.java.net/jdk/pull/760
Re: RFR: 8254723: add diagnostic command to write Linux perf map file [v2]
On Thu, 22 Oct 2020 04:57:18 GMT, Thomas Stuefe wrote:
>> Nick Gasson has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Update for review comments
>
> src/hotspot/share/code/codeCache.hpp line 194:
>
>> 192: static void print_summary(outputStream* st, bool detailed = true); //
>> Prints a summary of the code cache usage
>> 193: static void log_state(outputStream* st);
>> 194: static void write_perf_map(outputStream* st);
>
> Seems weird for this function to have an outputStream parameter only to write
> the dump to an unrelated file and ignore the stream for everything but the
> final message.
>
> I would either pass in the file name as an option - preferably configurable -
> and write the last message out here; or just write the whole perf dump to the
> outputstream itself, piping it to jcmd and let the caller do what he wants
> with it (e.g. just redirecting). The latter is what most commands do. Not
> sure how large the perf dump gets though, may be impractical.
OK. I think I'll change it so `write_perf_map()` writes to the `outputStream`
and then `PerfMapDCmd::execute()` handles redirecting it to a file. I don't
think it makes sense to write it directly to the jcmd output though.
> src/hotspot/share/code/codeCache.cpp line 1562:
>
>> 1560: }
>> 1561:
>> 1562: void CodeCache::write_perf_map(outputStream* st) {
>
> Could this whole function possibly live inside os/linux in an own file? Or
> would that expose too many code heap internals?
Probably creates too much dependency between the os layer and the codeCache
internals? I'll put it all in `#ifdef LINUX` though.
-
PR: https://git.openjdk.java.net/jdk/pull/760
Re: RFR: 8254723: add diagnostic command to write Linux perf map file [v2]
On Wed, 21 Oct 2020 17:57:46 GMT, Chris Plummer wrote: >> I'm not sure, I didn't want to add too much `#ifdef` mess. The code will >> compile on other platforms, it just won't be called. Better to add `#ifdef`s >> around all of it? > > Any reason not to have this dcmd supported on all platforms even though the > output is really targeted for use with the perf tool on linux? Would a user > ever have any other use for the output other than with the perf tool on linux? @plummercj I'm not sure: it's just a text file so could be easily consumed by tools other than perf. But I'm not aware of any tools on other platforms that could use it. - PR: https://git.openjdk.java.net/jdk/pull/760
