RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents

[Lindenmaier, Goetz]

Hi Richard,

> I'll answer to the obvious things in this mail now.
> I'll go through the code thoroughly again and write
> a review of my findings thereafter.
As promised a detailed walk-throug, but without any major findings:

c1_IR.hpp: ok
ci_Env.h|cpp: ok
compiledMethod.cpp, nmethod.cpp: ok
debugInfoRec.h|cpp: ok
scopeDesc.h|cpp ok

compileBroker.h|cpp: 
Maybe a bit of documentation how and why you start 
the threads? I had expected there are two test
scenarios run after each other, but now I understand 'Single'
and 'All' run simultaneously.  Well, this really is a stress test!
Also good the two variants of depotimization are
stressed against each other.
Besides that really nice it's all in one place.

rootResolver.cpp: ok
jvmciCodeInstaller.cpp: ok

c2compiler.cpp: The essence of this change! Just one line :)
Great!

callnode.hpp ok
escape.h|cpp ok
macro.cpp 
I was not that happy with the names saying not_global_escape
and similar. I now agreed you have to use the terms of the escape
analysis (NoEscape ArgEscape= throughout the runtime code. I'm still not happy with 
the 'not' in the term, I always try to expand the name to some
sentence with a negated verb, but it makes no sense.
For example, "has_not_global_escape_in_scope" expands to 
"Hasn't a global escape in its scope." in my thinking, which makes 
no sense. You probably mean
"Has not-global escape in its scope." or "Has {ArgEscape|NoEscape} 
in its scope."

C2 is using the word "non" in this context, e.g., here 
alloc->is_non_escaping.

non obviously negates the adjective 'global',
non-global or nonglobal even is a English term I find in the 
net. 
So what about "has_non_global_escape_in_scope?"

matcher.cpp ok

output.cpp:1071
Please break the long line.

jvmtiCodeBlobEvents.cpp ok

jvmtiEnv.cpp
MaxJavaStackTraceDepth is only documented to affect
the exceptions stack trace depth, not to limit jvmti 
operations. Therefore I wondered why it is used here. 
Non of your business, but the flag should
document this in globals.hpp, too.  
Does jvmti specify that the same limits are used ...?
ok on your side.

jvmtiEnvBase.cpp  ok
jvmtiImpl.h|cpp  ok
jvmtiTagMap.cpp ok
whitebox.cpp ok

deoptimization.cpp

line 177: Please break line
line 246, 281: Please break line
1578, 1583, 1589, 1632, 1649, 1651 Break line

1651: You use 'non'-terms, too: non-escaping :)

2805, 2929, 2946ff, break lines

deoptimization.hpp

158, 174, 176 ... I would break lines too, but here you are in
good company :)

globals.hpp ok
mutexLocker.h|cpp ok
objectMonitor.cpp ok

thread.cpp 

2631 typo: sapfepont --> safepoint

thread.hpp ok
thread.inline.hpp ok
vframe.cpp ok
vframe_hp.cpp   458ff break lines
vframe_hp.hpp ok
macros.hpp ok
TEST.ROOT ok
WhiteBox.java ok

IterateHeapWithEscapeAnalysisEnabled.java

line 415:
msg("wait until target thread has set testMethod_result");
while (testMethod_result == 0) {
    Thread.sleep(50);
}
Might the test run into timeouts at this place?
The field is volatile, i.e. it will be reloaded
in each iteration. But will dontinline_testMethod
write it back to main memory in time?

libIterateHeapWithEscapeAnalysisEnabled.c ok

EATests.java

This is a very elaborate test.
I found a row of test cases illustrating issues
we talked about before. Really helpful!

1311: TypeO materialize -> materialized

1640: setting local variable i triggers always deoptimization
  --> setting local variable i always triggers deoptimization

2176: dontinline_calee --> dontinline_callee
2510: poping --> popping  ... but I'm not sure here.

https://www.urbandictionary.com/define.php?term=poping
poping
Drinking large amounts of Dextromethorphan Hydrobromide (DXM)based cough syrup, and then embarking on an adventure while wandering around neighborhoods or parks all night. This is usually done while listening to Punk rock music from a portable jambox. 
;)
Don’t do it! ��

EATestsJVMTI.java

I think you can just copy this test description into the other
test. You can have two @test comments, they will be treated
as separate tests.  The @requires will be evaluated accordingly.
For an example see 
test/hotspot/jtreg/runtime/exceptionMsgs/NullPointerException/NullPointerExceptionTest.java
which has two different compile setups for the test class (-g).

so, that's it for reading code ...


Some general remarks, maybe a bit picky ...:
I think you could use less commas ',' in comments.
As I understand, you need a comma if the relative
sentence is at the beginning, but not if it is at 
the end:
  If Corona is over, I go to the office.
but
  I go to the office if Corona is over.
I think the same holds for 'because', 'while' etc.
E.g., jvmtiEnvBase.cpp:1313, jvmtiImpl.cpp:646ff, 
vframe_hp.hpp 104ff

Also, I like full sentences in comments.  
Especially for me as foreign speaker, this makes
things much more clear. I.e., I try to make it
a real sentence with articles, capitalized and a
dot at the end if there is a subject and a verb
in first place.
E.g., jvmtiEnvBase.cpp:1327
In many places, your comments read really 
well but some are quite abbreviated I think.

E.g. thread.cpp:2601 is an example where a simple
'a' helps a lot.
"Single deoptimization is typically very short."
I would add 'A': "A single deoptimization is typically very short (fast?)."
An other meaning of the comment I first considered is this:
"Single deoptimization is typically very short, all_threads deoptimization takes longer"
having in mind the functions
EscapeBarries::deoptimize_objects_all_threads()  
and 
EscapeBarries::deoptimize_objects() doing a single thread.
German with it's compound nouns is helpful here :)

Einzeldeoptimierung <--> eine einzelne Deoptimierung

Best regards,
  Goetz.