<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">New webrevs:<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~stefank/8033764/webrev.01.delta/">http://cr.openjdk.java.net/~stefank/8033764/webrev.01.delta/</a><br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~stefank/8033764/webrev.01">http://cr.openjdk.java.net/~stefank/8033764/webrev.01</a><br>
<br>
- Fixed most review comments. Waiting for more comments before
deciding on void* vs OopOrNarrowOopStar.<br>
- Fixed a copy-n-paste error in the tests:<br>
<meta http-equiv="content-type" content="text/html;
charset=ISO-8859-1">
<pre><span class="removed">- case 1: oops_do_narrow_then_full(cl);</span>
<span class="new">+ case 1:</span>
<span class="new">+ oops_do_full_then_narrow(cl);
</span></pre>
thanks,<br>
StefanK<br>
<br>
On 06/02/14 14:43, Stefan Karlsson wrote:<br>
</div>
<blockquote cite="mid:52F39193.5030407@oracle.com" type="cite">Mikael,
<br>
<br>
On 06/02/14 14:12, Mikael Gerdin wrote:
<br>
<blockquote type="cite">Stefan,
<br>
<br>
I think it turned up pretty good,
<br>
</blockquote>
<br>
Thanks.
<br>
<br>
<blockquote type="cite"> I have some primarily stylistic comments
<br>
inline.
<br>
<br>
On Thursday 06 February 2014 12.20.34 Stefan Karlsson wrote:
<br>
<blockquote type="cite">Hi all,
<br>
<br>
please review the following patch:
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~stefank/8033764/webrev.00/">http://cr.openjdk.java.net/~stefank/8033764/webrev.00/</a>
<br>
</blockquote>
-> bufferingOopClosure.hpp:
<br>
<br>
50 void* _buffer[BufferLength];
<br>
51 void** _oop_top;
<br>
52 void** _narrowOop_bottom;
<br>
Would it make sense to use the typedef OopOrNarrowOopStar (or a
union) instead
<br>
of raw void*?
<br>
</blockquote>
<br>
I don't mind the raw void*. I'll change it to OopOrNarrowOopStar
if the second reviewer agrees with you.
<br>
<br>
<blockquote type="cite">
<br>
62 bool is_buffer_full() {
<br>
63 return (uintptr_t)_narrowOop_bottom <
(uintptr_t)_oop_top;
<br>
64 }
<br>
Why do you cast the pointers to uintptr_t before the compare?
<br>
</blockquote>
<br>
I'll remove the cast.
<br>
<br>
<blockquote type="cite">
<br>
<br>
-> bufferingOopClosure.cpp
<br>
<br>
141
<br>
142 static void testCount(int num_narrow, int num_full, int
do_oop_order) {
<br>
143 FakeRoots fr(num_narrow, num_full);
<br>
144
<br>
Can you please add the parameter combination which caused the
assert to fail
<br>
to simplify investigating test failures?
<br>
<br>
If you don't want to do all the printing in the test cases you
can use macro
<br>
like execute_internal_vm_tests does:
<br>
5053 #define run_unit_test(unit_test_function_call)
\
<br>
5054 tty->print_cr("Running test: "
#unit_test_function_call); \
<br>
5055 unit_test_function_call
<br>
</blockquote>
<br>
Sure. Let's see what I end up with.
<br>
<br>
<blockquote type="cite">
<br>
<br>
(nitpicking)
<br>
100 void oops_do(OopClosure* cl, int do_oop_order) {
<br>
101 switch(do_oop_order) {
<br>
102 case 0: oops_do_narrow_then_full(cl);
<br>
103 break;
<br>
I'm pretty sure that we usually indent the break one level past
the "case".
<br>
</blockquote>
I'll change it.
<br>
<br>
Thanks for reviewing!
<br>
<br>
StefanK
<br>
<br>
<blockquote type="cite">
<br>
/Mikael
<br>
<br>
<blockquote type="cite">for the RFE:
<br>
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8033764">https://bugs.openjdk.java.net/browse/JDK-8033764</a>
<br>
<br>
This is a first step to simplify/unify the code root scanning,
something
<br>
we want for the G1 Class Unloading project:
<br>
<a class="moz-txt-link-freetext" href="http://openjdk.java.net/jeps/156">http://openjdk.java.net/jeps/156</a>
<br>
<br>
Background:
<br>
<br>
G1 uses the BufferingOopClosure to separate the root iteration
from the
<br>
object copying. The oop*/narrowOop* roots are gathered in a
buffer and
<br>
then bulk processed. By only timing the processing part and
not the root
<br>
iteration, the object copying time can be measured.
<br>
<br>
Currently, the BufferingOopClosure uses StarTask, from the
taskqueue
<br>
code, to differentiate between oop* and narrowOop* roots. The
StarTask
<br>
uses the least significant bit to mark whether the address
contains an
<br>
oop or a narrowOop. This works for most of the roots, since
the
<br>
addresses are aligned and the LSB is always 0. However, oops
can be
<br>
embedded as immediates in the JITed code and the addresses for
these are
<br>
not necessarily aligned. This prevents us from using StarTasks
with oops
<br>
in the CodeCache.
<br>
<br>
See this comment from g1_process_strong_roots:
<br>
...
<br>
BufferingOopClosure
buf_scan_non_heap_roots(scan_non_heap_roots);
<br>
<br>
// Walk the code cache/strong code roots w/o buffering,
because StarTask
<br>
// cannot handle unaligned oop locations.
<br>
CodeBlobToOopClosure
eager_scan_code_roots(scan_non_heap_roots, true /*
<br>
do_marking */);
<br>
...
<br>
<br>
I suggest that we replace the StartTask usage with another
<br>
implementation that allows the BufferingOopClosures to be used
for the
<br>
CodeCache scanning.
<br>
<br>
thanks,
<br>
StefanK
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>