RFR [M] : 8087333, Optionally Pre-Generate the HotSpot Template Interpreter
Bertrand Delsart
bertrand.delsart at oracle.com
Thu Jun 18 16:49:10 UTC 2015
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.
More comments inlined.
On 18/06/2015 06:48, David Holmes wrote:
> Hi Bertrand,
>
> I can't comment on all the details of this. Mostly it's a question of
> determining if the existing code is being modified based on the default
> definition of the CodeCacheExtensions class.
>
> On 13/06/2015 3:30 AM, Bertrand Delsart wrote:
>> Hi all,
>>
>> This webrev is related to a closed JEP. Its main objective is to
>> support the template interpreter in highly secure contexts where
>> dynamically generated code is not allowed.
>>
>> This will be pushed through hs-rt.
>>
>> https://bugs.openjdk.java.net/browse/JDK-8087333
>>
>> http://cr.openjdk.java.net/~bdelsart/8087333/webrev.00/webrev/
>
> agent/src/share/classes/sun/jvm/hotspot/code/CodeCache.java
> agent/src/share/classes/sun/jvm/hotspot/memory/CodeHeap.java
> make/linux/makefiles/buildtree.make
>
> Seems ok.
>
> ---
>
> make/linux/makefiles/rules.make
>
> $(QUIETLY) $(AS.S) $(DEPFLAGS) $(CPPFLAGS) $(CXXFLAGS) $(CFLAGS) -o $@
> $< $(COMPILE_DONE)
>
> CPPFLAGS no longer exists. If you specify CXXFLAGS and CFLAGS you will
> get a number of things specified twice - don't know if that is an issue,
> but it looks very odd to be passing C/C++ compiler flags to an assembler
> especially when:
>
> AS.S = $(AS) $(ASFLAGS)
>
> so ASFLAGS have already been applied. I'm thinking "CPPFLAGS" was really
> intended to be c-preprocessor flags, which are now folded in to the
> CXXFLAGS and CFLAGS.
ASFLAGS are used to compile our current .s files. They currently consist
only of flags specifying the target platforms.
The new rule was for my pregenerated assembly files, which require
preprocessing and a few -D options.
Now, the pregenerated additional flags should be harmless for the other
assembler files. Adding what I need to ASFLAGS seems to work. This may
be cleaner even if it impacts the command line used for all assembly files.
>
> ---
>
> make/linux/makefiles/vm.make
>
> Seems ok.
>
> ---
>
> src/share/vm/asm/codeBuffer.hpp
>
> Are there any concurrency issues with making _name mutable? (volatile?,
> release-store?)
Note that the value read by other threads does not really matter. The
name is just more explicit than the old one and the only thread that
needs to see this explicit name is the one that called set_name (value
is needed for the CodeCacheExtensions::handle_generated_handler call).
Hence, there is no concurrency risk on access.
The only other risk would be on deletion. However, the names are not
deleted explicitly (e.g. the thread calling set_name need not free the
previous value). The names are either C++ string constants or resource
allocated strings deleted implicitly shortly after being done with the
CodeBuffer. This holds for the set_name call I added in
SignatureHandlerLibrary::add, which is resource allocated.
>
> ---
>
> src/share/vm/c1/c1_Runtime1.cpp
>
> If you initialize here:
>
> 187 OopMapSet* oop_maps;
> 188 int frame_size;
> 189 bool must_gc_arguments;
>
> then you don't need:
>
> 231 } else {
> 232 /* ignored values */
> 233 oop_maps = NULL;
> 234 frame_size = 0;
> 235 must_gc_arguments = false;
I used the same pattern in a lot of places.
This was to avoid uselessly setting the values to 0 when a real value is
computed (e.g. in the other branch), avoiding useless overheads. In
addition, this may also help detect when the other branch changes and
someone inadvertently forgets to set all these values.
Now, if you think the gains mentioned above are not worth it and if you
really do not like my pattern, please confirm that I should modify all
the source files where I used that pattern.
>
> ---
>
> src/share/vm/code/codeBlob.cpp
> src/share/vm/code/codeBlob.hpp
>
> Seems okay
>
> src/share/vm/code/codeCache.cpp
>
> Does _heaps->length() >= 1 imply SegmentedCodeCache==true ?
No. (but SegmentedCodeCache==true implies _heaps->length() >= 1)
The SegmentedCodeCache is a split of a single contiguous code space in
smaller CodeHeap objects. Its use may be modified by ergonomics (based
on -Xint flag, code heap size, ...)
The PregeneratedCodeHeap is in a separate memory space and will always
exist when pregenerated code is used, independently of SegmentedCodeCache.
The support of multiple CodeHeaps, which was introduced for the
SegmentedCodeCache, nearly works out of the box for arbitrary CodeHeaps,
including PregeneratedCodeHeap. One of the minor issues is that we may
now have multiple code heaps even with -XX:-SegmentedCodeCache (when
using pregenerated code).
The code I modified is where what matters is the fact that there are
several CodeHeaps, not the fact that the default one is segmented (e.g.
mostly within FOR_ALL_HEAPS loops).
The change related to PrintCodeCacheExtension in CodeCache::allocate is
more arguable... not because of my change but because the initial code
looked strange :-) The test was IMHO useless because, unless I am
mistaken, heap->name() for the default heap is "CodeCache" when
SegmentedCodeCache is false (see CodeCache::initialize). I can either
leave that code as is (with SegmentedCodeCache check) because it does
not impact my extension, keep my change (using the fact that there are
several heaps as a good indication that their name matters) or
completely remove the test, always printing "heap->name()", assuming it
would have been set to whatever should be printed. This does not change
anything for the short term, this is just about whatever looks cleaner
with respect to future CodeHeap enhancements.
>
> src/share/vm/code/codeCache.hpp
>
> Seems ok.
>
> src/share/vm/code/stubs.cpp
>
> Can't comment.
>
> src/share/vm/code/stubs.hpp
> src/share/vm/interpreter/interpreter.cpp
>
> Seems ok
>
> src/share/vm/interpreter/interpreter.hpp
>
> Suggest "// possible extension" rather than "closed extension"
OK.
> src/share/vm/interpreter/interpreterRuntime.cpp
>
> Can't comment
>
> src/share/vm/interpreter/interpreterRuntime.hpp
>
> Seems ok.
>
> 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.
>
> ---
>
> src/share/vm/interpreter/templateInterpreter.hpp
>
> Seems ok.
>
> src/share/vm/memory/heap.hpp
>
> Seems ok - though odd to be adding virtual to such seemingly basic
> functions. The indent needs fixing up in a couple of places (inline tabs
> by the look of it).
Was not able to properly configure emacs on the machine where I do the
edits (indent-tabs-mode is nil but tabs are used anyway when I indent a
whole region instead of line by line). I currently have to hunt for
these tabs just before my pushes :-(
>
> ---
>
> src/share/vm/memory/virtualspace.cpp
>
> The expression:
>
> CodeCacheExtensions::support_dynamic_code() ? true : false
>
> is just:
>
> CodeCacheExtensions::support_dynamic_code()
Good catch.
>
> --
>
> 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 :-))
Anyway, I'll double-check before the push.
>
> src/share/vm/prims/methodHandles.cpp
> src/share/vm/runtime/arguments.cpp
>
> Seems okay.
>
> 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 :-)
>
> ---
> src/share/vm/runtime/init.cpp
>
> The complete_step mechanics seem somewhat intrusive. This would probably
> look better as a generic initialization/life-cycle hook mechanism,
> rather than being CodeCacheExtensions specific. Though a future RFE for
> that would be okay.
>
> ---
>
> 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.
>
> ---
>
> src/share/vm/runtime/stubCodeGenerator.cpp
> src/share/vm/runtime/stubCodeGenerator.hpp
> src/share/vm/runtime/stubRoutines.cpp
> src/share/vm/runtime/thread.cpp
> src/share/vm/runtime/vmStructs.cpp
> src/share/vm/runtime/vm_operations.cpp
> src/share/vm/runtime/vm_version.cpp
> src/share/vm/utilities/globalDefinitions.hpp
> src/share/vm/code/codeCacheExtensions.hpp
>
> Seems ok.
>
> ---
>
> src/share/vm/code/codeCacheExtensions_ext.hpp
>
> 39 // All the methods defined here are placeholders for proprietary
> extensions.
>
> proprietary -> optional | potential | possible
OK (selected "possible")
>
> ---
>
> src/share/vm/runtime/vmStructs_ext.hpp
>
> Seems ok.
>
> Thanks,
>
> David
> -----
>
>
>
>> This is a forward port to 9 of a feature that was in our embedded
>> workspace. That workspace was used to build both our regular embedded
>> binaries and pregenerated-enabled ones. As such, the 8 changes were
>> already mature and, even more important, did not lead to regressions on
>> platforms not using them. Similarly, no regression has been noticed on
>> the 9 forward and the changes pass JPRT (including the openJDK builds
>> and tests).
>>
>> The above webrev may not apply as is to jdk9/hs-rt since it comes on top
>> of other CRs concurrently being reviewed (the JEP was split into smaller
>> easier to review parts). The corresponding changesets are:
>> http://cr.openjdk.java.net/~bdelsart/8079473/
>> "allow demangling to be optional in dll_address_to_function_name"
>> (approved, not yet pushed)
>> http://cr.openjdk.java.net/~bdelsart/8081406/
>> "cleanup and minor extensions of the debugging facilities in
>> CodeStrings"
>> (waiting for reviewers)
>> http://cr.openjdk.java.net/~bdelsart/8087133/
>> "Improve sharing of native wrappers in SignatureHandlerLibrary"
>> (waiting for reviewers)
>> http://cr.openjdk.java.net/~bdelsart/8030076
>> "remove unused runtime related code"
>> (waiting for reviewers)
>>
>> Brief overview of the shared code changes for this webrev:
>>
>> - Minimum changes in agent code to support CodeHeap subclasses,
>> with different visitors, necessary for the pregenerated heap
>> Latest version is generic, allowing to define different CodeHeap
>> layouts for future extensions
>>
>> Validated with "search codebase" CLHSDB agent command, which
>> correctly parses both the pregenerated and the regular heap.
>>
>> - Build extension for the hotspot subtree, to support closed
>> extensions
>> * an extra generated 'extensions' directory
>> * ability to compile the *.S files, forcing the preprocessor to
>> be used before the assembler (used to convert pregenerated code
>> to *.o files)
>> * extra *.o files added into libjvm
>> * ability to extend the content of vm.def
>>
>> - Source code support for a new CodeCacheExtensions API, which
>> allows to load a pregenerated template interpreter (+all
>> stubs required for the interpreter to work). The same API
>> is also used to pregenerate this code.
>>
>> See hotspot/src/share/vm/code/codeCacheExtensions_ext.hpp
>> for a definition of that API. The default implementation in
>> open performs nothing and inlining should prevent overheads.
>>
>> These are HotSpot internals only, not accessible to the
>> end-user Java application.
>>
>> Details of the shared source code changes, mostly additional calls
>> allowing our closed extension to better control what happens during
>> bootstrap:
>>
>> - support for more than one CodeHeap (independently of
>> -XX:+SegmentedCodeCache), including CodeHeap subclasses
>> that have a different layout
>>
>> - ability to rename a CodeBuffer (allowing to pass the name to
>> SignatureHandlerLibrary::set_handler)
>>
>> - ability to save/load all stubs generated during bootstrap,
>> using CodeCacheExtensions::handle_<...> methods
>>
>> - ability to avoid wasting away CodeHeap space (by optimizing
>> the buffer sizes, avoiding to generate useless code, detecting which
>> interpreter entries are impacted by JVMTI, ...)
>>
>> - ability to install a list of pregenerated fast native
>> signature handlers.
>>
>> - ability, similarly to zero, to generate two interpreter loops,
>> one highly optimized and one which supports JVMTI. The interface
>> is in fact more flexible, allowing to have more than two such loops
>>
>> - ability to prevent the CodeHeap from being executable
>>
>> - a few complete_step calls to allow the extension mechanism to
>> perform additional actions. This could be extended to plug-in
>> other extensions during the bootstrap.
>>
>> Regards,
>>
>> Bertrand.
>>
>>
--
Bertrand Delsart, Grenoble Engineering Center
Oracle, 180 av. de l'Europe, ZIRST de Montbonnot
38330 Montbonnot Saint Martin, FRANCE
bertrand.delsart at oracle.com Phone : +33 4 76 18 81 23
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
NOTICE: This email message is for the sole use of the intended
recipient(s) and may contain confidential and privileged
information. Any unauthorized review, use, disclosure or
distribution is prohibited. If you are not the intended recipient,
please contact the sender by reply email and destroy all copies of
the original message.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
More information about the hotspot-runtime-dev
mailing list