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

David Holmes david.holmes at oracle.com
Tue Jun 23 05:52:31 UTC 2015


On 20/06/2015 3:55 AM, Bertrand Delsart wrote:
> Thanks David,
>
> Further trimming, keeping only the open questions and action items.
>
> In addition, I had to revert back some of what I have undone in webrev.01.
>
> Removing the %.S rule did not work as expected on all targets. gcc
> handles differently 's' and 'S' files, doing the preprocessing only for
> the 'S' extension. Hence I added back the %.S rules but without all the
> C/C++ related FLAGS. The %.S rule is the same as the %.s rules (except
> for the printed message) but gcc does something different thanks to the
> effective file extension.
>
> Incremental open diff:
>
> +# Copyright (c) 2003, 2015, Oracle and/or its affiliates. All rights
> reserved.
>   # DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>   #
>   # This code is free software; you can redistribute it and/or modify it
> @@ -170,6 +170,12 @@
>       $(QUIETLY) $(REMOVE_TARGET)
>       $(QUIETLY) $(AS.S) $(DEPFLAGS) -o $@ $< $(COMPILE_DONE)
>
> +# gcc applies preprocessing if the file extension is .S instead of .s
> +%.o: %.S
> +    @echo $(LOG_INFO) Preprocessing and assembling $<
> +    $(QUIETLY) $(REMOVE_TARGET)
> +    $(QUIETLY) $(AS.S) $(DEPFLAGS) -o $@ $< $(COMPILE_DONE)
> +
>   %.s: %.cpp
>       @echo $(LOG_INFO) Generating assembly for $<
>       $(QUIETLY) $(GENASM.CXX) -o $@ $<

Ok.

>
> On 19/06/2015 09:38, David Holmes wrote:
>>>> 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. :)
>
> This may explain why it is already broken in hs-rt for some closed
> platforms :-)
>
> Good news is that non PCH builds look OK on the non-closed platforms I
> tested before noticing the closed issue.
>
> Note sure the closed platforms are worth fixing in this CR since I saw
> errors in files that have not been touched for a long time. It probably
> means nobody is doing non-PCH builds for these platforms.

Nobody is but it has been a long standing item on my to-do list to fix it.

>>>> 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.
>
> I did add (or enhance) a few public APIs in a several classes, when it
> looked safe. However, there are calls that I'm reluctant to open with no
> control over who/when/how they would be used in production.
>
> I checked that set_mode_flags() is (currently) the only one I need in
> Arguments.
>
> Hence we are back to my initial question:
>  >> Are you willing to open that API to every hotspot component ?
>
> I'm reluctant to do it but I will not object. set_mode_flags is called
> from different locations as a way to guarantee some coherence (this is
> why I'm calling it). However, if opened widely, there is for instance
> the risk of someone thinking it can be used to safely to activate the
> compilers late during arguments marshalling. Past a given point, this is
> no longer safe.
>
> A "friend" declaration is not much better when it opens doors to a
> friend class used regularly in production (unless controlled by the team
> owning the opened class). However, this should not be the case for the
> friend declarations I added. First, they are limited to friend classes
> that are unused in openjdk and that are also empty in most of our
> deployed targets. In addition, Oracle's version of CodeCacheExtension is
> owned by runtime, meaning the friend class is part of the same team as
> the opened class. We cannot control what others might do with this
> privilege but this is the responsibility of whoever would implement
> another CodeCacheExtension.
>
> Knowing that, pick your choice: either open set_mode_flags (and maybe
> others later) or keep the friend declaration.

Keep your friends. :)

A consolidated webrev.02 might be in order - I think Coleen has only 
looked at .00

Thanks,
David

> Thanks,
>
> Bertrand.
>
>


More information about the hotspot-runtime-dev mailing list