<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Hi Jon,<br>
<br>
Thanks for reviewing this.<br>
<br>
<div class="moz-cite-prefix">On 08/29/2016 02:55 PM, Jon Masamitsu
wrote:<br>
</div>
<blockquote
cite="mid:04c8ed54-0780-64fa-38bb-50c8af0210b4@oracle.com"
type="cite">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
<br>
<br>
<div class="moz-cite-prefix">On 08/29/2016 01:31 AM, Stefan
Johansson wrote:<br>
</div>
<blockquote
cite="mid:f81c0a6f-a242-3250-1819-b6a315b4b3f2@oracle.com"
type="cite">...<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite"> <br>
<blockquote type="cite"> <br>
Another thing is the use of #ifdef to make this
conditional for Linux. Is this needed? Isn't the
return value for can_commit_large_page_memory() the
conditional we should care about? </blockquote>
We can know whether we are using 'pin region' from the
return value of can_commit_large_page_memory() and
UseLargePages flag. <br>
<br>
However the condition of comparison with page size in
Linux version of os::pd_free_memory() makes this
problem. For other platforms such as Windows, AIX and
BSD which have empty os::pd_free_memory(), it doesn't
matter. And Solaris doesn't have such condition. This
means for other platforms we don't need to exit
because of reverting to default page size. <br>
<br>
I mean if can_commit_large_page_memory() returns false
and UseLargePages enabled, we will try to use pin
region. But NUMA could try to use default page size.
It is okay in general except on Linux because of above
reason. <br>
<br>
<blockquote type="cite">Or will we fail some platform
too early. If so, we could add another capability
method to the os class and use that to avoid having
the #ifdef in the code. <br>
</blockquote>
I also considered shortly to add a new method to
decide. <br>
And I agree that not using #ifdef is better in
general. But I'm not sure for this case as it is too
Linux implementation specific. i.e. Linux version is
implemented pd_free_memory() to conditionally commit
after comparing with page size. If Linux
pd_free_memory() becomes blank or the condition is
changed, the decision method also should be changed
which seems not worth for me. This is why I stopped
considering it. <br>
<br>
</blockquote>
Ok, I don't see things changing as a big problem, you
could let the new capability just return the same as
can_commit_large_page_memory() for Linux and have the
other platforms return true. This would have the same
maintenance requirements as the current solution in my
eyes. <br>
</blockquote>
I think we have to consider not only the maintenance but
also the necessity of the method. I don't think we could
re-use it and this is why I described it as too Linux
implementation specific. <br>
<br>
If you strongly want to introduce a new method, may I ask
what is expected name/roll of the method? <br>
<br>
</blockquote>
I see your point and I'll leave it up to a second reviewer
to decide which way to go. <br>
</blockquote>
OK. <br>
</blockquote>
</blockquote>
<br>
Stefan and Sangheon,<br>
<br>
I agree that having the #ifdef is unfortunate but I'm really
hesitant about<br>
adding an new API at the os:: level. We could give it a good
name but it<br>
seems like it would just be there for GC.<br>
<br>
A little better I think would be to add a field to
MuntableNUMASpace<br>
<br>
bool _must_use_large_pages;<br>
<br>
and add the #ifdef in MutableNUMASpace::MutableNUMASpace() : ...,
_must_use_large_pages(false) ...<br>
<br>
<pre><span class="new"> 582 #ifdef LINUX</span>
<span class="new"> 583 // Changing the page size below can lead to freeing of memory. When using large pages</span>
<span class="new"> 584 // and the memory has been both reserved and committed, Linux does not support</span>
<span class="new"> 585 // freeing parts of it. So we fail initialization.</span>
<span class="new"> 586 if (UseLargePages && !os::can_commit_large_page_memory()) {</span>
<span class="new"> 587 _must_use_large_pages = true;</span>
<span class="new"> 588 }</span>
<span class="new"> 589 #endif // LINUX</span></pre>
There is not much code in the constructor and adding the #ifdef is
less<br>
distracting.<br>
<br>
Then the code in initialize() could be<br>
<pre> 580 if (base_space_size_pages / lgrp_spaces()->length() == 0
581 && page_size() > (size_t)os::vm_page_size()) {
if (_must_use_large_pages) {
<span class="new"> 587 vm_exit_during_initialization("Failed initializing NUMA with large pages. Too small heap size");
}
</span></pre>
</blockquote>
I think this would give better readability on 'initialize' method.<br>
<br>
Webrev:<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~sangheki/8023905/webrev.2">http://cr.openjdk.java.net/~sangheki/8023905/webrev.2</a><br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~sangheki/8023905/webrev.2_to_1">http://cr.openjdk.java.net/~sangheki/8023905/webrev.2_to_1</a><br>
<br>
Stefan, what do you think?<br>
webrev.2 seems better for me.<br>
<br>
Thanks,<br>
Sangheon<br>
<br>
<br>
<blockquote
cite="mid:04c8ed54-0780-64fa-38bb-50c8af0210b4@oracle.com"
type="cite">
<pre><span class="new">
</span><span class="new"></span>
</pre>
If you don't care for that suggestion, the patch as is looks fine.<br>
<br>
Jon<br>
<pre><span class="new"></span></pre>
<br>
</blockquote>
<br>
</body>
</html>