<div dir="auto">Thanks Thomas!<div dir="auto"><br></div><div dir="auto">I will change the comment.</div><div dir="auto"><br></div><div dir="auto"><br></div><div dir="auto">Yasumasa</div><div dir="auto"><br></div><div dir="auto"><br></div></div><br><div class="gmail_quote"><div dir="ltr">2019年2月1日(金) 18:26、Thomas Stüfe さん(<a href="mailto:thomas.stuefe@gmail.com">thomas.stuefe@gmail.com</a>)のメッセージ:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr">Hi Yasumasa, <div><br></div><div>looks good to me as well.</div><div><br></div><div>I like the added comment but would suggest making it more precise: "Try to increase metaspace size by v bytes" reads like it does anything of substance like allocating memory etc, but it does nothing but increase the limit variable. How about this:</div><div><br></div><div>"Try to increase the _capacity_until_GC limit counter by v bytes. </div><div> Returns true if it succeeded. It may fail if either another thread concurrently increased the limit or the new limit would be larger than MaxMetaspaceSize."<br></div><div><br></div><div>Maybe also indicate that can_retry is only defined if function returns false, new_cap_until_GC and old_cap_until_GC only if return value is true:</div><div><br></div><div><div>- // Optionally returns new and old metaspace capacity in</div><div>+ // On success, optionally returns new and old metaspace capacity in<br></div><div><br></div><div>- // Optionally sets can_retry to indicate whether if there is actually</div><div>+ // On error, optionally sets can_retry to indicate whether if there is actually</div></div><div><br></div><div>If you change the comment, I am fine with the patch as it is and do not need another webrev.<br></div><div><br></div><div>Thank you, Thomas</div><div><br></div><div><br></div><div><br></div><div><br></div><div><br></div></div></div></div></div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Jan 31, 2019 at 3:00 PM Yasumasa Suenaga <<a href="mailto:yasuenag@gmail.com" target="_blank" rel="noreferrer">yasuenag@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Thanks Thomas!<br>
<br>
I will add the comment to inc_capacity_until_GC() declaration.<br>
<br>
I'm waiting for second reviewer.<br>
<br>
<br>
Yasumasa<br>
<br>
<br>
On 2019/01/31 18:16, Thomas Schatzl wrote:<br>
> Hi Yasumasa,<br>
> <br>
> On Thu, 2019-01-31 at 14:09 +0900, Yasumasa Suenaga wrote:<br>
>> Hi Thomas,<br>
>><br>
>> I uploaded new webrev:<br>
>>    <a href="http://cr.openjdk.java.net/~ysuenaga/JDK-8217432/webrev.03/" rel="noreferrer noreferrer" target="_blank">http://cr.openjdk.java.net/~ysuenaga/JDK-8217432/webrev.03/</a><br>
>><br>
>> I agree with you that inc_capacity_until_GC() returns additional<br>
>> bool value whether new HWM is exceeded MaxMetaspaceSize.<br>
>><br>
>> I added `can_retry` to argument of inc_capacity_until_GC().<br>
>> It will be set to false if `new_value` exceeds MaxMetaspaceSize.<br>
>> Then inc_capacity_until_GC() returns false, but it is not break<br>
>> assert() in compute_new_size() because inc_capacity_until_GC()<br>
>> is called with `expand_bytes` which is limited by MaxMetaspaceSize.<br>
>> (It is ensured by this change)<br>
>><br>
>> This change has passed vmTestbase/metaspace, gc/metaspace, and<br>
>> submit repo tests.<br>
> <br>
>    looks good to me. Can you add some documentation to the declaration<br>
> of inc_capacity_until_GC() like:<br>
> <br>
> // Try to increase metaspace size by v bytes. Returns true if<br>
> // succeeded, false if not due to competing threads trying.<br>
> // Optionally returns new and old metaspace capacity in<br>
> // new_cap_until_GC and old_cap_until_GC respectively.<br>
> // Optionally sets can_retry to indicate whether if there is actually<br>
> // enough space remaining to satisfy the request.<br>
> <br>
> No need for a re-review for that (or something similar potentially<br>
> better worded description.<br>
> <br>
> Thanks,<br>
>    Thomas<br>
> <br>
> <br>
</blockquote></div>
</blockquote></div>