The new job finished, its ID is:

 mach5-one-rkennke-JDK-8227269-2-20200325-2027-9716289

Thank you,
Roman


> Yes, please submit a new job. I'll start my testing once I see that the
> builds are done.
> 
> Chris
> 
> On 3/25/20 12:59 PM, Roman Kennke wrote:
>> Hi Chris,
>>
>> Apparently we can get into classTrack_reset() before calling activate(),
>> and we're seeing a null deletedSignatureBag. A simple NULL-check around
>> the cleaning routine fixes the problem for me.
>>
>> http://cr.openjdk.java.net/~rkennke/JDK-8227269/webrev.08/
>>
>> Should I post another submit-repo job with that fix?
>>
>> Thanks,
>> Roman
>>
>>
>>> Hi Roman,
>>>
>>> com/sun/jdi/JdwpAllowTest.java crashed on many runs:
>>>
>>> Stack: [0x00007fbb790f9000,0x00007fbb791fa000], 
>>> sp=0x00007fbb791f8af0,  free space=1022k
>>> Native frames: (J=compiled Java code, A=aot compiled Java code,
>>> j=interpreted, Vv=VM code, C=native code)
>>> C  [libjdwp.so+0xdb71]  bagEnumerateOver+0x11
>>> C  [libjdwp.so+0xe365]  classTrack_reset+0x25
>>> C  [libjdwp.so+0xfca1]  debugInit_reset+0x71
>>> C  [libjdwp.so+0x12e0d]  debugLoop_run+0x38d
>>> C  [libjdwp.so+0x25700]  acceptThread+0x80
>>> V  [libjvm.so+0xf4b5a7]  JvmtiAgentThread::call_start_function()+0x1c7
>>> V  [libjvm.so+0x15215c6]  JavaThread::thread_main_inner()+0x226
>>> V  [libjvm.so+0x1527736]  Thread::call_run()+0xf6
>>> V  [libjvm.so+0x1250ade]  thread_native_entry(Thread*)+0x10e
>>>
>>>
>>> This happened during a test task run of open/test/jdk/:jdk_jdi. There
>>> doesn't seem to be anything magic on the command line that might be
>>> triggering. Pretty much I see it with all the various VM configs we
>>> test.
>>>
>>> I'm also seeing crashes in the following tests, but not as often:
>>>
>>> serviceability/jvmti/ModuleAwareAgents/ThreadStart/MAAThreadStart.java
>>> vmTestbase/nsk/jdwp/VirtualMachine/Version/version002/TestDescription.java
>>>
>>> vmTestbase/nsk/jdwp/VirtualMachine/ReleaseEvents/releaseevents002/TestDescription.java
>>>
>>> vmTestbase/nsk/jdwp/VirtualMachine/HoldEvents/holdevents002/TestDescription.java
>>>
>>> vmTestbase/nsk/jdwp/VirtualMachine/Dispose/dispose001/TestDescription.java
>>>
>>>
>>> thanks,
>>>
>>> Chris
>>>
>>>
>>> On 3/25/20 11:37 AM, Roman Kennke wrote:
>>>> Hi Chris,
>>>>
>>>>> Regarding the new assert:
>>>>>
>>>>>   105     if (gdata && gdata->assertOn) {
>>>>>   106         // Check this is not already tagged.
>>>>>   107         jlong tag;
>>>>>   108         error = JVMTI_FUNC_PTR(trackingEnv, GetTag)(env,
>>>>> klass, &tag);
>>>>>   109         if (error != JVMTI_ERROR_NONE) {
>>>>>   110             EXIT_ERROR(error, "Unable to GetTag with class
>>>>> trackingEnv");
>>>>>   111         }
>>>>>   112         JDI_ASSERT(tag == NOT_TAGGED);
>>>>>   113     }
>>>>>
>>>>> I think you should remove the gdata check. gdata should never be NULL
>>>>> when you get to this code. If it is ever NULL then there's a bug, and
>>>>> the check will hide the bug.
>>>> Ok, will remove this.
>>>>
>>>>> Regarding testing, after you do the submit repo testing let me know
>>>>> the
>>>>> jobID and I'll do additional testing on it.
>>>> I did the submit repo earlier today, and it came back green:
>>>>
>>>> mach5-one-rkennke-JDK-8227269-2-20200325-1421-9706762
>>>>
>>>> Thanks,
>>>> Roman
>>>>
>>>>> thanks,
>>>>>
>>>>> Chris
>>>>>
>>>>> On 3/25/20 6:00 AM, Roman Kennke wrote:
>>>>>> Hi Sergei,
>>>>>>
>>>>>>> The fix looks pretty clean now.
>>>>>>> I also like new name of the lock.:)
>>>>>> Thank you!
>>>>>>
>>>>>>> Just one comment below.
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~rkennke/JDK-8227269/webrev.06/src/jdk.jdwp.agent/share/native/libjdwp/classTrack.c.frames.html
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> 110 if (tag != 0l) {
>>>>>>> 111 return; // Already added
>>>>>>>    112     }
>>>>>>>
>>>>>>>    It is better to use a named constant or macro instead.
>>>>>>>    Also, it'd be nice to add a short comment about this value is.
>>>>>> As I replied to Chris earlier, this whole block can be turned into an
>>>>>> assert. I also made a constant for the value 0, which should be
>>>>>> pretty
>>>>>> much self-explaining.
>>>>>>
>>>>>> http://cr.openjdk.java.net/~rkennke/JDK-8227269/webrev.07/
>>>>>>
>>>>>>> How do you test the fix?
>>>>>> I am using a manual test that is provided in this bug report:
>>>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1751985
>>>>>>
>>>>>> "Script to compare performance of GC with and without debugger, when
>>>>>> many classes are loaded and classes are being unloaded":
>>>>>>
>>>>>> https://bugzilla.redhat.com/attachment.cgi?id=1640688
>>>>>>
>>>>>> I am also using this test and manually attach/detach jdb a couple of
>>>>>> times in a row to check that disconnecting and reconnecting works
>>>>>> well
>>>>>> (this tended to deadlock or crash with an earlier version of the
>>>>>> patch,
>>>>>> and is now looking good).
>>>>>>
>>>>>> I am also running tier1 and tier2 tests locally, and as soon as we
>>>>>> all
>>>>>> agree that the fix is reasonable, I will push it to the submit
>>>>>> repo. I
>>>>>> am not sure if any of those tests actually exercise that code,
>>>>>> though.
>>>>>> Let me know if you want me to run any specific tests.
>>>>>>
>>>>>> Thank you,
>>>>>> Roman
>>>>>>
>>>>>>
>>>>>>
>>>>>>> Thanks,
>>>>>>> Serguei
>>>>>>>
>>>>>>>
>>>>>>> On 3/20/20 08:30, Roman Kennke wrote:
>>>>>>>> I believe I came up with a much simpler solution that also
>>>>>>>> solves the
>>>>>>>> problems of the existing one, and the ones I proposed earlier.
>>>>>>>>
>>>>>>>> It turns out that we can take advantage of the fact that we can use
>>>>>>>> *anything* as tags in JVMTI, even pointers to stuff (this is
>>>>>>>> explicitely
>>>>>>>> mentioned in the JVMTI spec). This means we can simply stick a
>>>>>>>> pointer
>>>>>>>> to the signature of a class into the tag, and pull it out again
>>>>>>>> when we
>>>>>>>> get notified that the class gets unloaded.
>>>>>>>>
>>>>>>>> This means we don't need an extra data-structure to keep track of
>>>>>>>> classes and signatures, and it also makes the story around locking
>>>>>>>> *much* simpler. Performance-wise this is O(1), i.e. no scanning
>>>>>>>> of all
>>>>>>>> classes needed (as in the current implementation) and no
>>>>>>>> searching of
>>>>>>>> table needed (like in my previous attempts).
>>>>>>>>
>>>>>>>> Please review this new revision:
>>>>>>>> http://cr.openjdk.java.net/~rkennke/JDK-8227269/webrev.06/
>>>>>>>>
>>>>>>>> (Notice that there still appears to be a performance bottleneck
>>>>>>>> with
>>>>>>>> class-unloading when an actual debugger is attached. This
>>>>>>>> doesn't seem
>>>>>>>> to be related to the classTrack.c implementation though, but
>>>>>>>> looks like
>>>>>>>> a consequence of getting all those class-unload notifications
>>>>>>>> over the
>>>>>>>> wire. My testcase generates 1000s of them, and it's clogging up the
>>>>>>>> buffers.)
>>>>>>>>
>>>>>>>> I am not sure why jdb needs to enable class-unload listener
>>>>>>>> always. A
>>>>>>>> simple hack disables it, and performance is brilliant, even when
>>>>>>>> jdb is
>>>>>>>> attached:
>>>>>>>> http://cr.openjdk.java.net/~rkennke/disable-jdk-class-unload.patch
>>>>>>>>
>>>>>>>> But this is not in the scope of this bug.)
>>>>>>>>
>>>>>>>> Roman
>>>>>>>>
>>>>>>>>
>>>>>>>> On 3/16/20 8:05 AM, serguei.spit...@oracle.com wrote:
>>>>>>>>> Sorry, forgot to complete my comments at the end (see below).
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 3/15/20 23:57, serguei.spit...@oracle.com wrote:
>>>>>>>>>> Hi Roman,
>>>>>>>>>>
>>>>>>>>>> Thank you for the update and sorry for the latency in review.
>>>>>>>>>>
>>>>>>>>>> Some comments are below.
>>>>>>>>>>
>>>>>>>>>> http://cr.openjdk.java.net/~rkennke/JDK-8227269/webrev.05/src/jdk.jdwp.agent/share/native/libjdwp/classTrack.c.frames.html
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> 87 cbTrackingObjectFree(jvmtiEnv* jvmti_env, jlong tag)
>>>>>>>>>>     88 {
>>>>>>>>>> 89 debugMonitorEnter(deletedSignatureLock);
>>>>>>>>>> 90 if (currentClassTag == -1) {
>>>>>>>>>> 91 // Class tracking not initialized, nobody's interested
>>>>>>>>>> 92 debugMonitorExit(deletedSignatureLock);
>>>>>>>>>> 93 return;
>>>>>>>>>>     94     }
>>>>>>>>>> Just a question:
>>>>>>>>>>     Q1: Should the ObjectFree events be disabled for the jvmtiEnv
>>>>>>>>>> that does
>>>>>>>>>>         the class tracking if class tracking has not been
>>>>>>>>>> initialized?
>>>>>>>>>>
>>>>>>>>>> 70 static jlong currentClassTag; I'm thinking if the name is
>>>>>>>>>> better to
>>>>>>>>>> be something like: lastClassTag or highestClassTag.
>>>>>>>>>>
>>>>>>>>>> 99 KlassNode* klass = *klass_ptr;
>>>>>>>>>> 100 102 while (klass != NULL && klass->klass_tag != tag) { 103
>>>>>>>>>> klass_ptr = &klass->next; 104 klass = *klass_ptr;
>>>>>>>>>> 105 } 106 if (klass != NULL || klass->klass_tag != tag) { //
>>>>>>>>>> klass
>>>>>>>>>> not
>>>>>>>>>> found - ignore.
>>>>>>>>>> 107 debugMonitorExit(deletedSignatureLock);
>>>>>>>>>> 108 return;
>>>>>>>>>>    109     }
>>>>>>>>>>    It seems to me, something is wrong in the condition at L106
>>>>>>>>>> above.
>>>>>>>>>>    Should it be? :
>>>>>>>>>>       if (klass == NULL || klass->klass_tag != tag)
>>>>>>>>>>
>>>>>>>>>>    Otherwise, how can the second check ever work correctly as the
>>>>>>>>>> return
>>>>>>>>>> will always happen when (klass != NULL)?
>>>>>>>>>>
>>>>>>>>>>    There are several places in this file with the the indent:
>>>>>>>>>> 90 if (currentClassTag == -1) {
>>>>>>>>>> 91 // Class tracking not initialized, nobody's interested
>>>>>>>>>> 92 debugMonitorExit(deletedSignatureLock);
>>>>>>>>>> 93 return;
>>>>>>>>>>     94     }
>>>>>>>>>>    ...
>>>>>>>>>> 152 if (currentClassTag == -1) {
>>>>>>>>>> 153 // Class tracking not initialized yet, nobody's interested
>>>>>>>>>> 154 debugMonitorExit(deletedSignatureLock);
>>>>>>>>>> 155 return;
>>>>>>>>>>    156     }
>>>>>>>>>>    ...
>>>>>>>>>> 161 if (error != JVMTI_ERROR_NONE) {
>>>>>>>>>> 162 EXIT_ERROR(error, "Unable to GetTag with class trackingEnv");
>>>>>>>>>>    163     }
>>>>>>>>>> 164 if (tag != 0l) {
>>>>>>>>>> 165 debugMonitorExit(deletedSignatureLock);
>>>>>>>>>> 166 return; // Already added
>>>>>>>>>>    167     }
>>>>>>>>>>    ...
>>>>>>>>>> 281 cleanDeleted(void *signatureVoid, void *arg)
>>>>>>>>>> 282 {
>>>>>>>>>> 283 char* sig = (char*)signatureVoid;
>>>>>>>>>> 284 jvmtiDeallocate(sig);
>>>>>>>>>> 285 return JNI_TRUE;
>>>>>>>>>>    286 }
>>>>>>>>>>    ...
>>>>>>>>>>    291 void
>>>>>>>>>>    292 classTrack_reset(void)
>>>>>>>>>>    293 {
>>>>>>>>>> 294 int idx;
>>>>>>>>>> 295 debugMonitorEnter(deletedSignatureLock);
>>>>>>>>>> 296
>>>>>>>>>> 297 for (idx = 0; idx < CT_SLOT_COUNT; ++idx) {
>>>>>>>>>> 298 KlassNode* node = table[idx];
>>>>>>>>>> 299 while (node != NULL) {
>>>>>>>>>> 300 KlassNode* next = node->next;
>>>>>>>>>> 301 jvmtiDeallocate(node->signature);
>>>>>>>>>> 302 jvmtiDeallocate(node);
>>>>>>>>>> 303 node = next;
>>>>>>>>>> 304 }
>>>>>>>>>> 305 }
>>>>>>>>>> 306 jvmtiDeallocate(table);
>>>>>>>>>> 307
>>>>>>>>>> 308 bagEnumerateOver(deletedSignatureBag, cleanDeleted, NULL);
>>>>>>>>>> 309 bagDestroyBag(deletedSignatureBag);
>>>>>>>>>> 310
>>>>>>>>>> 311 currentClassTag = -1;
>>>>>>>>>> 312
>>>>>>>>>> 313
>>>>>>>>>> (void)JVMTI_FUNC_PTR(trackingEnv,DisposeEnvironment)(trackingEnv);
>>>>>>>>>>
>>>>>>>>>> 314 trackingEnv = NULL;
>>>>>>>>>> 315
>>>>>>>>>> 316 debugMonitorExit(deletedSignatureLock);
>>>>>>>>>>
>>>>>>>>>> Could you, please, fix several comments below?
>>>>>>>>>> 63 * The JVMTI tracking env to keep track of klass tags, for
>>>>>>>>>> class-unloads
>>>>>>>>>>    The comma is not needed.
>>>>>>>>>>    Would it better to replace: klass tags => klass_tag's ?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> 73 * Lock to keep table, currentClassTag and deletedSignatureBag
>>>>>>>>>> consistent
>>>>>>>>>>    Maybe: Lock to guard ... or lock to keep integrity of ...
>>>>>>>>>>
>>>>>>>>>> 84 * Callback when classes are freed, Finds the signature and
>>>>>>>>>> remembers it in deletedSignatureBag. Would be better to use words
>>>>>>>>>> like
>>>>>>>>>> "store" or "record", "Find" should not start from capital letter:
>>>>>>>>>> Invoke the callback when classes are freed, find and record the
>>>>>>>>>> signature in deletedSignatureBag.
>>>>>>>>>>
>>>>>>>>>> 96 // Find deleted KlassNode 133 // Class tracking not
>>>>>>>>>> initialized,
>>>>>>>>>> nobody's interested 153 // Class tracking not initialized yet,
>>>>>>>>>> nobody's interested 158 /* Check this is not a duplicate */
>>>>>>>>>> Missed
>>>>>>>>>> dot
>>>>>>>>>> at the end. 106 if (klass != NULL || klass->klass_tag != tag)
>>>>>>>>>> { //
>>>>>>>>>> klass not found - ignore. In opposite, dot is not needed as the
>>>>>>>>>> comment does not start from a capital letter. 111 // At this
>>>>>>>>>> point we
>>>>>>>>>> have the KlassNode corresponding to the tag
>>>>>>>>>> 112 // in klass, and the pointer to it in klass_node.
>>>>>>>>>    The comment above can be better. Maybe, something like:
>>>>>>>>>      " At this point, we found the KlassNode matching the klass
>>>>>>>>> tag(and it is
>>>>>>>>> linked).
>>>>>>>>>
>>>>>>>>>> 113 // Remember the unloaded signature.
>>>>>>>>>    Better: Record the signature of the unloaded class and
>>>>>>>>> unlink it.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Serguei
>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Serguei
>>>>>>>>>>
>>>>>>>>>> On 3/9/20 05:39, Roman Kennke wrote:
>>>>>>>>>>> Hello all,
>>>>>>>>>>>
>>>>>>>>>>> Can I please get reviews of this change? In the meantime,
>>>>>>>>>>> we've done
>>>>>>>>>>> more testing and also field-/torture-testing by a customer
>>>>>>>>>>> who is
>>>>>>>>>>> happy
>>>>>>>>>>> now. :-)
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Roman
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> Hi Serguei,
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks for reviewing!
>>>>>>>>>>>>
>>>>>>>>>>>> I updated the patch to reflect your suggestions, very good!
>>>>>>>>>>>> It also includes a fix to allow re-connecting an agent after
>>>>>>>>>>>> disconnect,
>>>>>>>>>>>> namely move setup of the trackingEnv and deletedSignatureBag to
>>>>>>>>>>>> _activate() to ensure have those structures after re-connect.
>>>>>>>>>>>>
>>>>>>>>>>>> http://cr.openjdk.java.net/~rkennke/JDK-8227269/webrev.05/
>>>>>>>>>>>>
>>>>>>>>>>>> Let me know what you think!
>>>>>>>>>>>> Roman
>>>>>>>>>>>>
>>>>>>>>>>>>> Hi Roman,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thank you for taking care about this scalability issue!
>>>>>>>>>>>>>
>>>>>>>>>>>>> I have a couple of quick comments.
>>>>>>>>>>>>>
>>>>>>>>>>>>> http://cr.openjdk.java.net/~rkennke/JDK-8227269/webrev.04/src/jdk.jdwp.agent/share/native/libjdwp/classTrack.c.frames.html
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> 72 /*
>>>>>>>>>>>>> 73 * Lock to protect deletedSignatureBag
>>>>>>>>>>>>> 74 */
>>>>>>>>>>>>> 75 static jrawMonitorID deletedSignatureLock; 76 77 /*
>>>>>>>>>>>>> 78 * A bag containing all the deleted classes' signatures.
>>>>>>>>>>>>> Must be
>>>>>>>>>>>>> accessed under
>>>>>>>>>>>>> 79 * deletedTagLock,
>>>>>>>>>>>>>     80  */
>>>>>>>>>>>>> 81 struct bag* deletedSignatureBag;
>>>>>>>>>>>>>
>>>>>>>>>>>>>     The comments contradict to each other.
>>>>>>>>>>>>>     I guess, the lock name at line 79 has to be
>>>>>>>>>>>>> deletedSignatureLock
>>>>>>>>>>>>> instead of deletedTagLock.
>>>>>>>>>>>>>     Also, comma at the end must be replaced with dot.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> 101 // Tag not found? Ignore.
>>>>>>>>>>>>> 102 if (klass == NULL) {
>>>>>>>>>>>>> 103 debugMonitorExit(deletedSignatureLock);
>>>>>>>>>>>>> 104 return;
>>>>>>>>>>>>> 105 }
>>>>>>>>>>>>>    106
>>>>>>>>>>>>> 107 // Scan linked-list.
>>>>>>>>>>>>> 108 jlong found_tag = klass->klass_tag;
>>>>>>>>>>>>> 109 while (klass != NULL && found_tag != tag) {
>>>>>>>>>>>>> 110 klass_ptr = &klass->next;
>>>>>>>>>>>>> 111 klass = *klass_ptr;
>>>>>>>>>>>>> 112 found_tag = klass->klass_tag;
>>>>>>>>>>>>>    113     }
>>>>>>>>>>>>> 114
>>>>>>>>>>>>> 115 // Tag not found? Ignore.
>>>>>>>>>>>>> 116 if (found_tag != tag) {
>>>>>>>>>>>>> 117 debugMonitorExit(deletedSignatureLock);
>>>>>>>>>>>>> 118 return;
>>>>>>>>>>>>>    119     }
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>    The code above can be simplified, so that the lines 101-105
>>>>>>>>>>>>> are not
>>>>>>>>>>>>> needed anymore.
>>>>>>>>>>>>>    It can be something like this:
>>>>>>>>>>>>>
>>>>>>>>>>>>> // Scan linked-list.
>>>>>>>>>>>>> while (klass != NULL && klass->klass_tag != tag) {
>>>>>>>>>>>>> klass_ptr = &klass->next;
>>>>>>>>>>>>> klass = *klass_ptr;
>>>>>>>>>>>>>        }
>>>>>>>>>>>>> if (klass == NULL || klass->klass_tag != tag) { // klass not
>>>>>>>>>>>>> found - ignore.
>>>>>>>>>>>>> debugMonitorExit(deletedSignatureLock);
>>>>>>>>>>>>> return;
>>>>>>>>>>>>>        }
>>>>>>>>>>>>>
>>>>>>>>>>>>> It will take more time when I get a chance to look at the
>>>>>>>>>>>>> rest.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>> Serguei
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 12/21/19 13:24, Roman Kennke wrote:
>>>>>>>>>>>>>> Here comes an update that resolves some races that happen
>>>>>>>>>>>>>> when
>>>>>>>>>>>>>> disconnecting an agent. In particular, we need to take the
>>>>>>>>>>>>>> lock on
>>>>>>>>>>>>>> basically every operation, and also need to check whether
>>>>>>>>>>>>>> or not
>>>>>>>>>>>>>> class-tracking is active and return an appropriate result
>>>>>>>>>>>>>> (e.g. an empty
>>>>>>>>>>>>>> list) when we're not.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Updated webrev:
>>>>>>>>>>>>>> http://cr.openjdk.java.net/~rkennke/JDK-8227269/webrev.04/
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>> Roman
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> So, here comes the O(1) implementation:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> - Whenever a class is 'prepared', it is registered with a
>>>>>>>>>>>>>>> tag, and we
>>>>>>>>>>>>>>> set-up a listener to get notified when it is unloaded.
>>>>>>>>>>>>>>> - Prepared classes are kept in a datastructure that is a
>>>>>>>>>>>>>>> table, which
>>>>>>>>>>>>>>> each entry being the head of a linked-list of KlassNode*.
>>>>>>>>>>>>>>> The
>>>>>>>>>>>>>>> table is
>>>>>>>>>>>>>>> indexed by tag % slot-count, and then simply prepend the new
>>>>>>>>>>>>>>> KlassNode*.
>>>>>>>>>>>>>>> This is O(1) operation.
>>>>>>>>>>>>>>> - When we get notified of unloading a class, we look up the
>>>>>>>>>>>>>>> signature of
>>>>>>>>>>>>>>> the reported tag in that table, and remember it in a bag.
>>>>>>>>>>>>>>> The
>>>>>>>>>>>>>>> KlassNode*
>>>>>>>>>>>>>>> is then unlinked from the table and deallocated. This is
>>>>>>>>>>>>>>> ~O(1) operation
>>>>>>>>>>>>>>> too, depending on the depth of the table. In my testcase
>>>>>>>>>>>>>>> which hammered
>>>>>>>>>>>>>>> the code with class-loads and unloads, I usually see depths
>>>>>>>>>>>>>>> of like 2-3,
>>>>>>>>>>>>>>> but not usually more. It should be ok.
>>>>>>>>>>>>>>> - when processUnloads() gets called, we simply hand out that
>>>>>>>>>>>>>>> bag, and
>>>>>>>>>>>>>>> allocate a new one.
>>>>>>>>>>>>>>> - I also added cleanup-code in classTrack_reset() to avoid
>>>>>>>>>>>>>>> leaking the
>>>>>>>>>>>>>>> signatures and KlassNode* etc when debug agent gets detached
>>>>>>>>>>>>>>> and/or
>>>>>>>>>>>>>>> re-attached (was missing before).
>>>>>>>>>>>>>>> - I also added locks around data-structure-manipulation (was
>>>>>>>>>>>>>>> missing
>>>>>>>>>>>>>>> before).
>>>>>>>>>>>>>>> - Also, I only activate this whole process when an actual
>>>>>>>>>>>>>>> listener gets
>>>>>>>>>>>>>>> registered on EI_GC_FINISH. This seems to happen right when
>>>>>>>>>>>>>>> attaching a
>>>>>>>>>>>>>>> jdb, not sure why jdb does that though. This may be
>>>>>>>>>>>>>>> something
>>>>>>>>>>>>>>> to improve
>>>>>>>>>>>>>>> in the future?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> In my tests, the performance of class-tracking itself looks
>>>>>>>>>>>>>>> really good.
>>>>>>>>>>>>>>> The bottleneck now is clearly actual synthesizing the
>>>>>>>>>>>>>>> class-unload
>>>>>>>>>>>>>>> events. I don't see how this can be helped when the debug
>>>>>>>>>>>>>>> agent asks for it?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Updated webrev:
>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~rkennke/JDK-8227269/webrev.03/
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Please let me know what you think of it.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>> Roman
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Alright, the perfectionist in me got me. I am implementing
>>>>>>>>>>>>>>>> the even more
>>>>>>>>>>>>>>>> efficient ~O(1) class tracking. Please hold off reviewing
>>>>>>>>>>>>>>>> for now.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Thanks,Roman
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>    Hi Chris,
>>>>>>>>>>>>>>>>>> I'll have a look at this, although it might not be for a
>>>>>>>>>>>>>>>>>> few days. In
>>>>>>>>>>>>>>>>>> the meantime, maybe you can describe your new
>>>>>>>>>>>>>>>>>> implementation in
>>>>>>>>>>>>>>>>>> classTrack.c so it's easier to look through the changes.
>>>>>>>>>>>>>>>>> Sure.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> The purpose of this class-tracking is to be able to
>>>>>>>>>>>>>>>>> determine the
>>>>>>>>>>>>>>>>> signatures of unloaded classes when GC/class-unloading
>>>>>>>>>>>>>>>>> happened, so that
>>>>>>>>>>>>>>>>> we can generate the appropriate JDWP event.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> The current implementation does so by maintaining a table
>>>>>>>>>>>>>>>>> of currently
>>>>>>>>>>>>>>>>> prepared classes by building that table when classTrack is
>>>>>>>>>>>>>>>>> initialized,
>>>>>>>>>>>>>>>>> and then add new classes whenever a class gets loaded.
>>>>>>>>>>>>>>>>> When
>>>>>>>>>>>>>>>>> unloading
>>>>>>>>>>>>>>>>> occurs, that cache is rebuilt into a new table, and
>>>>>>>>>>>>>>>>> compared with the
>>>>>>>>>>>>>>>>> old table, and whatever is in the old, but not in the new
>>>>>>>>>>>>>>>>> table gets
>>>>>>>>>>>>>>>>> returned. The problem is that when GCs happen frequently
>>>>>>>>>>>>>>>>> and/or many
>>>>>>>>>>>>>>>>> classes get loaded+unloaded, this amounts to
>>>>>>>>>>>>>>>>> O(classCount*gcCount)
>>>>>>>>>>>>>>>>> complexity.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> The new implementation keeps a linked-list of prepared
>>>>>>>>>>>>>>>>> classes, and also
>>>>>>>>>>>>>>>>> tracks unloads via the listener cbTrackingObjectFree().
>>>>>>>>>>>>>>>>> Whenever an
>>>>>>>>>>>>>>>>> unload/GC occurs, the list of prepared classes is scanned,
>>>>>>>>>>>>>>>>> and classes
>>>>>>>>>>>>>>>>> that are also in the deletedTagBag are unlinked (thus
>>>>>>>>>>>>>>>>> maintaining the
>>>>>>>>>>>>>>>>> prepared-classes-list) and its signature put in the list
>>>>>>>>>>>>>>>>> that gets returned.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> The implementation is not perfect. In order to determine
>>>>>>>>>>>>>>>>> whether or not
>>>>>>>>>>>>>>>>> a class is unloaded, it needs to scan the deletedTagBag.
>>>>>>>>>>>>>>>>> That process is
>>>>>>>>>>>>>>>>> therefore still O(unloadedClassCount). The assumption here
>>>>>>>>>>>>>>>>> is that
>>>>>>>>>>>>>>>>> unloadedClassCount << classCount. In my experiments this
>>>>>>>>>>>>>>>>> seems to be
>>>>>>>>>>>>>>>>> true, and also reasonable to expect.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> (I have some ideas how to improve the implementation to
>>>>>>>>>>>>>>>>> ~O(1) but it
>>>>>>>>>>>>>>>>> would be considerably more complex: have to maintain a
>>>>>>>>>>>>>>>>> (hash)table that
>>>>>>>>>>>>>>>>> maps tags -> KlassNode*, unlink them directly upon unload,
>>>>>>>>>>>>>>>>> and build the
>>>>>>>>>>>>>>>>> unloaded-signatures list there, but I don't currently see
>>>>>>>>>>>>>>>>> that it's
>>>>>>>>>>>>>>>>> worth the effort).
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> In addition to all that, this process is only activated
>>>>>>>>>>>>>>>>> when there's an
>>>>>>>>>>>>>>>>> actual listener registered for EI_GC_FINISH.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>> Roman
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Chris
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> On 12/18/19 5:05 AM, Roman Kennke wrote:
>>>>>>>>>>>>>>>>>>> Hello all,
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Issue:
>>>>>>>>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8227269
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> I am proposing what amounts to a rewrite of
>>>>>>>>>>>>>>>>>>> classTrack.c.
>>>>>>>>>>>>>>>>>>> It avoids
>>>>>>>>>>>>>>>>>>> throwing away the class cache on GC, and instead keeps
>>>>>>>>>>>>>>>>>>> track of
>>>>>>>>>>>>>>>>>>> loaded/unloaded classes one-by-one.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> In addition to that, it avoids this whole dance until an
>>>>>>>>>>>>>>>>>>> agent
>>>>>>>>>>>>>>>>>>> registers interest in EI_GC_FINISH.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Webrev:
>>>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~rkennke/JDK-8227269/webrev.01/
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Testing: manual testing of provided test scenarios and
>>>>>>>>>>>>>>>>>>> timing.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Eg with the testcase provided here:
>>>>>>>>>>>>>>>>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1751985
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> I am getting those numbers:
>>>>>>>>>>>>>>>>>>> unpatched: no debug: 84s with debug: 225s
>>>>>>>>>>>>>>>>>>> patched:   no debug: 85s with debug: 95s
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> I also tested successfully through jdk/submit repo
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Can I please get a review?
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>> Roman
>>>>>>>>>>>>>>>>>>>
> 
> 

Attachment: signature.asc
Description: OpenPGP digital signature

Reply via email to