<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
<div class="moz-cite-prefix">On 2014-10-08 15:11, Erik Österlund
wrote:<br>
</div>
<blockquote cite="mid:D9D9A71E-42BC-40B0-8A24-6CD9E1E217E1@lnu.se"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
<div style="word-wrap:break-word">Hi Stefan,
<div><br>
<div>
<div>On 08 Oct 2014, at 14:30, Stefan Karlsson <<a
moz-do-not-send="true"
href="mailto:stefan.karlsson@oracle.com">stefan.karlsson@oracle.com</a>>
wrote:</div>
<br class="x_Apple-interchange-newline">
<blockquote type="cite">
<div bgcolor="#FFFFFF">Hi Erik,<br>
<br>
<div class="x_moz-cite-prefix">On 2014-10-08 13:20, Erik
Österlund wrote:<br>
</div>
<blockquote type="cite">
<div style="word-wrap:break-word">
<div>Greetings,</div>
<div><br>
</div>
<div>So here in this email is attached a patch-file
containing my changes since I don't have access to
the cr-server.</div>
If anyone could help me make a RFR with a bug-ID
with this changeset, that would be great.
</div>
</blockquote>
<br>
Webrev: <a moz-do-not-send="true"
class="x_moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Estefank/eriko/8059936/webrev.00/">
http://cr.openjdk.java.net/~stefank/eriko/8059936/webrev.00/</a><br>
bug: <a moz-do-not-send="true"
class="x_moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8059936">
https://bugs.openjdk.java.net/browse/JDK-8059936</a><br>
<br>
I ran this through JPRT and got this failure on Solaris
x86:<br>
<pre>"hotspot/src/share/vm/memory/specialized_oop_closures.hpp", line 222: Warning: Identifier expected instead of "}".
1 Warning(s) detected.
gmake[8]: *** [abstractCompiler.o] Error 2
gmake[7]: *** [the_vm] Error 2
gmake[6]: *** [fastdebug] Error 2
gmake[5]: *** [generic_build2] Error 2
gmake[4]: *** [fastdebug] Error 2
</pre>
</div>
</blockquote>
<div>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.</div>
</div>
</div>
</div>
</blockquote>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~stefank/eriko/8059936/webrev.01.delta/">http://cr.openjdk.java.net/~stefank/eriko/8059936/webrev.01.delta/</a><br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~stefank/eriko/8059936/webrev.01/">http://cr.openjdk.java.net/~stefank/eriko/8059936/webrev.01/</a><br>
<br>
<blockquote cite="mid:D9D9A71E-42BC-40B0-8A24-6CD9E1E217E1@lnu.se"
type="cite">
<div style="word-wrap:break-word">
<div>
<div>
<div>There's no way I can run JPRT myself is there?</div>
</div>
</div>
</div>
</blockquote>
<br>
Unfortunately, no.<br>
<br>
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?<br>
<br>
I saw that one of those failures happened after
specialized_oop_closures.inline.hpp was included from oop.hpp:<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~stefank/eriko/8059936/webrev.01/src/share/vm/oops/oop.hpp.udiff.html">http://cr.openjdk.java.net/~stefank/eriko/8059936/webrev.01/src/share/vm/oops/oop.hpp.udiff.html</a><br>
<br>
You need to rework you dependencies so that you don't include
.inline.hpp files from .hpp files.<br>
<br>
See the new Files section of:<br>
<a class="moz-txt-link-freetext" href="https://wiki.openjdk.java.net/display/HotSpot/StyleGuide">https://wiki.openjdk.java.net/display/HotSpot/StyleGuide</a><br>
<br>
cheers,<br>
StefanK<br>
<br>
<blockquote cite="mid:D9D9A71E-42BC-40B0-8A24-6CD9E1E217E1@lnu.se"
type="cite">
<div style="word-wrap:break-word">
<div>
<div>
<div><br>
</div>
<div>Thank you,</div>
<div><br>
</div>
<div>/Erik</div>
<div><br>
</div>
</div>
</div>
</div>
<div style="word-wrap:break-word">
<div>
<div>
<div><br>
</div>
<blockquote type="cite">
<div bgcolor="#FFFFFF">
<pre>StefanK
</pre>
<br>
<blockquote type="cite">
<div style="word-wrap:break-word">
<div><br>
<div>== Implementation Summary ==</div>
<div><br>
</div>
<div>=== Dispatching To Closures ===</div>
<div>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.</div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>=== Dispatching To Klass ===</div>
<div>Two mechanisms are used for retaining the
OopClosureType when dispatching to it using a
specific Klass.</div>
<div>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".</div>
<div>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.</div>
<div><br>
</div>
<div>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. :)</div>
<div><br>
</div>
<div>Thanks,</div>
<div><br>
</div>
<div>/Erik</div>
</div>
</div>
<div style="word-wrap:break-word">
<div><br>
<div>
<div>On 01 Oct 2014, at 17:02, Erik Österlund
<<a moz-do-not-send="true"
href="mailto:erik.osterlund@lnu.se">erik.osterlund@lnu.se</a>>
wrote:</div>
<br class="x_x_Apple-interchange-newline">
<blockquote type="cite">
<div style="font-family:Helvetica;
font-size:12px; font-style:normal;
font-variant:normal; font-weight:normal;
letter-spacing:normal; line-height:normal;
orphans:auto; text-align:start;
text-indent:0px; text-transform:none;
white-space:normal; widows:auto;
word-spacing:0px">
<br class="x_x_Apple-interchange-newline">
On 01 Oct 2014, at 16:11, Stefan Karlsson
<<a moz-do-not-send="true"
href="mailto:stefan.karlsson@oracle.com">stefan.karlsson@oracle.com</a>>
wrote:</div>
<br class="x_x_Apple-interchange-newline"
style="font-family:Helvetica;
font-size:12px; font-style:normal;
font-variant:normal; font-weight:normal;
letter-spacing:normal; line-height:normal;
orphans:auto; text-align:start;
text-indent:0px; text-transform:none;
white-space:normal; widows:auto;
word-spacing:0px">
<blockquote type="cite"
style="font-family:Helvetica;
font-size:12px; font-style:normal;
font-variant:normal; font-weight:normal;
letter-spacing:normal; line-height:normal;
orphans:auto; text-align:start;
text-indent:0px; text-transform:none;
white-space:normal; widows:auto;
word-spacing:0px">
<div style="font-size:12px;
font-style:normal; font-variant:normal;
font-weight:normal; letter-spacing:normal;
line-height:normal; orphans:auto;
text-align:start; text-indent:0px;
text-transform:none; white-space:normal;
widows:auto; word-spacing:0px">
<br>
On 2014-10-01 15:41, Erik Österlund wrote:<br>
<blockquote type="cite">Hey,<br>
<br>
I have to admit I'm not a huge fan of
the current system for explicitly
specializing closures (removing virtual
calls).<br>
<br>
Here's a couple of problems I see with
the current solution:<br>
1. The specialization macros are
obviously not pretty<br>
2. It's awkward to have to remember to
explicitly list the closure as
specialized in the macros<br>
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.<br>
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.<br>
<br>
With my alternative template magic
solution:<br>
1. You won't have to explicitly
specialize wanted closure types - they
are automatically specialized unless the
contrary is explicitly stated.<br>
2. Parameterized composite closure types
can be used without unnecessary virtual
call overheads.<br>
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.<br>
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.<br>
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*) )<br>
<br>
For the interested reader, this is how
the old system worked:<br>
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++.<br>
<br>
And this is how my system solves this:<br>
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.<br>
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.<br>
<br>
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). :)<br>
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).<br>
</blockquote>
<br>
Yes, please provide a patch!<br>
<br>
I've had a patch to start solving some of
these problems for a long time:<br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Estefank/prototype/oop_iterate_dispatch/webrev.00/">http://cr.openjdk.java.net/~stefank/prototype/oop_iterate_dispatch/webrev.00/</a><br>
<br>
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:<br>
template <bool nv, typename
OopClosureType><br>
inline int
oopDesc::oop_iterate(OopClosureType* blk)
{<br>
CONCRETE_KLASS_DO_AND_RETURN(klass(),
oop_oop_iterate<nv>(this,blk));<br>
}<br>
<br>
and klass.inline.hpp for the dispatch
code:<br>
<br>
+#define
CONCRETE_KLASS_DO_AND_RETURN(the_klass,
todo) \<br>
+ do { \<br>
+ switch
((the_klass)->dispatch_tag()) { \<br>
+ case Klass::_instance:
return
InstanceKlass::cast(the_klass)->todo; \<br>
+ case Klass::_instance_ref:
return
InstanceRefKlass::cast(the_klass)->todo;
\<br>
+ case Klass::_instance_mirror:
return
InstanceMirrorKlass::cast(the_klass)->todo;
\<br>
+ case Klass::_instance_class_loader:
return
InstanceClassLoaderKlass::cast(the_klass)->todo;
\<br>
+ case Klass::_obj_array:
return
ObjArrayKlass::cast(the_klass)->todo; \<br>
+ case Klass::_type_array:
return
TypeArrayKlass::cast(the_klass)->todo;
\<br>
+ default: fatal("Incorrect dispatch
index"); return 0; \<br>
+ } \<br>
+ } while (false)<br>
+<br>
</div>
</blockquote>
<div style="font-family:Helvetica;
font-size:12px; font-style:normal;
font-variant:normal; font-weight:normal;
letter-spacing:normal; line-height:normal;
orphans:auto; text-align:start;
text-indent:0px; text-transform:none;
white-space:normal; widows:auto;
word-spacing:0px">
<br>
</div>
<div style="font-family:Helvetica;
font-size:12px; font-style:normal;
font-variant:normal; font-weight:normal;
letter-spacing:normal; line-height:normal;
orphans:auto; text-align:start;
text-indent:0px; text-transform:none;
white-space:normal; widows:auto;
word-spacing:0px">
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).</div>
<div style="font-family:Helvetica;
font-size:12px; font-style:normal;
font-variant:normal; font-weight:normal;
letter-spacing:normal; line-height:normal;
orphans:auto; text-align:start;
text-indent:0px; text-transform:none;
white-space:normal; widows:auto;
word-spacing:0px">
<br>
</div>
<div style="font-family:Helvetica;
font-size:12px; font-style:normal;
font-variant:normal; font-weight:normal;
letter-spacing:normal; line-height:normal;
orphans:auto; text-align:start;
text-indent:0px; text-transform:none;
white-space:normal; widows:auto;
word-spacing:0px">
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).</div>
<div style="font-family:Helvetica;
font-size:12px; font-style:normal;
font-variant:normal; font-weight:normal;
letter-spacing:normal; line-height:normal;
orphans:auto; text-align:start;
text-indent:0px; text-transform:none;
white-space:normal; widows:auto;
word-spacing:0px">
<br>
</div>
<div style="font-family:Helvetica;
font-size:12px; font-style:normal;
font-variant:normal; font-weight:normal;
letter-spacing:normal; line-height:normal;
orphans:auto; text-align:start;
text-indent:0px; text-transform:none;
white-space:normal; widows:auto;
word-spacing:0px">
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. :)</div>
<div style="font-family:Helvetica;
font-size:12px; font-style:normal;
font-variant:normal; font-weight:normal;
letter-spacing:normal; line-height:normal;
orphans:auto; text-align:start;
text-indent:0px; text-transform:none;
white-space:normal; widows:auto;
word-spacing:0px">
<br>
</div>
<div style="font-family:Helvetica;
font-size:12px; font-style:normal;
font-variant:normal; font-weight:normal;
letter-spacing:normal; line-height:normal;
orphans:auto; text-align:start;
text-indent:0px; text-transform:none;
white-space:normal; widows:auto;
word-spacing:0px">
/Erik</div>
<br style="font-family:Helvetica;
font-size:12px; font-style:normal;
font-variant:normal; font-weight:normal;
letter-spacing:normal; line-height:normal;
orphans:auto; text-align:start;
text-indent:0px; text-transform:none;
white-space:normal; widows:auto;
word-spacing:0px">
<blockquote type="cite"
style="font-family:Helvetica;
font-size:12px; font-style:normal;
font-variant:normal; font-weight:normal;
letter-spacing:normal; line-height:normal;
orphans:auto; text-align:start;
text-indent:0px; text-transform:none;
white-space:normal; widows:auto;
word-spacing:0px">
<div style="font-size:12px;
font-style:normal; font-variant:normal;
font-weight:normal; letter-spacing:normal;
line-height:normal; orphans:auto;
text-align:start; text-indent:0px;
text-transform:none; white-space:normal;
widows:auto; word-spacing:0px">
thanks,<br>
StefanK<br>
<br>
<br>
<blockquote type="cite"><br>
Cheers!<br>
<br>
/Erik</blockquote>
</div>
</blockquote>
</blockquote>
</div>
<br>
</div>
</div>
</blockquote>
<br>
</div>
</blockquote>
</div>
<br>
</div>
</div>
</blockquote>
<br>
</body>
</html>