<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Hi Erik,<br>
<br>
<div class="moz-cite-prefix">On 2014-10-15 13:51, Erik Österlund
wrote:<br>
</div>
<blockquote cite="mid:66CF044E-75AC-443F-913B-BEA9A588CDE1@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:43, 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"><br>
<div class="x_moz-cite-prefix">On 2014-10-08 15:11, Erik
Österlund wrote:<br>
</div>
<blockquote type="cite">
<div style="word-wrap:break-word">Hi Stefan,
<div><br>
<div>
<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 moz-do-not-send="true"
class="x_moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Estefank/eriko/8059936/webrev.01.delta/">http://cr.openjdk.java.net/~stefank/eriko/8059936/webrev.01.delta/</a><br>
<a moz-do-not-send="true"
class="x_moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Estefank/eriko/8059936/webrev.01/">http://cr.openjdk.java.net/~stefank/eriko/8059936/webrev.01/</a><br>
<br>
<blockquote 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 moz-do-not-send="true"
class="x_moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Estefank/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 moz-do-not-send="true"
class="x_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>
</div>
</blockquote>
<div><br>
</div>
<div>Here is a new take. I have:</div>
<div>1. Fixed the dependencies so it builds without
precompiled header as you said.</div>
<div>2. Did the same for all other oop_iterate variants
(bounded, backwards, array range, with and without headers
etc).</div>
<div>4. Made closures for the old PS and MS macros that
didn't go through oop_oop_iterate, but still used the old
macros.</div>
<div>5. Finally actually deleted all the old oop_iterate
macros to reflect the bug description.</div>
<div>6. Tested by running through some DaCapo benchmarks
using loads of different GC configurations.</div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>I have attached a zip file with an incremental (from
last time) and full hg export with my changes.</div>
<div><br>
</div>
<div>Could you please run this in JPRT for me?</div>
<div>May the compilers be with me this time...</div>
</div>
</div>
</div>
</blockquote>
<br>
JPRT report:<br>
<br>
Sun Studio compiler:<br>
<pre>"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 ...
</pre>
StefanK<br>
<br>
<blockquote cite="mid:66CF044E-75AC-443F-913B-BEA9A588CDE1@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>
</div>
</div>
<div style="word-wrap:break-word">
<div>
<div>
<div><br>
</div>
<br>
<blockquote type="cite">
<div bgcolor="#FFFFFF"><br>
<blockquote 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_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_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_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>
</div>
</blockquote>
</div>
<br>
</div>
</div>
</blockquote>
<br>
</body>
</html>