On Sat, 3 Sep 2022 09:47:05 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
> Also, I'm curious how did you verify that no regressions have been introduced?
Run all Redefine/Retransform Classes tests:
test/jdk/java/lang/instrument
test/hotspot/jtreg/serviceability/jvmti/RedefineClasses
test/hotspot/jtreg/vmTestbase/nsk/jvmti/RedefineClasses
test/hotspot/jtreg/vmTestbase/nsk/jvmti/RetransformClasses
> test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineRetransform/RedefineRetransform.java
> line 76:
>
>> 74: private static byte[] initialClassBytes;
>> 75:
>> 76: private static class VersionScanner extends ClassVisitor {
>
> This class needs some explanation.
> It is unclear how does it work.
> Could you, please, add relevant comments where needed?
> For instance, how are the class file bytes constructed and what role does the
> version play.
Done.
> test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineRetransform/RedefineRetransform.java
> line 236:
>
>> 234: redefine(2, 5); // updates cached class bytes
>> 235: retransform(3, 2);
>> 236: retransform(4, 2);
>
> The comments for all test cases need to be aligned.
> Also, it is better to comment all redefine/retransform cases.
Done.
> test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineRetransform/libRedefineRetransform.cpp
> line 179:
>
>> 177: if (err != JVMTI_ERROR_NONE) {
>> 178: _log("nRedefine: SetEventNotificationMode(JVMTI_DISABLE) error
>> %d\n", err);
>> 179: }
>
> It makes sense to introduce an utility function which does
> SetEventNotificationMode.
> It can be two separate functions to enable and disable or one single function
> can support both cases.
Removed code duplication - now the code is in helper class
> test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineRetransform/libRedefineRetransform.cpp
> line 195:
>
>> 193: _log("nRedefine: classLoadHookSavedClassBytes is NULL\n");
>> 194: return nullptr;
>> 195: }
>
> The checks 181-195 seems to be the same in functions nRedefine and
> nRetransform,
> so one utility function can be used in both cases.
Removed code duplication - now the code is in helper class
> test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineRetransform/libRedefineRetransform.cpp
> line 274:
>
>> 272: classLoadHookSavedClassBytes = nullptr;
>> 273:
>> 274: return result;
>
> The fragments 197-214 and 256-274 do the same.
> I'd suggest to define and use a function that does this post-processing.
reworked the code a bit, moved all common code to helper class
-------------
PR: https://git.openjdk.org/jdk/pull/10032
