On Wed, 7 Oct 2020 04:28:16 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> Hi Serguei
>> (@sspitsyn)
>>
>> are you ok with the changes I made based on your comments?
>> Will you further review the change?
>>
>> Thanks, Richard.
>
> Hi Richard,
>
> Thank you for making the refactoring. I like it more now. :)
> So, the fix looks good to me in general.
>
> But could I ask you, to adjust some formatting, please?
> There are several things that can be done to improve the code readability.
>
> src/hotspot/share/prims/jvmtiDeferredUpdates.hpp:
>
> I'd suggest to add an empty line before lines 40, 71, 73, 93, 95, 109 to make
> class definitions and function
> declarations/definitions with comments more catchable by eyes. The following
> lines can be removed: 81, 82, 103
>
> Also, there is inconsistency in function definitions formatting:
> - some functions have big indent between the type and name
> - some functions have no indent between the type and name but a big indent
> between name and body
> I'd suggest to either to remove all indents or make it reasonably smaller but
> consistent.
>
> It seems, there is no reason to keep these class declarations:
> 38 class jvmtiDeferredLocalVariable;
> 108 class jvmtiDeferredLocalVariableSet;
>
> src/hotspot/share/prims/jvmtiDeferredUpdates.cpp:
>
> 82 // Free deferred updates.
> 83 // (Note the 'list' of local variable updates is embedded in
> 'updates')
>
> A suggestion to change the line 83 as follows:
> ` 83 // Note, the 'list' of local variable updates is embedded in
> 'updates'.`
>
> src/hotspot/share/runtime/escapeBarrier.hpp:
>
> Add dots at the end of comments at lines 97, 99, 103.
> I'd suggest to add an empty line before lines 39, 40, 80, 81, 93, 94, 99,
> 119, 121.
>
> src/hotspot/share/runtime/escapeBarrier.cpp:
>
> The following class declaration is not needed:
> ` 49 class jvmtiDeferredLocalVariableSet;`
>
> because you already added this line:
> ` 29 #include "prims/jvmtiDeferredUpdates.hpp"`
>
> The lines below deserve a refactoring. It can be separate functions for
> locals, expressions and monitors, or just one
> function for the whole fragment:
> 345 GrowableArray* scopeLocals = cvf->scope()->locals();
> 346 StackValueCollection* locals = cvf->locals();
> 347 if (locals != NULL) {
> 348 for (int i2 = 0; i2 < locals->size(); i2++) {
> 349 StackValue* var = locals->at(i2);
> 350 if (var->type() == T_OBJECT &&
> scopeLocals->at(i2)->is_object()) {
> 351 jvalue val;
> 352 val.l = cast_from_oop(locals->at(i2)->get_obj()());
> 353 cvf->update_local(T_OBJECT, i2, val);
> 354 }
> 355 }
> 356 }
> 357
> 358 // expressions
> 359 GrowableArray* scopeExpressions = cvf->scope()->expressions();
> 360 StackValueCollection* expressions = cvf->expressions();
> 361 if (expressions != NULL) {
> 362 for (int i2 = 0; i2 < expressions->size(); i2++) {
> 363 StackValue* var = expressions->at(i2);
> 364 if (var->type() == T_OBJECT &&
> scopeExpressions->at(i2)->is_object()) {
> 365 jvalue val;
> 366 val.l = cast_from_oop(expressions->at(i2)->get_obj()());
> 367 cvf->update_stack(T_OBJECT, i2, val);
> 368 }
> 369 }
> 370 }
> 371
> 372 // monitors
> 373 GrowableArray* monitors = cvf->monitors();
> 374 if (monitors != NULL) {
> 375 for (int i2 = 0; i2 < monitors->length(); i2++) {
> 376 if (monitors->at(i2)->eliminated()) {
> 377 assert(!monitors->at(i2)->owner_is_scalar_replaced(),
> 378 "reallocation failure, should not update");
> 379 cvf->update_monitor(i2, monitors->at(i2));
> 380 }
> 381 }
> 382 }
>
>
> src/hotspot/share/prims/jvmtiImpl.cpp:
>
> 420 // Constructor for non-object getter
> 421 VM_GetOrSetLocal::VM_GetOrSetLocal(JavaThread* thread, jint depth, jint
> index, BasicType type)
> 422 : _thread(thread)
> 423 , _calling_thread(NULL)
> 424 , _depth(depth)
> 425 , _index(index)
> 426 , _type(type)
> 427 , _jvf(NULL)
> 428 , _set(false)
> 429 , _eb(type == T_OBJECT, NULL, NULL)
> 430 , _result(JVMTI_ERROR_NONE)
> 431 {
> 432 }
>
> I still think, that the line 429 is going to cause confusions.
> It is a non-object getter, so the type should never be T_OBJECT.
> It won't change in the future to allow the T_OBJECT types.
> The only way to allow it is to merge the constructors for object and
> non-object getters.
> So, I'm suggesting to replace this line with:
> ` 429 , _eb(false, NULL, NULL)`
Hi Serguei,
> Thank you for making the refactoring. I like it more now. :)
> So, the fix looks good to me in general.
Good :)
> But could I ask you, to adjust some formatting, please?
> There are several things that can be done to improve the code readability.
>
> src/hotspot/share/prims/jvmtiDeferredUpdates.hpp:
>
> I'd suggest to add an empty line before lines 40, 71, 73, 93, 95, 109 to make
> class definitions and function
> declarations/definitions with comments more catchable by eyes. The following
> lines can be removed: 81, 82, 103
Sure. I've made the changes.
> Also, there is inconsistency in function definitions formatting:
>
> * some functions have big indent between the type and name
>
> * some functions have no indent between the type and name but a big
> indent between name and body
> I'd suggest to either to remove all indents or make it reasonably
> smaller but consistent.
>
I've made the indents smaller. I also moved private members
jvmtiDeferredLocalVariable at the beginning. Looks better now.
> It seems, there is no reason to keep these class declarations:
>
> ```
> 38 class jvmtiDeferredLocalVariable;
> 108 class jvmtiDeferredLocalVariableSet;
> ```
Removed.
> src/hotspot/share/prims/jvmtiDeferredUpdates.cpp:
>
> ```
> 82 // Free deferred updates.
> 83 // (Note the 'list' of local variable updates is embedded in
> 'updates')
> ```
>
> A suggestion to change the line 83 as follows:
> ` 83 // Note, the 'list' of local variable updates is embedded in 'updates'.`
Done.
> src/hotspot/share/runtime/escapeBarrier.hpp:
>
> Add dots at the end of comments at lines 97, 99, 103.
> I'd suggest to add an empty line before lines 39, 40, 80, 81, 93, 94, 99,
> 119, 121.
Done.
> src/hotspot/share/runtime/escapeBarrier.cpp:
>
> The following class declaration is not needed:
> ` 49 class jvmtiDeferredLocalVariableSet;`
>
> because you already added this line:
> ` 29 #include "prims/jvmtiDeferredUpdates.hpp"`
Your right. Thanks.
> The lines below deserve a refactoring. It can be separate functions for
> locals, expressions and monitors, or just one
> function for the whole fragment:
> ```
> 345 GrowableArray* scopeLocals = cvf->scope()->locals();
> 346 StackValueCollection* locals = cvf->locals();
> 347 if (locals != NULL) {
> 348 for (int i2 = 0; i2 < locals->size(); i2++) {
> 349 StackValue* var = locals->at(i2);
> 350 if (var->type() == T_OBJECT &&
> scopeLocals->at(i2)->is_object()) {
> 351 jvalue val;
> 352 val.l = cast_from_oop(locals->at(i2)->get_obj()());
> 353 cvf->update_local(T_OBJECT, i2, val);
> 354 }
> 355 }
> 356 }
> 357
> 358 // expressions
> 359 GrowableArray* scopeExpressions = cvf->scope()->expressions();
> 360 StackValueCollection* expressions = cvf->expressions();
> 361 if (expressions != NULL) {
> 362 for (int i2 = 0; i2 < expressions->size(); i2++) {
> 363 StackValue* var = expressions->at(i2);
> 364 if (var->type() == T_OBJECT &&
> scopeExpressions->at(i2)->is_object()) {
> 365 jvalue val;
> 366 val.l = cast_from_oop(expressions->at(i2)->get_obj()());
> 367 cvf->update_stack(T_OBJECT, i2, val);
> 368 }
> 369 }
> 370 }
> 371
> 372 // monitors
> 373 GrowableArray* monitors = cvf->monitors();
> 374 if (monitors != NULL) {
> 375 for (int i2 = 0; i2 < monitors->length(); i2++) {
> 376 if (monitors->at(i2)->eliminated()) {
> 377 assert(!monitors->at(i2)->owner_is_scalar_replaced(),
> 378 "reallocation failure, should not update");
> 379 cvf->update_monitor(i2, monitors->at(i2));
> 380 }
> 381 }
> 382 }
> ```
I moved the fragment into a new method in compiledVFrame.
Please note that an equal fragment exists here too:
https://github.com/openjdk/jdk/blob/1e8e543b264bb985bfee535fedc9ffe7db5ad482/src/hotspot/share/jvmci/jvmciCompilerToVM.cpp#L1524-L1558
Actually this location could be implemented on top of EscapeBarrier. Maybe
(maybe not?) in a follow-up...
> src/hotspot/share/prims/jvmtiImpl.cpp:
>
> ```
> 420 // Constructor for non-object getter
> 421 VM_GetOrSetLocal::VM_GetOrSetLocal(JavaThread* thread, jint depth, jint
> index, BasicType type)
> 422 : _thread(thread)
> 423 , _calling_thread(NULL)
> 424 , _depth(depth)
> 425 , _index(index)
> 426 , _type(type)
> 427 , _jvf(NULL)
> 428 , _set(false)
> 429 , _eb(type == T_OBJECT, NULL, NULL)
> 430 , _result(JVMTI_ERROR_NONE)
> 431 {
> 432 }
> ```
>
> I still think, that the line 429 is going to cause confusions.
> It is a non-object getter, so the type should never be T_OBJECT.
> It won't change in the future to allow the T_OBJECT types.
> The only way to allow it is to merge the constructors for object and
> non-object getters.
> So, I'm suggesting to replace this line with:
> ` 429 , _eb(false, NULL, NULL)`
Ok, done.
-------------
PR: https://git.openjdk.java.net/jdk/pull/119
