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, [email protected] wrote: >>>>>>>>> Sorry, forgot to complete my comments at the end (see below). >>>>>>>>> >>>>>>>>> >>>>>>>>> On 3/15/20 23:57, [email protected] 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 >>>>>>>>>>>>>>>>>>> > >
signature.asc
Description: OpenPGP digital signature
