RFR [M] : 8087333, Optionally Pre-Generate the HotSpot Template Interpreter

[David Holmes]

Hi Bertrand,

On 19/06/2015 2:49 AM, Bertrand Delsart wrote:
> Thanks David,
>
> Updated webrev and incremental webrev:
>
> http://cr.openjdk.java.net/~bdelsart/8087333/webrev.01/webrev/
>
> http://cr.openjdk.java.net/~bdelsart/8087333/webrev.00-01/webrev/
>
> Changes already performed, in short:
> - removed .S support, will add my flags in ASFLAGS and use %.s rules
> - a few changes you suggested
> - continue returning abstract_method_handler instead of NULL
>    but replaced it with a handler that will always cause a VM
>    error if used.

Changes as outlined seem fine. Thanks.

> More comments inlined.

A couple of minor follow ups below with other stuff trimmed out ...

>> src/share/vm/code/codeCache.cpp
>>
>> Does _heaps->length() >= 1 imply SegmentedCodeCache==true ?
>
> No. (but SegmentedCodeCache==true implies _heaps->length() >= 1)

Okay I got that reversed in my head - the main thing is that new 
condition will always be true (in the absence of a pregenerated 
interpreter) when the old condition would be true.

>> src/share/vm/interpreter/templateInterpreter.cpp
>>
>> In:
>>
>> 240 void TemplateInterpreterGenerator::generate_all() {
>>   241   // Loop, in case we need several variants of the interpreter
>> entries
>>   242   do {
>>   243     if (!CodeCacheExtensions::skip_code_generation()) {
>>
>> Can skip_code_generation return different results for different
>> iterations?
>
> I do not need to do it for the pregenerated interpreter extension. For a
> given run of the JVM, I always return the same value, computed at startup.
>
> Note that this is just an optimization, avoiding to waste away space
> from the dynamic code heap (and the time needed for generating unused
> code). I'm not sure of what your concern is but if someone needs not to
> skip some of the code then the fallback solution is not to skip anything
> at all.

Given that condition guards the entire body of the loop I was wondering 
whether it could go outside the loop, or whether it potentially changed 
and so had to be inside. It just looked odd to me to both add the loop 
and the guard the way you did. :)

>> src/share/vm/precompiled/precompiled.hpp
>>
>> Seems okay - but of course begs the question whether this has been
>> checked with PCH disabled?
>
> It has been tested but maybe not for the latest version. It does pass
> JPRT but I'm not sure of whether JPRT performs both PCH and non-PCH
> builds (if not, it should :-))

Yes it should but it doesn't. :)

>> src/share/vm/runtime/arguments.hpp
>>
>> I'm getting doubtful about the numerous, and varied "friend"
>> declarations. Why should CodeCacheExtensions need anything but the
>> public API of Arguments ?
>
> Because CodeCacheExtensions may include ergonomics, some of them being
> more complex than just changing one flag.
>
> As an example, my extension may have to force interpreter mode and the
> cleanest and most robust way of doing it is through a non public API:
>
>      Arguments::set_mode_flags(Arguments::_int);
>
> Are you willing to open that API to every hotspot component ?
>
> There is always a trade-off between opening a few calls to everybody and
> opening all of them to selected friends we can watch closely. I'd rather
> keep the friend declaration but I'm biased since it makes it way easier
> for me :-)

I guess I just don't like "secret societies" :) Whenever a friend gets 
introduced it always begs the questions: does this class really need to 
access internals? If so, does that suggest the need for a more extensive 
public API? I didn't (and still don't) have enough context to answer 
that here.

>> src/share/vm/runtime/sharedRuntime.cpp
>>
>> This change looks to me like something better handled in the callers
>> that always expect to find a handler, rather than just returning
>> something non-NULL.
>
> If I remember correctly, returning NULL would be intrusive because it
> was leading to issues in different places. They would all have to be
> modified so as to take into account
> CodeCacheExtensions::skip_compiler_support().
>
> The cleanest fix would be not to call this method in -Xint mode but
> unfortunately HotSpot performs a lot of stuff for C1 and C2 even in
> -Xint mode. Fixing that is outside the scope of this CR. This is why I
> instead added CodeCacheExtensions::skip_compiler_support(), focusing on
> the issues this created when combined with my extensions and trying to
> find simple non intrusive fixes.
>
> Now, your concern may be that I'm returning _abstract_method_handler,
> which is usually supposed to mean something else. However, with
> CodeCacheExtensions::skip_compiler_support(), note whatever I decide to
> return here is the only handler ever returned. In particular, we never
> need the real abstract_method_handler for another purpose.
>
> To alleviate your concern, I've implemented what I was suggesting at the
> end of my comment: modify this unique handler so that it is no longer
> related to abstract methods but instead causes a VM error if used.

Okay - thanks for doing that.


Thanks,
David