<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">Hi Dmitry,<div><br></div><div>Thank you for the feedback! You are definitely right about freeing the archived regions in those failure cases. I should have thought it more thoroughly, instead of just fixing the case that I ran into. Will exam all cases.</div><div><br></div><div>I’ll change the test to incorporate your suggestion. Will fix the extra spaces before committing. I have one question, how do you detect the extra spaces from the webrev? :)</div><div><br></div><div>Thanks,</div><div>Jiangli<br><div><br><div><div>On Aug 6, 2015, at 2:59 PM, Dmitry Dmitriev <<a href="mailto:dmitry.dmitriev@oracle.com">dmitry.dmitriev@oracle.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">
<meta content="text/html; charset=windows-1252" http-equiv="Content-Type">
<div bgcolor="#FFFFFF" text="#000000">
Hello Jiangli,<br>
<br>
I have few comments/questions.<br>
<br>
src/share/vm/memory/filemap.cpp module:<br>
1) Should free_archive_regions also called when
verify_string_regions() returns false on line 717?<br>
2) The same question about unmap_string_regions(). Region must be
freed when unmap_string_regions() is called?<br>
3) Extra space at the end of the line 711.<br>
<br>
test/runtime/SharedArchiveFile/SharedStringsRunAuto.java<br>
1) Unneeded second creating of OutputAnalyzer on line 61. Also,
probably will be better to use same scheme for OutputAnalyzer? On
lines 46-49 you not use local variable, but on lines 61-63 use local
variable.<br>
59 OutputAnalyzer output = new OutputAnalyzer(pb.start());<br>
60 <br>
61 output = new OutputAnalyzer(pb.start());<br>
2) Extra space at the end of the lines 25,26,30, 51<br>
<br>
Thanks,<br>
Dmitry<br>
<br>
<div class="moz-cite-prefix">On 07.08.2015 0:32, Jiangli Zhou wrote:<br>
</div>
<blockquote cite="mid:AD45178E-A442-4A0D-80D6-50E91608E1FE@oracle.com" type="cite">Hi,
<div><br>
</div>
<div>Here is the runtime part of the bug fix that calls the new
free_archive_regions() when shared string mapping fails. I also
added a jtreg test to test shared strings with -Xshare:auto.</div>
<div><br>
</div>
<div> <a moz-do-not-send="true" href="http://cr.openjdk.java.net/%7Ejiangli/8131734/webrev.00/">http://cr.openjdk.java.net/~jiangli/8131734/webrev.00/</a></div>
<div><br>
</div>
<div>Test:</div>
<div>- Tested by explicitly making the shared string mapping fail
on linux-x64, -Xshare:auto runs without crash with the fix</div>
<div>- Tested with the new SharedStringsRunAuto test</div>
<div>- Tested with <span style="font-family: Menlo; font-size:
11px;">XX:+PrintNMTStatistics -XX:NativeMemoryTracking=detail</span></div>
<div><span style="font-family: Menlo; font-size: 11px;"><br>
</span></div>
<div><span style="font-family: Menlo; font-size: 11px;">Thanks,</span></div>
<div><span style="font-family: Menlo; font-size: 11px;">Jiangli</span></div>
</blockquote>
<br>
</div>
</blockquote></div><br></div></div></body></html>