RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Reingruber, Richard
richard.reingruber at sap.com
Wed Jul 22 20:18:23 UTC 2020
Hi Goetz,
> > 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.
Done.
> 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.
There is also ConnectionGraph::not_global_escape()
> 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?"
And what about has_ea_local_in_scope?
> matcher.cpp ok
> output.cpp:1071
> Please break the long line.
Done.
> 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.
I don't know and didn't find anything in a quick search.
> 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 :)
I know :) At least here it is wrong I'd say. "...has to be a not escaping obj..." sounds better
(hopefully not only to my german ears).
> 2805, 2929, 2946ff, break lines
> deoptimization.hpp
> 158, 174, 176 ... I would break lines too, but here you are in
> good company :)
Done.
> globals.hpp ok
> mutexLocker.h|cpp ok
> objectMonitor.cpp ok
> thread.cpp
> 2631 typo: sapfepont --> safepoint
Done.
> 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?
You mean, the test could hang in that loop for a couple of minutes? I don't
think so. There are cache coherence protocols in place which will invalidate
stale data very timely.
> 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
Found and fix typo at line 1369.
(Probably the cursor was on 1311 and your eyes on 1369 ;))
> 1640: setting local variable i triggers always deoptimization
> --> setting local variable i always triggers deoptimization
Fixed.
> 2176: dontinline_calee --> dontinline_callee
> 2510: poping --> popping ... but I'm not sure here.
Done.
> 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! ��
OMG! How come you know?! ;)
> 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).
Done.
> 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.
That seem's to be correct except "If Corona is over" isn't a relative sentence
but a conditional sentence, isn't it?
The general rule seems to be: the subordinate clause is separated with a comma
from a following main clause. No comma separation is needed if the subordinate
clause follows the main clause.
Thanks, that's a lesson I learned!
> I think the same holds for 'because', 'while' etc.
> E.g., jvmtiEnvBase.cpp:1313, jvmtiImpl.cpp:646ff,
> vframe_hp.hpp 104ff
Ok. I've removed quite a lot of the occurrances.
> 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
Are you referring to the following?
(from http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.6/src/hotspot/share/prims/jvmtiEnvBase.cpp.frames.html)
1326
1327 // If the frame is a compiled one, need to deoptimize it.
1328 if (vf->is_compiled_frame()) {
This line 1327 is preexisting.
> In many places, your comments read really
> well but some are quite abbreviated I think.
Yeah, but not only because I'm lazy... It is the style that I prefer and I think
it matches the surrounding code quite well.
> 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
I've added the 'A' and I'll try to use complete sentences in the future. The
telegram style has advantages, too, though ;)
Thanks!
Cheers, Richard.
-----Original Message-----
From: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
Sent: Freitag, 17. Juli 2020 14:31
To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Reingruber, Richard <richard.reingruber at sap.com>; serviceability-dev at openjdk.java.net; hotspot-compiler-dev at openjdk.java.net; hotspot-runtime-dev at openjdk.java.net
Subject: RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
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.
More information about the hotspot-compiler-dev
mailing list