On Thu, 19 Nov 2020 00:39:52 GMT, Kim Barrett <kbarr...@openjdk.org> wrote:
>> Hi Coleen,
>> It looks good to me.
>> Just a couple of nits below.
>>
>> src/hotspot/share/prims/jvmtiTagMap.cpp:
>>
>> There is a double-check for _needs_cleaning, so the one at line 136 can be
>> removed:
>> 136 if (_needs_cleaning &&
>> 137 post_events &&
>> 138 env()->is_enabled(JVMTI_EVENT_OBJECT_FREE)) {
>> 139 remove_dead_entries(true /* post_object_free */);
>> 1158 void JvmtiTagMap::remove_dead_entries(bool post_object_free) {
>> 1159 assert(is_locked(), "precondition");
>> 1160 if (_needs_cleaning) {
>> 1161 log_info(jvmti, table)("TagMap table needs cleaning%s",
>> 1162 (post_object_free ? " and posting" : ""));
>> 1163 hashmap()->remove_dead_entries(env(), post_object_free);
>> 1164 _needs_cleaning = false;
>> 1165 }
>> 1166 }
>> test/hotspot/jtreg/vmTestbase/nsk/jvmti/AttachOnDemand/attach021/attach021Agent00.cpp:
>>
>> The change below is not needed as the call to
>> nsk_jvmti_aod_disableEventAndFinish() does exactly the same:
>> - nsk_jvmti_aod_disableEventAndFinish(agentName, JVMTI_EVENT_OBJECT_FREE,
>> success, jvmti, jni);
>> +
>> + /* Flush any pending ObjectFree events, which may set success to 1 */
>> + if (jvmti->SetEventNotificationMode(JVMTI_DISABLE,
>> + JVMTI_EVENT_OBJECT_FREE,
>> + NULL) != JVMTI_ERROR_NONE) {
>> + success = 0;
>> + }
>> +
>> + nsk_aod_agentFinished(jni, agentName, success);
>> }
>
>> test/hotspot/jtreg/vmTestbase/nsk/jvmti/AttachOnDemand/attach021/attach021Agent00.cpp:
>>
>> The change below is not needed as the call to
>> nsk_jvmti_aod_disableEventAndFinish() does exactly the same:
>>
>> ```
>> - nsk_jvmti_aod_disableEventAndFinish(agentName, JVMTI_EVENT_OBJECT_FREE,
>> success, jvmti, jni);
>> +
>> + /* Flush any pending ObjectFree events, which may set success to 1 */
>> + if (jvmti->SetEventNotificationMode(JVMTI_DISABLE,
>> + JVMTI_EVENT_OBJECT_FREE,
>> + NULL) != JVMTI_ERROR_NONE) {
>> + success = 0;
>> + }
>> +
>> + nsk_aod_agentFinished(jni, agentName, success);
>> }
>> ```
>
> This change really is needed.
>
> The success variable in the test is a global, initially 0, set to 1 by the
> ObjectFree handler.
>
> In the old code, if the ObjectFree event hasn't been posted yet, we pass the
> initial 0 value of success to nsk_jvmti_aod_disabledEventAndFinish, where
> it's a local variable (so unaffected by any changes to the variable in the
> test), so stays 0 through to the call to nsk_aod_agentFinished. So the test
> fails.
>
> The split in the change causes the updated post-ObjectFree event success
> value of 1 to be passed to agentFinished. So the test passes.
>
> That required some head scratching to find at the time. That's the point of
> the comment about flushing pending events. Maybe the comment should be
> improved.
@kimbarrett
Okay, thank you for explanation.
I agree, it'd make sense to improve the comment a little bit.
Thanks,
Serguei
-------------
PR: https://git.openjdk.java.net/jdk/pull/967
