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.
