RFR (M): JDK-8030177: G1: Enable TLAB resizing (original) (raw)

Bengt Rutisson bengt.rutisson at oracle.com
Thu Jan 23 14:21:40 UTC 2014

Hi Thomas,

Thanks for looking at this again!

On 2014-01-23 11:35, Thomas Schatzl wrote:

On Thu, 2014-01-23 at 10:27 +0100, Bengt Rutisson wrote:

Hi all,

I went through this change with Stefan Karlsson. Here is an updated webrev based on his suggestions: http://cr.openjdk.java.net/~brutisso/8030177/webrev.01/ Here is a webrev with just the changes that Stefan suggested: http://cr.openjdk.java.net/~brutisso/8030177/webrev.01delta/ A few more cleanup requests: - G1CollectedHeap::unsafemaxtlaballoc(Thread* ignored) const could use the new maxtlabsize() method too instead of doing the same calculation. It seems to return a just too large value too..

Good point. Fixed.

- the comment for G1CollectedHeap::maxtlabsize() could probably improved, e.g. "TLABs should not contain..." instead of "TLABs should not be containing..."

Fixed.

- in G1CollectedHeap::tlabcapacity(Thread), why not use g1policy->younglisttargetlength() - younglist()->length() as a better approximation of the remaining amount of space?

Actually it should not return the remaining space but the total space. So, the current implementation is correct.

- ThreadLocalAllocBuffer::setmaxsize() does not seem to be used anywhere any more. Which makes the assert() in maxsize() fail always. (I grepped http://cr.openjdk.java.net/~brutisso/8030177/webrev.01/hs-gc9.patch and the only occurrence of setmaxsize() is its declaration). Is the patch complete?

Sorry. This was lost when I was doing some mq-tricks to get the webrevs together. Thanks for catching it!

The call should be in Universe::initialize_heap(). I added it back now.

- not sure why the change makes G1CollectedHeap::younglist() const while adding younglisttargetlenght() non-const. Any reasons?

It is because I now use it in G1CollectedHeap::tlab_used(), which is const and can only call const methods.

- could the patch initialize the other members of ThreadLocalAllocBuffer in ThreadLocalAllocBuffer::ThreadLocalAllocBuffer() too?

I'd prefer not to do this as part of this change. The constructor now has this comment:

 // do nothing.  tlabs must be inited by initialize() calls

So, I think it might be better to move all initialization from initialize() to the constructor in a separate change.

New webrev: http://cr.openjdk.java.net/~brutisso/8030177/webrev.02/

Thanks, Bengt

Thanks, Thomas

More information about the hotspot-gc-dev mailing list