Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]
On Thu, 5 Nov 2020 14:36:44 GMT, Erik Österlund wrote: >> Ok, so there were many test failures with other approaches. Having GC >> trigger the posting was the most reliable way to post the events when the >> tests (and presumably the jvmti customers) expected the events to be posted. >> We could revisit during event disabling if a customer complains about GC >> pause times. > > The point of this change was not necessarily to be lazy about updating the > tagmap, until someone uses it. The point was to get rid of the last annoying > serial GC phase. Doing it all lazily would certainly also achieve that. But > it would also lead to situations where no event is ever posted from GC to GC. > So you would get the event 20 GCs later, which might come as a surprise. It > did come as a surprise to some tests, so it is reasonable to assume it would > come as a surprise to users too. And I don't think we want such surprises > unless we couldn't deal with them. And we can. Kim's change to post the events from the service thread or before other JVMTI operations removes posting events from the gc_notification, which was the objection. - PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]
On Wed, 4 Nov 2020 13:22:57 GMT, Coleen Phillimore wrote: >> For the GCs that call the num_dead notification in a pause it is much worse >> than what we had. As I pointed out elsewhere, it used to be that tagmap >> processing was all-in-one, as a single serial subtask taken by the first >> thread that reached it in WeakProcessor processing. Other threads would find >> that subtask taken and move on to processing oopstores in parallel with the >> tagmap processing. Now everything except the oopstorage-based clearing of >> dead entries is a single threaded serial task done by the VMThread, after >> all the parallel WeakProcessor work is done, because that's where the >> num-dead callbacks are invoked. WeakProcessor's parallel oopstorage >> processing doesn't have a way to do the num-dead callbacks by the last >> thread out of each parallel oopstorage processing. Instead it's left to the >> end, on the assumption that the callbacks are relatively cheap. But that >> could still be much worse than the old code, since the tagmap oopstorage >> could be late in the order of processing, and so still effectively be a serial subtask after all the parallel subtasks are done or mostly done. > > Yes, you are right that the processing will be done serially and not by a > parallel worker thread. This is could spawn a new GC worker thread to > process the posts, as you suggest. We could do that if we find a customer > that has a complaint about the pause time of this processing. So both before and now, this task is a single threaded task. The difference is that before that single threaded task could be performed in parallel to other tasks. So if the table is small, you probably won't be able to notice any difference as small table implies not much to do. And if the table is large, you still probably won't be able to notice any difference as a large table implies it will dominate the pause with both the old and new approach. Any difference at all is bounded at 2x processing time, as it was serial both before and after. But now if we have a perfectly medium balanced table, we can at the very worst observe a theoretical 2x worse processing of this JVMTI table. I think that if we truly did care about this difference, and that it is important to keep this code as well performed as possible, then we would not have a serial phase for this at all. The fact that this has been serial suggests to me that it is not a path that is critical, and therefore I don't think op timizing the theoretical max 2x worse processing times for perfectly medium sized JVMTI tag map tables, is worth the hassle. At least I can't see why this would be of any importance. - PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]
On Wed, 4 Nov 2020 13:32:07 GMT, Coleen Phillimore wrote: >> I know of nothing that leads to "presumably during GC" being a requirement. >> Having all pending events of some type occur before that type of event is >> disabled seems like a reasonable requirement, but just means that event >> disabling also requires the table to be "up to date", in the sense that any >> GC-cleared entries need to be dealt with. That can be handled just like >> other operations that use the table contents, rather than during the GC. >> That is, use post_dead_object_on_vm_thread if there are or might be any >> pending dead objects, before disabling the event. > > Ok, so there were many test failures with other approaches. Having GC > trigger the posting was the most reliable way to post the events when the > tests (and presumably the jvmti customers) expected the events to be posted. > We could revisit during event disabling if a customer complains about GC > pause times. The point of this change was not necessarily to be lazy about updating the tagmap, until someone uses it. The point was to get rid of the last annoying serial GC phase. Doing it all lazily would certainly also achieve that. But it would also lead to situations where no event is ever posted from GC to GC. So you would get the event 20 GCs later, which might come as a surprise. It did come as a surprise to some tests, so it is reasonable to assume it would come as a surprise to users too. And I don't think we want such surprises unless we couldn't deal with them. And we can. - PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]
On Tue, 3 Nov 2020 21:14:04 GMT, Coleen Phillimore wrote: >> src/hotspot/share/prims/jvmtiTagMap.cpp line 3018: >> >>> 3016: } >>> 3017: // Later GC code will relocate the oops, so defer rehashing >>> until then. >>> 3018: tag_map->_needs_rehashing = true; >> >> This is wrong for some collectors. I think all collectors ought to be >> calling set_needs_rehashing in appropriate places, and it can't be be >> correctly piggybacked on the num-dead callback. (See discussion above for >> that function.) >> >> For example, G1 remark pause does weak processing (including weak >> oopstorage) and will call the num-dead callback, but does not move objects, >> so does not require tagmap rehashing. >> >> (I think CMS oldgen remark may have been similar, for what that's worth.) > > Ok, so I'm going to need help to know where in all the different GCs to make > this call. This seemed simpler at the expense of maybe causing a rehash at > some points when it might not be necessary. For what GC is this wrong? I can see that it might yield more work than required, when performing a full GC, but not that it would do too little work. In other words, I can't see how it is wrong, as opposed to inaccurate. Littering GCs with JVMTI hooks so that we can optimize away an operation we do every young GC, from a full GC, does not really seem worth it IMO. - PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]
On Wed, 4 Nov 2020 09:27:39 GMT, Kim Barrett wrote: >> I didn't say it "doesn't work for shenandoah", I said it wouldn't have >> worked with the old shenandoah barriers without additional work, like adding >> calls to resolve. I understand the design intent of notifying the table >> management that its hash codes are out of date. And the num-dead callback >> isn't the right place, since there are num-dead callback invocations that >> aren't associated with hash code invalidation. (It's not a correctness >> wrong, it's a "these things are unrelated and this causes unnecessary work" >> wrong.) > > It used to be that jvmti tagmap processing was all-in-one (in GC weak > reference processing, with the weak clearing, dead table entry removal, and > rehashing all done in one pass. This change has split that up, with the weak > clearing happening in a different place (still as part of the GC's weak > reference processing) than the others (which I claim can now be part of the > mutator, whether further separated or not). > > "Concurrent GC" has nothing to do with whether tagmaps need rehashing. Any > copying collector needs to do so. A non-copying collector (whether concurrent > or not) would not. (We don't have any purely non-copying collectors, but G1 > concurrent oldgen collection is non-copying.) And weak reference clearing > (whether concurrent or not) has nothing to do with whether objects have been > moved and so the hashing has been invalidated. > > There's also a "well known" issue with address-based hashing and generational > or similar collectors, where a simple boolean "objects have moved" flag can > be problematic, and which tagmaps seem likely to be prone to. The old > do_weak_oops tries to mitigate it by recognizing when the object didn't move. The new rehash function also doesn't move the objects either. It essentially does the same as the old weak_oops_do function. - PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]
On Wed, 4 Nov 2020 10:05:29 GMT, Kim Barrett wrote: >> The comment is trying to describe the situation like: >> 1. mark-end pause (WeakHandle.peek() returns NULL because object A is >> unmarked) >> 2. safepoint for heap walk >>2a. Need to post ObjectFree event for object A before the heap walk >> doesn't find object A. >> 3. gc_notification - would have posted an ObjectFree event for object A if >> the heapwalk hadn't intervened >> >> The check_hashmap() function also checks whether the hash table needs to be >> rehashed before the next operation that uses the hashtable. >> >> Both operations require the table to be locked. >> >> The unlink and post needs to be in a GC pause for reasons that I stated >> above. The unlink and post were done in a GC pause so this isn't worse for >> any GCs. The lock can be held for concurrent GC while the number of entries >> are processed and this would be a delay for some applications that have >> requested a lot of tags, but these applications have asked for this and it's >> not worse than what we had with GC walking this table in safepoints. > > For the GCs that call the num_dead notification in a pause it is much worse > than what we had. As I pointed out elsewhere, it used to be that tagmap > processing was all-in-one, as a single serial subtask taken by the first > thread that reached it in WeakProcessor processing. Other threads would find > that subtask taken and move on to processing oopstores in parallel with the > tagmap processing. Now everything except the oopstorage-based clearing of > dead entries is a single threaded serial task done by the VMThread, after all > the parallel WeakProcessor work is done, because that's where the num-dead > callbacks are invoked. WeakProcessor's parallel oopstorage processing doesn't > have a way to do the num-dead callbacks by the last thread out of each > parallel oopstorage processing. Instead it's left to the end, on the > assumption that the callbacks are relatively cheap. But that could still be > much worse than the old code, since the tagmap oopstorage could be late in > the order of processing, and so still effectively be a serial subtask after all the parallel subtasks are done or mostly done. Yes, you are right that the processing will be done serially and not by a parallel worker thread. This is could spawn a new GC worker thread to process the posts, as you suggest. We could do that if we find a customer that has a complaint about the pause time of this processing. >> The JVMTI code expects the posting to be done quite eagerly presumably >> during GC, before it has a chance to disable the event or some other such >> operation. So the posting is done during the notification because it's as >> soon as possible. Deferring to the ServiceThread had two problems. 1. the >> event came later than the caller is expecting it, and in at least one test >> the event was disabled before posting, and 2. there's a comment in the code >> why we can't post events with a JavaThread. We'd have to transition into >> native while holding a no safepoint lock (or else deadlock). The point of >> making this change was so that the JVMTI table does not need GC code to >> serially process the table. > > I know of nothing that leads to "presumably during GC" being a requirement. > Having all pending events of some type occur before that type of event is > disabled seems like a reasonable requirement, but just means that event > disabling also requires the table to be "up to date", in the sense that any > GC-cleared entries need to be dealt with. That can be handled just like other > operations that use the table contents, rather than during the GC. That is, > use post_dead_object_on_vm_thread if there are or might be any pending dead > objects, before disabling the event. Ok, so there were many test failures with other approaches. Having GC trigger the posting was the most reliable way to post the events when the tests (and presumably the jvmti customers) expected the events to be posted. We could revisit during event disabling if a customer complains about GC pause times. - PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]
On Tue, 3 Nov 2020 21:40:39 GMT, Coleen Phillimore wrote: >> src/hotspot/share/prims/jvmtiTagMap.cpp line 127: >> >>> 125: // The table cleaning, posting and rehashing can race for >>> 126: // concurrent GCs. So fix it here once we have a lock or are >>> 127: // at a safepoint. >> >> I think this comment and the one below about locking are confused, at least >> about rehashing. I _think_ this is referring to concurrent num-dead >> notification? I've already commented there about it being a problem to do >> the unlink &etc in the GC pause (see later comment). It also seems like a >> bad idea to be doing this here and block progress by a concurrent GC because >> we're holding the tagmap lock for a long time, which is another reason to >> not have the num-dead notification do very much (and not require a lock that >> might be held here for a long time). > > The comment is trying to describe the situation like: > 1. mark-end pause (WeakHandle.peek() returns NULL because object A is > unmarked) > 2. safepoint for heap walk >2a. Need to post ObjectFree event for object A before the heap walk > doesn't find object A. > 3. gc_notification - would have posted an ObjectFree event for object A if > the heapwalk hadn't intervened > > The check_hashmap() function also checks whether the hash table needs to be > rehashed before the next operation that uses the hashtable. > > Both operations require the table to be locked. > > The unlink and post needs to be in a GC pause for reasons that I stated > above. The unlink and post were done in a GC pause so this isn't worse for > any GCs. The lock can be held for concurrent GC while the number of entries > are processed and this would be a delay for some applications that have > requested a lot of tags, but these applications have asked for this and it's > not worse than what we had with GC walking this table in safepoints. For the GCs that call the num_dead notification in a pause it is much worse than what we had. As I pointed out elsewhere, it used to be that tagmap processing was all-in-one, as a single serial subtask taken by the first thread that reached it in WeakProcessor processing. Other threads would find that subtask taken and move on to processing oopstores in parallel with the tagmap processing. Now everything except the oopstorage-based clearing of dead entries is a single threaded serial task done by the VMThread, after all the parallel WeakProcessor work is done, because that's where the num-dead callbacks are invoked. WeakProcessor's parallel oopstorage processing doesn't have a way to do the num-dead callbacks by the last thread out of each parallel oopstorage processing. Instead it's left to the end, on the assumption that the callbacks are relatively cheap. But that could still be much worse than the old code, since the tagmap oopstorage could be late in the order of processing, an d so still effectively be a serial subtask after all the parallel subtasks are done or mostly done. - PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]
On Wed, 4 Nov 2020 07:52:12 GMT, Kim Barrett wrote: >> So the design is that when the oops have new addresses, we set a flag in the >> table to rehash it. Not sure why this is wrong and why wouldn't it work for >> shenandoah? @zhengyu123 ? When we call WeakHandle.peek()/resolve() after >> the call, the new/moved oop address should be returned. Why wouldn't this >> be the case? > > I didn't say it "doesn't work for shenandoah", I said it wouldn't have worked > with the old shenandoah barriers without additional work, like adding calls > to resolve. I understand the design intent of notifying the table management > that its hash codes are out of date. And the num-dead callback isn't the > right place, since there are num-dead callback invocations that aren't > associated with hash code invalidation. (It's not a correctness wrong, it's > a "these things are unrelated and this causes unnecessary work" wrong.) It used to be that jvmti tagmap processing was all-in-one (in GC weak reference processing, with the weak clearing, dead table entry removal, and rehashing all done in one pass. This change has split that up, with the weak clearing happening in a different place (still as part of the GC's weak reference processing) than the others (which I claim can now be part of the mutator, whether further separated or not). "Concurrent GC" has nothing to do with whether tagmaps need rehashing. Any copying collector needs to do so. A non-copying collector (whether concurrent or not) would not. (We don't have any of those in HotSpot today.) And weak reference clearing (whether concurrent or not) has nothing to do with whether objects have been moved and so the hashing has been invalidated. There's also a "well known" issue with address-based hashing and generational or similar collectors, where a simple boolean "objects have moved" flag can be problematic, and which tagmaps seem likely to be prone to. The old do_weak_oops tries to mitigate it by recognizing when the object didn't move. - PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]
On Tue, 3 Nov 2020 21:31:35 GMT, Coleen Phillimore wrote:
>> src/hotspot/share/prims/jvmtiTagMap.cpp line 2979:
>>
>>> 2977:
>>> 2978: // Concurrent GC needs to call this in relocation pause, so after the
>>> objects are moved
>>> 2979: // and have their new addresses, the table can be rehashed.
>>
>> I think the comment is confusing and wrong. The requirement is that the
>> collector must call this before exposing moved objects to the mutator, and
>> must provide the to-space invariant. (This whole design would not work with
>> the old Shenandoah barriers without additional work. I don't know if tagmaps
>> ever worked at all for them? Maybe they added calls to Access<>::resolve
>> (since happily deceased) to deal with that?) I also think there are a bunch
>> of missing calls; piggybacking on the num-dead callback isn't correct (see
>> later comment about that).
>
> So the design is that when the oops have new addresses, we set a flag in the
> table to rehash it. Not sure why this is wrong and why wouldn't it work for
> shenandoah? @zhengyu123 ? When we call WeakHandle.peek()/resolve() after the
> call, the new/moved oop address should be returned. Why wouldn't this be the
> case?
I didn't say it "doesn't work for shenandoah", I said it wouldn't have worked
with the old shenandoah barriers without additional work, like adding calls to
resolve. I understand the design intent of notifying the table management that
its hash codes are out of date. And the num-dead callback isn't the right
place, since there are num-dead callback invocations that aren't associated
with hash code invalidation. (It's not a correctness wrong, it's a "these
things are unrelated and this causes unnecessary work" wrong.)
>> src/hotspot/share/prims/jvmtiTagMap.cpp line 3015:
>>
>>> 3013: if (tag_map != NULL && !tag_map->is_empty()) {
>>> 3014: if (num_dead_entries != 0) {
>>> 3015: tag_map->hashmap()->unlink_and_post(tag_map->env());
>>
>> Why are we doing this in the callback, rather than just setting a flag? I
>> thought part of the point of this change was to get tagmap processing out of
>> GC pauses. The same question applies to the non-safepoint side. The idea
>> was to be lazy about updating the tagmap, waiting until someone actually
>> needed to use it. Or if more prompt ObjectFree notifications are needed
>> then signal some thread (maybe the service thread?) for followup.
>
> The JVMTI code expects the posting to be done quite eagerly presumably during
> GC, before it has a chance to disable the event or some other such operation.
> So the posting is done during the notification because it's as soon as
> possible. Deferring to the ServiceThread had two problems. 1. the event
> came later than the caller is expecting it, and in at least one test the
> event was disabled before posting, and 2. there's a comment in the code why
> we can't post events with a JavaThread. We'd have to transition into native
> while holding a no safepoint lock (or else deadlock). The point of making
> this change was so that the JVMTI table does not need GC code to serially
> process the table.
I know of nothing that leads to "presumably during GC" being a requirement.
Having all pending events of some type occur before that type of event is
disabled seems like a reasonable requirement, but just means that event
disabling also requires the table to be "up to date", in the sense that any
GC-cleared entries need to be dealt with. That can be handled just like other
operations that use the table contents, rather than during the GC. That is,
use post_dead_object_on_vm_thread if there are or might be any pending dead
objects, before disabling the event.
-
PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]
On Tue, 3 Nov 2020 21:12:49 GMT, Albert Mingkun Yang wrote: >> Coleen Phillimore has updated the pull request with a new target base due to >> a merge or a rebase. The pull request now contains five commits: >> >> - Merge branch 'master' into jvmti-table >> - Merge branch 'master' into jvmti-table >> - More review comments from Stefan and ErikO >> - Code review comments from StefanK. >> - 8212879: Make JVMTI TagMap table not hash on oop address > > src/hotspot/share/utilities/hashtable.cpp line 164: > >> 162: } >> 163: } >> 164: return newsize; > > It is existing code, but could be made clearer as part of this PR: > int newsize; > for (int i=0; i newsize = primelist[i]; > if (newsize >= requested) > break; > } > return newsize; > Additionally, this method could be made `const`, right? > > PS: not a review, just a comment in passing Yes, that is a lot simpler and better. I'd copied that code from another file without changing it that much. - PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]
On Tue, 3 Nov 2020 21:41:55 GMT, Coleen Phillimore wrote: > > @coleenp - please make sure you hear from someone on the Serviceability team > > for this PR... > > I've asked @sspitsyn to review this. Yes, I'm reviewing this. Still need another pass. - PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]
On Mon, 2 Nov 2020 13:19:08 GMT, Coleen Phillimore wrote: >> Coleen Phillimore has updated the pull request with a new target base due to >> a merge or a rebase. The pull request now contains five commits: >> >> - Merge branch 'master' into jvmti-table >> - Merge branch 'master' into jvmti-table >> - More review comments from Stefan and ErikO >> - Code review comments from StefanK. >> - 8212879: Make JVMTI TagMap table not hash on oop address > > I think I addressed your comments, retesting now. Thank you! > @coleenp - please make sure you hear from someone on the Serviceability team > for this PR... I've asked @sspitsyn to review this. - PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]
On Tue, 3 Nov 2020 16:17:58 GMT, Kim Barrett wrote: >> Coleen Phillimore has updated the pull request with a new target base due to >> a merge or a rebase. The pull request now contains five commits: >> >> - Merge branch 'master' into jvmti-table >> - Merge branch 'master' into jvmti-table >> - More review comments from Stefan and ErikO >> - Code review comments from StefanK. >> - 8212879: Make JVMTI TagMap table not hash on oop address > > src/hotspot/share/prims/jvmtiTagMap.cpp line 127: > >> 125: // The table cleaning, posting and rehashing can race for >> 126: // concurrent GCs. So fix it here once we have a lock or are >> 127: // at a safepoint. > > I think this comment and the one below about locking are confused, at least > about rehashing. I _think_ this is referring to concurrent num-dead > notification? I've already commented there about it being a problem to do > the unlink &etc in the GC pause (see later comment). It also seems like a > bad idea to be doing this here and block progress by a concurrent GC because > we're holding the tagmap lock for a long time, which is another reason to not > have the num-dead notification do very much (and not require a lock that > might be held here for a long time). The comment is trying to describe the situation like: 1. mark-end pause (WeakHandle.peek() returns NULL because object A is unmarked) 2. safepoint for heap walk 2a. Need to post ObjectFree event for object A before the heap walk doesn't find object A. 3. gc_notification - would have posted an ObjectFree event for object A if the heapwalk hadn't intervened The check_hashmap() function also checks whether the hash table needs to be rehashed before the next operation that uses the hashtable. Both operations require the table to be locked. The unlink and post needs to be in a GC pause for reasons that I stated above. The unlink and post were done in a GC pause so this isn't worse for any GCs. The lock can be held for concurrent GC while the number of entries are processed and this would be a delay for some applications that have requested a lot of tags, but these applications have asked for this and it's not worse than what we had with GC walking this table in safepoints. - PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]
On Tue, 3 Nov 2020 16:12:21 GMT, Kim Barrett wrote: >> Coleen Phillimore has updated the pull request with a new target base due to >> a merge or a rebase. The pull request now contains five commits: >> >> - Merge branch 'master' into jvmti-table >> - Merge branch 'master' into jvmti-table >> - More review comments from Stefan and ErikO >> - Code review comments from StefanK. >> - 8212879: Make JVMTI TagMap table not hash on oop address > > src/hotspot/share/prims/jvmtiTagMap.cpp line 2979: > >> 2977: >> 2978: // Concurrent GC needs to call this in relocation pause, so after the >> objects are moved >> 2979: // and have their new addresses, the table can be rehashed. > > I think the comment is confusing and wrong. The requirement is that the > collector must call this before exposing moved objects to the mutator, and > must provide the to-space invariant. (This whole design would not work with > the old Shenandoah barriers without additional work. I don't know if tagmaps > ever worked at all for them? Maybe they added calls to Access<>::resolve > (since happily deceased) to deal with that?) I also think there are a bunch > of missing calls; piggybacking on the num-dead callback isn't correct (see > later comment about that). So the design is that when the oops have new addresses, we set a flag in the table to rehash it. Not sure why this is wrong and why wouldn't it work for shenandoah? @zhengyu123 ? When we call WeakHandle.peek()/resolve() after the call, the new/moved oop address should be returned. Why wouldn't this be the case? - PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]
On Mon, 2 Nov 2020 13:19:08 GMT, Coleen Phillimore wrote: >> Coleen Phillimore has updated the pull request with a new target base due to >> a merge or a rebase. The pull request now contains five commits: >> >> - Merge branch 'master' into jvmti-table >> - Merge branch 'master' into jvmti-table >> - More review comments from Stefan and ErikO >> - Code review comments from StefanK. >> - 8212879: Make JVMTI TagMap table not hash on oop address > > I think I addressed your comments, retesting now. Thank you! @coleenp - please make sure you hear from someone on the Serviceability team for this PR... - PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]
On Tue, 3 Nov 2020 14:50:36 GMT, Kim Barrett wrote:
>> Coleen Phillimore has updated the pull request with a new target base due to
>> a merge or a rebase. The pull request now contains five commits:
>>
>> - Merge branch 'master' into jvmti-table
>> - Merge branch 'master' into jvmti-table
>> - More review comments from Stefan and ErikO
>> - Code review comments from StefanK.
>> - 8212879: Make JVMTI TagMap table not hash on oop address
>
> src/hotspot/share/prims/jvmtiTagMap.cpp line 3015:
>
>> 3013: if (tag_map != NULL && !tag_map->is_empty()) {
>> 3014: if (num_dead_entries != 0) {
>> 3015: tag_map->hashmap()->unlink_and_post(tag_map->env());
>
> Why are we doing this in the callback, rather than just setting a flag? I
> thought part of the point of this change was to get tagmap processing out of
> GC pauses. The same question applies to the non-safepoint side. The idea
> was to be lazy about updating the tagmap, waiting until someone actually
> needed to use it. Or if more prompt ObjectFree notifications are needed then
> signal some thread (maybe the service thread?) for followup.
The JVMTI code expects the posting to be done quite eagerly presumably during
GC, before it has a chance to disable the event or some other such operation.
So the posting is done during the notification because it's as soon as
possible. Deferring to the ServiceThread had two problems. 1. the event came
later than the caller is expecting it, and in at least one test the event was
disabled before posting, and 2. there's a comment in the code why we can't post
events with a JavaThread. We'd have to transition into native while holding a
no safepoint lock (or else deadlock). The point of making this change was so
that the JVMTI table does not need GC code to serially process the table.
-
PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]
On Tue, 3 Nov 2020 14:53:12 GMT, Coleen Phillimore wrote: >> This change turns the HashTable that JVMTI uses for object tagging into a >> regular Hotspot hashtable - the one in hashtable.hpp with resizing and >> rehashing. Instead of pointing directly to oops so that GC has to walk the >> table to follow oops and then to rehash the table, this table points to >> WeakHandle. GC walks the backing OopStorages concurrently. >> >> The hash function for the table is a hash of the lower 32 bits of the >> address. A flag is set during GC (gc_notification if in a safepoint, and >> through a call to JvmtiTagMap::needs_processing()) so that the table is >> rehashed at the next use. >> >> The gc_notification mechanism of weak oop processing is used to notify Jvmti >> to post ObjectFree events. In concurrent GCs there can be a window of time >> between weak oop marking where the oop is unmarked, so dead (the phantom >> load in peek returns NULL) but the gc_notification hasn't been done yet. In >> this window, a heap walk or GetObjectsWithTags call would not find an object >> before the ObjectFree event is posted. This is dealt with in two ways: >> >> 1. In the Heap walk, there's an unconditional table walk to post events if >> events are needed to post. >> 2. For GetObjectWithTags, if a dead oop is found in the table and posting is >> required, we use the VM thread to post the event. >> >> Event posting cannot be done in a JavaThread because the posting needs to be >> done while holding the table lock, so that the JvmtiEnv state doesn't change >> before posting is done. ObjectFree callbacks are limited in what they can >> do as per the JVMTI Specification. The allowed callbacks to the VM already >> have code to allow NonJava threads. >> >> To avoid rehashing, I also tried to use object->identity_hash() but this >> breaks because entries can be added to the table during heapwalk, where the >> objects use marking. The starting markWord is saved and restored. Adding a >> hashcode during this operation makes restoring the former markWord (locked, >> inflated, etc) too complicated. Plus we don't want all these objects to >> have hashcodes because locking operations after tagging would have to always >> use inflated locks. >> >> Much of this change is to remove serial weak oop processing for the >> weakProcessor, ZGC and Shenandoah. The GCs have been stress tested with >> jvmti code. >> >> It has also been tested with tier1-6. >> >> Thank you to Stefan, Erik and Kim for their help with this change. > > Coleen Phillimore has updated the pull request with a new target base due to > a merge or a rebase. The pull request now contains five commits: > > - Merge branch 'master' into jvmti-table > - Merge branch 'master' into jvmti-table > - More review comments from Stefan and ErikO > - Code review comments from StefanK. > - 8212879: Make JVMTI TagMap table not hash on oop address src/hotspot/share/utilities/hashtable.cpp line 164: > 162: } > 163: } > 164: return newsize; It is existing code, but could be made clearer as part of this PR: int newsize; for (int i=0; i= requested) break; } return newsize; Additionally, this method could be made `const`, right? PS: not a review, just a comment in passing - PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]
On Tue, 3 Nov 2020 14:47:35 GMT, Kim Barrett wrote: >> Coleen Phillimore has updated the pull request with a new target base due to >> a merge or a rebase. The pull request now contains five commits: >> >> - Merge branch 'master' into jvmti-table >> - Merge branch 'master' into jvmti-table >> - More review comments from Stefan and ErikO >> - Code review comments from StefanK. >> - 8212879: Make JVMTI TagMap table not hash on oop address > > src/hotspot/share/prims/jvmtiTagMap.cpp line 3018: > >> 3016: } >> 3017: // Later GC code will relocate the oops, so defer rehashing >> until then. >> 3018: tag_map->_needs_rehashing = true; > > This is wrong for some collectors. I think all collectors ought to be calling > set_needs_rehashing in appropriate places, and it can't be be correctly > piggybacked on the num-dead callback. (See discussion above for that > function.) > > For example, G1 remark pause does weak processing (including weak oopstorage) > and will call the num-dead callback, but does not move objects, so does not > require tagmap rehashing. > > (I think CMS oldgen remark may have been similar, for what that's worth.) Ok, so I'm going to need help to know where in all the different GCs to make this call. - PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]
On Tue, 3 Nov 2020 14:53:12 GMT, Coleen Phillimore wrote:
>> This change turns the HashTable that JVMTI uses for object tagging into a
>> regular Hotspot hashtable - the one in hashtable.hpp with resizing and
>> rehashing. Instead of pointing directly to oops so that GC has to walk the
>> table to follow oops and then to rehash the table, this table points to
>> WeakHandle. GC walks the backing OopStorages concurrently.
>>
>> The hash function for the table is a hash of the lower 32 bits of the
>> address. A flag is set during GC (gc_notification if in a safepoint, and
>> through a call to JvmtiTagMap::needs_processing()) so that the table is
>> rehashed at the next use.
>>
>> The gc_notification mechanism of weak oop processing is used to notify Jvmti
>> to post ObjectFree events. In concurrent GCs there can be a window of time
>> between weak oop marking where the oop is unmarked, so dead (the phantom
>> load in peek returns NULL) but the gc_notification hasn't been done yet. In
>> this window, a heap walk or GetObjectsWithTags call would not find an object
>> before the ObjectFree event is posted. This is dealt with in two ways:
>>
>> 1. In the Heap walk, there's an unconditional table walk to post events if
>> events are needed to post.
>> 2. For GetObjectWithTags, if a dead oop is found in the table and posting is
>> required, we use the VM thread to post the event.
>>
>> Event posting cannot be done in a JavaThread because the posting needs to be
>> done while holding the table lock, so that the JvmtiEnv state doesn't change
>> before posting is done. ObjectFree callbacks are limited in what they can
>> do as per the JVMTI Specification. The allowed callbacks to the VM already
>> have code to allow NonJava threads.
>>
>> To avoid rehashing, I also tried to use object->identity_hash() but this
>> breaks because entries can be added to the table during heapwalk, where the
>> objects use marking. The starting markWord is saved and restored. Adding a
>> hashcode during this operation makes restoring the former markWord (locked,
>> inflated, etc) too complicated. Plus we don't want all these objects to
>> have hashcodes because locking operations after tagging would have to always
>> use inflated locks.
>>
>> Much of this change is to remove serial weak oop processing for the
>> weakProcessor, ZGC and Shenandoah. The GCs have been stress tested with
>> jvmti code.
>>
>> It has also been tested with tier1-6.
>>
>> Thank you to Stefan, Erik and Kim for their help with this change.
>
> Coleen Phillimore has updated the pull request with a new target base due to
> a merge or a rebase. The pull request now contains five commits:
>
> - Merge branch 'master' into jvmti-table
> - Merge branch 'master' into jvmti-table
> - More review comments from Stefan and ErikO
> - Code review comments from StefanK.
> - 8212879: Make JVMTI TagMap table not hash on oop address
src/hotspot/share/prims/jvmtiTagMap.cpp line 3018:
> 3016: }
> 3017: // Later GC code will relocate the oops, so defer rehashing
> until then.
> 3018: tag_map->_needs_rehashing = true;
This is wrong for some collectors. I think all collectors ought to be calling
set_needs_rehashing in appropriate places, and it can't be be correctly
piggybacked on the num-dead callback. (See discussion above for that function.)
For example, G1 remark pause does weak processing (including weak oopstorage)
and will call the num-dead callback, but does not move objects, so does not
require tagmap rehashing.
(I think CMS oldgen remark may have been similar, for what that's worth.)
src/hotspot/share/prims/jvmtiTagMap.cpp line 3015:
> 3013: if (tag_map != NULL && !tag_map->is_empty()) {
> 3014: if (num_dead_entries != 0) {
> 3015: tag_map->hashmap()->unlink_and_post(tag_map->env());
Why are we doing this in the callback, rather than just setting a flag? I
thought part of the point of this change was to get tagmap processing out of GC
pauses. The same question applies to the non-safepoint side. The idea was to
be lazy about updating the tagmap, waiting until someone actually needed to use
it. Or if more prompt ObjectFree notifications are needed then signal some
thread (maybe the service thread?) for followup.
src/hotspot/share/prims/jvmtiTagMap.cpp line 2979:
> 2977:
> 2978: // Concurrent GC needs to call this in relocation pause, so after the
> objects are moved
> 2979: // and have their new addresses, the table can be rehashed.
I think the comment is confusing and wrong. The requirement is that the
collector must call this before exposing moved objects to the mutator, and must
provide the to-space invariant. (This whole design would not work with the old
Shenandoah barriers without additional work. I don't know if tagmaps ever
worked at all for them? Maybe they added calls to Access<>::resolve (since
happily deceased) to deal with that?) I a
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]
> This change turns the HashTable that JVMTI uses for object tagging into a > regular Hotspot hashtable - the one in hashtable.hpp with resizing and > rehashing. Instead of pointing directly to oops so that GC has to walk the > table to follow oops and then to rehash the table, this table points to > WeakHandle. GC walks the backing OopStorages concurrently. > > The hash function for the table is a hash of the lower 32 bits of the > address. A flag is set during GC (gc_notification if in a safepoint, and > through a call to JvmtiTagMap::needs_processing()) so that the table is > rehashed at the next use. > > The gc_notification mechanism of weak oop processing is used to notify Jvmti > to post ObjectFree events. In concurrent GCs there can be a window of time > between weak oop marking where the oop is unmarked, so dead (the phantom load > in peek returns NULL) but the gc_notification hasn't been done yet. In this > window, a heap walk or GetObjectsWithTags call would not find an object > before the ObjectFree event is posted. This is dealt with in two ways: > > 1. In the Heap walk, there's an unconditional table walk to post events if > events are needed to post. > 2. For GetObjectWithTags, if a dead oop is found in the table and posting is > required, we use the VM thread to post the event. > > Event posting cannot be done in a JavaThread because the posting needs to be > done while holding the table lock, so that the JvmtiEnv state doesn't change > before posting is done. ObjectFree callbacks are limited in what they can do > as per the JVMTI Specification. The allowed callbacks to the VM already have > code to allow NonJava threads. > > To avoid rehashing, I also tried to use object->identity_hash() but this > breaks because entries can be added to the table during heapwalk, where the > objects use marking. The starting markWord is saved and restored. Adding a > hashcode during this operation makes restoring the former markWord (locked, > inflated, etc) too complicated. Plus we don't want all these objects to have > hashcodes because locking operations after tagging would have to always use > inflated locks. > > Much of this change is to remove serial weak oop processing for the > weakProcessor, ZGC and Shenandoah. The GCs have been stress tested with > jvmti code. > > It has also been tested with tier1-6. > > Thank you to Stefan, Erik and Kim for their help with this change. Coleen Phillimore has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains five commits: - Merge branch 'master' into jvmti-table - Merge branch 'master' into jvmti-table - More review comments from Stefan and ErikO - Code review comments from StefanK. - 8212879: Make JVMTI TagMap table not hash on oop address - Changes: https://git.openjdk.java.net/jdk/pull/967/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=967&range=03 Stats: 1749 lines in 41 files changed: 627 ins; 1020 del; 102 mod Patch: https://git.openjdk.java.net/jdk/pull/967.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/967/head:pull/967 PR: https://git.openjdk.java.net/jdk/pull/967
