RFR: 8227269: Slow class loading when running JVM in debug mode
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Tue Mar 24 21:50:25 UTC 2020
On 3/24/20 14:47, Chris Plummer wrote:
> On 3/24/20 2:46 PM, serguei.spitsyn at oracle.com wrote:
>> On 3/24/20 14:39, Chris Plummer wrote:
>>> On 3/24/20 1:45 PM, Roman Kennke wrote:
>>>>>>> I assume JVMTI maintains separate tagging data for each agent so
>>>>>>> having
>>>>>>> two agents doing tagging won't result in confusion. I didn't
>>>>>>> actually
>>>>>>> find this in the spec. Would be nice to confirm that it is the
>>>>>>> case.
>>>>>>> However, your implementation does seem to conflict with other
>>>>>>> uses of
>>>>>>> tagging in the debug agent:
>>>>>> The tagging data is per-jvmtiEnv. We create and use our own env
>>>>>> (private
>>>>>> to class-tracking), so this wouldn't conflict with other uses of
>>>>>> tags.
>>>>>> Could it be a problem that we have a single trackingEnv per JVM,
>>>>>> though?
>>>>>> /me scratches head.
>>>>> Ok. This is an area I'm not familiar with, but the spec does say:
>>>>>
>>>>> "Each call to GetEnv creates a new JVM TI connection and thus a
>>>>> new JVM
>>>>> TI environment."
>>>>>
>>>>> So it looks like what you are doing should be ok. I still think
>>>>> you have
>>>>> a bug where you are not deallocating signatures of classes that are
>>>>> unloaded. If you think otherwise please point out where this is done.
>>>> Signatures that make it out of processUnloading() are deallocated in
>>>> eventHandler.c, in synthesizeUnload(), right after it has been used.
>>>>
>>>> http://hg.openjdk.java.net/jdk/jdk/file/b9562cc25fc0/src/jdk.jdwp.agent/share/native/libjdwp/eventHandler.c#l527
>>>>
>>> Ok. Good to know. Not the best of designs, but that's not your
>>> fault. I'll make another pass over the changes, but I think in
>>> general it looks good. I don't think I've seen another reviewer yet,
>>> so hopefully someone jumps in.
>>
>> As I understand, Roman already resolved my previous comments.
>> So, I will do another pass for v6.
> I think it's pretty much a rewrite since you last reviewed it.
Yes, I'm expecting this as some performance related issues were discovered.
Thanks,
Serguei
>
> Chris
>>
>> Thanks,
>> Serguei
>>
>>>
>>> Chris
>>>> Pending signatures on debug-agent-disconnect are deallocated in
>>>> classTrack.c, in the reset() routine.
>>>>
>>>> Thanks,
>>>> Roman
>>>>
>>>>> thanks,
>>>>>
>>>>> Chris
>>>>>>> What would cause classTrack_addPreparedClass() to be called for
>>>>>>> a Class
>>>>>>> you've already seen? I don't understand the need for the "tag !=
>>>>>>> 0l"
>>>>>>> check.
>>>>>> It's probably not needed, may be a left-over from previous
>>>>>> installments
>>>>>> of this implementation. I will check it, and turn into an assert
>>>>>> or so.
>>>>>>
>>>>>> Thanks,
>>>>>> Roman
>>>>>>
>>>>>>> thanks,
>>>>>>>
>>>>>>> Chris
>>>>>>>
>>>>>>> On 3/20/20 12:52 PM, Chris Plummer wrote:
>>>>>>>> On 3/20/20 8:30 AM, 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/
>>>>>>>> I'll have a look at this.
>>>>>>>>> (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.)
>>>>>>>> At least this is only a one-shot hit when the classes are
>>>>>>>> unloaded,
>>>>>>>> and the performance hit is based on the number of classes being
>>>>>>>> unloaded. The main issue is happening every GC, and is O(n)
>>>>>>>> where n is
>>>>>>>> the number of loaded classes.
>>>>>>>>> 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
>>>>>>>>>
>>>>>>>> This is JDI, not jdb. It looks like it needs ClassUnload events
>>>>>>>> so it
>>>>>>>> can maintain typesBySignature, which is used by public APIs like
>>>>>>>> allClasses(). So we have caching of loaded classes both in the
>>>>>>>> debug
>>>>>>>> agent and in JDI.
>>>>>>>>
>>>>>>>> Chris
>>>>>>>>> But this is not in the scope of this bug.)
>>>>>>>>>
>>>>>>>>> Roman
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 3/16/20 8:05 AM, serguei.spitsyn at oracle.com wrote:
>>>>>>>>>> Sorry, forgot to complete my comments at the end (see below).
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 3/15/20 23:57, serguei.spitsyn at 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
>>>>>>>>>>>>>>>>>>>>
>>>>>
>>>
>>>
>>
>
>
More information about the serviceability-dev
mailing list