<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<br>
Hi Stefan,<br>
<br>
<div class="moz-cite-prefix">On 2014-02-06 16:58, Stefan Karlsson
wrote:<br>
</div>
<blockquote cite="mid:52F3B117.9040004@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<div class="moz-cite-prefix">New webrevs:<br>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Estefank/8033764/webrev.01.delta/">http://cr.openjdk.java.net/~stefank/8033764/webrev.01.delta/</a><br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Estefank/8033764/webrev.01">http://cr.openjdk.java.net/~stefank/8033764/webrev.01</a><br>
</div>
</blockquote>
<br>
Looks good to me.<br>
<br>
<br>
<blockquote cite="mid:52F3B117.9040004@oracle.com" type="cite">
<div class="moz-cite-prefix"> <br>
- Fixed most review comments. Waiting for more comments before
deciding on void* vs OopOrNarrowOopStar.<br>
</div>
</blockquote>
<br>
I don't have a strong opinion, but I kind of prefer the void*
version.<br>
<br>
Bengt<br>
<br>
<blockquote cite="mid:52F3B117.9040004@oracle.com" type="cite">
<div class="moz-cite-prefix"> - 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 moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Estefank/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 moz-do-not-send="true" 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 moz-do-not-send="true" 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>
</blockquote>
<br>
</body>
</html>