Automatic Closure Specialization
Stefan Karlsson
stefan.karlsson at oracle.com
Thu Oct 16 08:32:09 UTC 2014
Hi Erik,
On 2014-10-15 13:51, Erik Österlund wrote:
> Hi Stefan,
>
> On 08 Oct 2014, at 14:43, Stefan Karlsson <stefan.karlsson at oracle.com
> <mailto:stefan.karlsson at oracle.com>> wrote:
>
>>
>> On 2014-10-08 15:11, Erik Österlund wrote:
>>> Hi Stefan,
>>>
>>> Here's a new take where I declare my enum on that line with a
>>> compiler helmet on. The new archive contains the _incremental and
>>> _full patch to fix this issue.
>>
>> http://cr.openjdk.java.net/~stefank/eriko/8059936/webrev.01.delta/
>> <http://cr.openjdk.java.net/%7Estefank/eriko/8059936/webrev.01.delta/>
>> http://cr.openjdk.java.net/~stefank/eriko/8059936/webrev.01/
>> <http://cr.openjdk.java.net/%7Estefank/eriko/8059936/webrev.01/>
>>
>>> There's no way I can run JPRT myself is there?
>>
>> Unfortunately, no.
>>
>> This patch fails on some platforms that don't use precompiled
>> headers. Can you compile with USE_PRECOMPILED_HEADER=0 and take care
>> of the compile errors?
>>
>> I saw that one of those failures happened after
>> specialized_oop_closures.inline.hpp was included from oop.hpp:
>> http://cr.openjdk.java.net/~stefank/eriko/8059936/webrev.01/src/share/vm/oops/oop.hpp.udiff.html
>> <http://cr.openjdk.java.net/%7Estefank/eriko/8059936/webrev.01/src/share/vm/oops/oop.hpp.udiff.html>
>>
>> You need to rework you dependencies so that you don't include
>> .inline.hpp files from .hpp files.
>>
>> See the new Files section of:
>> https://wiki.openjdk.java.net/display/HotSpot/StyleGuide
>>
>> cheers,
>> StefanK
>
> Here is a new take. I have:
> 1. Fixed the dependencies so it builds without precompiled header as
> you said.
> 2. Did the same for all other oop_iterate variants (bounded,
> backwards, array range, with and without headers etc).
> 4. Made closures for the old PS and MS macros that didn't go through
> oop_oop_iterate, but still used the old macros.
> 5. Finally actually deleted all the old oop_iterate macros to reflect
> the bug description.
> 6. Tested by running through some DaCapo benchmarks using loads of
> different GC configurations.
>
> Note that I left the macros specifying which closures to specialize in
> case we want it for something else, but every macro related to
> oop_iterate and its family is gone.
>
> I have attached a zip file with an incremental (from last time) and
> full hg export with my changes.
>
> Could you please run this in JPRT for me?
> May the compilers be with me this time...
JPRT report:
Sun Studio compiler:
"hotspot/src/share/vm/opto/convertnode.cpp", line 88: Error: SharedRuntime is not defined.
"hotspot/src/share/vm/opto/convertnode.cpp", line 88: Error: The function "d2i" must have a prototype.
"hotspot/src/share/vm/opto/convertnode.cpp", line 113: Error: SharedRuntime is not defined.
"hotspot/src/share/vm/opto/convertnode.cpp", line 113: Error: The function "d2l" must have a prototype.
"hotspot/src/share/vm/opto/convertnode.cpp", line 150: Error: SharedRuntime is not defined.
"hotspot/src/share/vm/opto/convertnode.cpp", line 150: Error: The function "f2i" must have a prototype.
"hotspot/src/share/vm/opto/convertnode.cpp", line 177: Error: SharedRuntime is not defined.
"hotspot/src/share/vm/opto/convertnode.cpp", line 177: Error: The function "f2l" must have a prototype.
MSVC:
hotspot\src\share\vm\memory\specialized_oop_closures.inline.hpp(35) : error C2244: 'OopClosureDispatcher::do_oop_internal_try_v' : unable to match function definition to an existing declaration
definition
'enable_if<!has_member_function_do_oop<OopClosureType,void(__cdecl OopClosureType::* )(OopType *)>::value,void>::type OopClosureDispatcher::do_oop_internal_try_v(OopClosureType *,OopType *)'
existing declarations
'enable_if<has_member_function_do_oop<OopClosureType,void(__cdecl OopClosureType::* )(OopType *)>::value,void>::type OopClosureDispatcher::do_oop_internal_try_v(OopClosureType *,OopType *)'
'enable_if<!has_member_function_do_oop<OopClosureType,void(__cdecl OopClosureType::* )(OopType *)>::value,void>::type OopClosureDispatcher::do_oop_internal_try_v(OopClosureType *,OopType *)'
... and more ...
StefanK
>
> Thank you,
>
> /Erik
>
>
>>
>>>
>>> Thank you,
>>>
>>> /Erik
>>>
>>>
>>>> StefanK
>>>>
>>>>>
>>>>> == Implementation Summary ==
>>>>>
>>>>> === Dispatching To Closures ===
>>>>> All dispatching to OopClosure go through an all static class
>>>>> OopClosureDispatcher. It uses SFINAE to find the most appropriate
>>>>> member function to call by checking certain conditions of the
>>>>> OopClosureType. For any template parameterized OopClosureType
>>>>> calls to member function X in {do_oop, do_metadata, do_klass,
>>>>> do_class_loader_data} it will attempt the following in this order:
>>>>> 1) Check if the OopClosureType is manually unspecialized for
>>>>> whatever reason (currently only used by the abstract classes
>>>>> OopClosure and ExtendedOopClosure), in that case use virtual call.
>>>>> 2) Check if OopClosureType (and not a parent) declares X_nv, then
>>>>> use a non-virtual call to it. Otherwise 3) check if OopClosureType
>>>>> (and not a parent) declares X, then use a non-virtual call to X
>>>>> (hence removing need to define both X and X_nv any more, but still
>>>>> being backward compatible). Otherwise 3) use a normal virtual call
>>>>> to X.
>>>>>
>>>>> The reason why checking that candidates for non-virtual calls are
>>>>> declared in the concrete OopClosureType and not a parent is to be
>>>>> safe rather than sorry in detecting "abstract" base classes that
>>>>> are never used by themselves. However their derived types are
>>>>> used, but the type passed in to oop_iterate is not the derived
>>>>> closure type but the abstract one. So in summary we make sure we
>>>>> make non-virtual calls to the derived closure types and not the
>>>>> base types.
>>>>>
>>>>> === Dispatching To Klass ===
>>>>> Two mechanisms are used for retaining the OopClosureType when
>>>>> dispatching to it using a specific Klass.
>>>>> Attempt one hopes there is a separation of concern between finding
>>>>> oops and dispatching to OopClosureType. A virtual call is made to
>>>>> the Klass to either a) return back the oop maps or b) identify
>>>>> which Klass it is with a "DispatchTag".
>>>>> If oop maps are returned (InstanceKlass, OopArrayKlass,
>>>>> TypeArrayKlass) then they are iterated over from oop_iterate where
>>>>> all the type info about the closure is still known. If this is not
>>>>> supported (InstanceRefKlass, InstanceMirrorKlass,
>>>>> InstanceClassLoaderKlass), then the dispatch tag is used to call
>>>>> an inline template method defined in brand new
>>>>> Instance*Klass.inline.hpp files.
>>>>>
>>>>> The new methods in Instance*Klass.inline.hpp are (non-virtual)
>>>>> template variants of what the old oop_oop_iterate macros did. They
>>>>> use SFINAE to check if the OopClosureType expects metadata related
>>>>> calls too (if it's an ExtendedOopClosure) otherwise does nothing.
>>>>> Note that oop_iterate now takes an arbitrary type instead of
>>>>> ExtendedOopClosure, but it uses SFINAE to force template
>>>>> instantiations only if it's a subtype of ExtendedOopClosure and
>>>>> otherwise generate compiler errors and similarly on
>>>>> oop_iterate_no_header, specialization is used and it explicitly
>>>>> checks the template parameter is of type OopClosure. If it's an
>>>>> ExtendedOopClosure, it sends the first ever composite closure type
>>>>> NoHeaderOopClosure<OopClosureType> to the dispatch system so that
>>>>> metadata-related calls are never called in the first place. :)
>>>>>
>>>>> Thanks,
>>>>>
>>>>> /Erik
>>>>>
>>>>> On 01 Oct 2014, at 17:02, Erik Österlund <erik.osterlund at lnu.se
>>>>> <mailto:erik.osterlund at lnu.se>> wrote:
>>>>>
>>>>>>
>>>>>> On 01 Oct 2014, at 16:11, Stefan Karlsson
>>>>>> <stefan.karlsson at oracle.com <mailto:stefan.karlsson at oracle.com>>
>>>>>> wrote:
>>>>>>
>>>>>>>
>>>>>>> On 2014-10-01 15:41, Erik Österlund wrote:
>>>>>>>> Hey,
>>>>>>>>
>>>>>>>> I have to admit I'm not a huge fan of the current system for
>>>>>>>> explicitly specializing closures (removing virtual calls).
>>>>>>>>
>>>>>>>> Here's a couple of problems I see with the current solution:
>>>>>>>> 1. The specialization macros are obviously not pretty
>>>>>>>> 2. It's awkward to have to remember to explicitly list the
>>>>>>>> closure as specialized in the macros
>>>>>>>> 3. There are plenty of composite closure types out there. What
>>>>>>>> I mean by this is when closures are combined, e.g. one closure
>>>>>>>> is used to filter a memory range, and if passing the filter, it
>>>>>>>> will invoke the actual closure, currently resulting in a
>>>>>>>> virtual call even though the composite structure is completely
>>>>>>>> known at the call site.
>>>>>>>> 4. Each closure has to have like do_oop, do_oop_v, do_oop_nv,
>>>>>>>> for both oop types and then a do_oop_work for joining them.
>>>>>>>> Yuck! Asserts try to check that the _v and _nv methods do the
>>>>>>>> same thing to combat programmer mistakes.
>>>>>>>>
>>>>>>>> With my alternative template magic solution:
>>>>>>>> 1. You won't have to explicitly specialize wanted closure types
>>>>>>>> - they are automatically specialized unless the contrary is
>>>>>>>> explicitly stated.
>>>>>>>> 2. Parameterized composite closure types can be used without
>>>>>>>> unnecessary virtual call overheads.
>>>>>>>> 3. Only a single do_oop (do_metadata etc) member function is
>>>>>>>> needed, and hence no need to put asserts trying to keep _v and
>>>>>>>> _nv synchronized.
>>>>>>>> 4. It is backward compatible and does not require major
>>>>>>>> refactoring; could transition into this system step by step.
>>>>>>>> The two systems can even co-exist.
>>>>>>>> 5. It supports an interface where OopClosure is the interface
>>>>>>>> to oop_iterate, rather than ExtendedOopClosure. It uses SFINAE
>>>>>>>> to send metadata info to the closure only if the derived type
>>>>>>>> is an ExtendedOopClosure, otherwise it simply sends the oops
>>>>>>>> (do_oop) only. (of course I can remove this if it's unwanted
>>>>>>>> and we intentionally don't want to support
>>>>>>>> oop_iterate(OopClosure*) )
>>>>>>>>
>>>>>>>> For the interested reader, this is how the old system worked:
>>>>>>>> The ugly macros generate overloads of oop_iterate on oopDesc
>>>>>>>> which uses a virtual call to the Klass (also using macro
>>>>>>>> generated overloads) to figure out where the oops are and then
>>>>>>>> call the closure. This step with the virtual call to the Klass
>>>>>>>> to call the closure removes any potential for template magic
>>>>>>>> because template member functions can't be virtual in C++.
>>>>>>>>
>>>>>>>> And this is how my system solves this:
>>>>>>>> A template oop_iterate (with closure type as parameter) member
>>>>>>>> function uses a virtual call to the Klass, but only to acquire
>>>>>>>> information where oops can be found (and NOT to call the actual
>>>>>>>> closure too). It then uses static template polymorphism (CRTP
>>>>>>>> idiom) to invoke the do_oop method of the corresponding derived
>>>>>>>> closure types (without virtual calls). This required support
>>>>>>>> from the Klass implementations. I currently support object
>>>>>>>> arrays and normal instances. If the Klass implementation does
>>>>>>>> not support this new scheme, it simply reverts to a normal
>>>>>>>> virtual call like before.
>>>>>>>> As a bonus I made a new include file in
>>>>>>>> utilities/templateIdioms.hpp with some template magic I needed
>>>>>>>> and which I was missing but could likely be used in more places
>>>>>>>> in the future.
>>>>>>>>
>>>>>>>> Would this change be interesting for the GC group? In that case
>>>>>>>> I could prepare a patch (and perhaps add support to the other
>>>>>>>> Klass implementations). :)
>>>>>>>> I would also need some help to check if this works on your wide
>>>>>>>> range of platforms and compilers etc (only checked the assembly
>>>>>>>> output for my own setup).
>>>>>>>
>>>>>>> Yes, please provide a patch!
>>>>>>>
>>>>>>> I've had a patch to start solving some of these problems for a
>>>>>>> long time:
>>>>>>> http://cr.openjdk.java.net/~stefank/prototype/oop_iterate_dispatch/webrev.00/
>>>>>>> <http://cr.openjdk.java.net/%7Estefank/prototype/oop_iterate_dispatch/webrev.00/>
>>>>>>>
>>>>>>> one problem that I haven't solved in my patch is how to pass
>>>>>>> down the OopClosureType past the Klass::oop_iterate virtual
>>>>>>> calls. See oop.inline.hpp:
>>>>>>> template <bool nv, typename OopClosureType>
>>>>>>> inline int oopDesc::oop_iterate(OopClosureType* blk) {
>>>>>>> CONCRETE_KLASS_DO_AND_RETURN(klass(),
>>>>>>> oop_oop_iterate<nv>(this,blk));
>>>>>>> }
>>>>>>>
>>>>>>> and klass.inline.hpp for the dispatch code:
>>>>>>>
>>>>>>> +#define CONCRETE_KLASS_DO_AND_RETURN(the_klass, todo) \
>>>>>>> + do { \
>>>>>>> + switch ((the_klass)->dispatch_tag()) { \
>>>>>>> + case Klass::_instance: return
>>>>>>> InstanceKlass::cast(the_klass)->todo; \
>>>>>>> + case Klass::_instance_ref: return
>>>>>>> InstanceRefKlass::cast(the_klass)->todo; \
>>>>>>> + case Klass::_instance_mirror: return
>>>>>>> InstanceMirrorKlass::cast(the_klass)->todo; \
>>>>>>> + case Klass::_instance_class_loader: return
>>>>>>> InstanceClassLoaderKlass::cast(the_klass)->todo; \
>>>>>>> + case Klass::_obj_array: return
>>>>>>> ObjArrayKlass::cast(the_klass)->todo; \
>>>>>>> + case Klass::_type_array: return
>>>>>>> TypeArrayKlass::cast(the_klass)->todo; \
>>>>>>> + default: fatal("Incorrect dispatch index"); return 0; \
>>>>>>> + } \
>>>>>>> + } while (false)
>>>>>>> +
>>>>>>
>>>>>> Yeah, I was faced with the same problem. It's impossible in C++
>>>>>> to pass template arguments to virtual member functions. (Note
>>>>>> however that it's possible to have template parameterized /types/
>>>>>> with virtual member functions, so with your choice of using enums
>>>>>> with a dispatch tag I think it should be possible to have a
>>>>>> KlassDispatchProxy<ClosureType> cl; cl.get_and_dispatch(obj);
>>>>>> where KlassDispatchProxy type (rather than Klass type) is picked
>>>>>> using the enum and get_and_dispatch is a virtual method for the
>>>>>> parameterized type).
>>>>>>
>>>>>> However, my choice was to instead recognize finding oop maps and
>>>>>> dispatching to the closure as two fundamentally separate
>>>>>> concerns. So instead I implemented a new virtual Klass member
>>>>>> function to only get the oop maps (if supported) of an oop (for
>>>>>> InstanceKlass simply return the array of oop map blocks, for
>>>>>> ObjArrayKlass simply return an oop map dummy with the interval of
>>>>>> oops for the array). This way, I can get the oop maps with a
>>>>>> virtual call without having to know the ClosureType needed for
>>>>>> dispatching, and then when I have the oop maps, simply dispatc
>>>>>> (partially using SFINAE to dispatch the stuff the closure expects
>>>>>> to receive; depending on if it's an ExtendedOopClosure or not).
>>>>>>
>>>>>> I'm glad this is something useful. Currently I only enable this
>>>>>> for InstanceKlass and ObjArrayKlass, otherwise it reverts to
>>>>>> standard virtual calls. Will prepare a patch with a few more
>>>>>> Klass implementations to make it a bit more complete. :)
>>>>>>
>>>>>> /Erik
>>>>>>
>>>>>>> thanks,
>>>>>>> StefanK
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Cheers!
>>>>>>>>
>>>>>>>> /Erik
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20141016/d6e38599/attachment.htm>
More information about the hotspot-gc-dev
mailing list