RFR: 8264424: Support OopStorage bulk allocation

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8264424: Support OopStorage bulk allocation

Kim Barrett-2
Please review this change to OopStorage to support bulk allocation.  A new overload for allocate is provided:
size_t allocate(oop** entries, size_t size)
The approach taken is to claim all of the available entries in the first available block, add as many of those entries as will fit in the buffer, and release any remaining entries back to the block.  Only the claim part needs to be done while holding the allocation mutex.  This is is optimized for clients that want more than just a couple entries, and minimizes time under the lock.  The maximum number of entries (the number of entries in a block) is provided as a constant for sizing requests, to avoid the release of unrequested entries.  An application that wants more than that, or a specific number that might not be available from the next block, needs to make multiple bulk allocation calls, but that's still going to be much faster than one at a time allocations.

Testing:
mach5 tier1
New gtest for bulk allocation.
I've been using this for a while as part of another in-development change that makes heavy use of this feature.

-------------

Commit messages:
 - oopstorage bulk allocation

Changes: https://git.openjdk.java.net/jdk/pull/3264/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3264&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8264424
  Stats: 132 lines in 4 files changed: 118 ins; 1 del; 13 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3264.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3264/head:pull/3264

PR: https://git.openjdk.java.net/jdk/pull/3264
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264424: Support OopStorage bulk allocation

Albert Mingkun Yang
On Tue, 30 Mar 2021 11:55:59 GMT, Kim Barrett <[hidden email]> wrote:

> Please review this change to OopStorage to support bulk allocation.  A new overload for allocate is provided:
> size_t allocate(oop** entries, size_t size)
> The approach taken is to claim all of the available entries in the first available block, add as many of those entries as will fit in the buffer, and release any remaining entries back to the block.  Only the claim part needs to be done while holding the allocation mutex.  This is is optimized for clients that want more than just a couple entries, and minimizes time under the lock.  The maximum number of entries (the number of entries in a block) is provided as a constant for sizing requests, to avoid the release of unrequested entries.  An application that wants more than that, or a specific number that might not be available from the next block, needs to make multiple bulk allocation calls, but that's still going to be much faster than one at a time allocations.
>
> Testing:
> mach5 tier1
> New gtest for bulk allocation.
> I've been using this for a while as part of another in-development change that makes heavy use of this feature.

Marked as reviewed by ayang (Committer).

src/hotspot/share/gc/shared/oopStorage.cpp line 478:

> 476:     block = block_for_allocation();
> 477:     if (block == NULL) return 0; // Block allocation failed.
> 478:     // Take exclusive use of this block for allocation.

Maybe "Taking all remaining entries, so remove from list."? The "exclusive use" is ensured by the above `MutexLocker`, not `unlink`.

src/hotspot/share/gc/shared/oopStorage.hpp line 119:

> 117:   // Allocates multiple entries, returning them in the ptrs buffer.  The
> 118:   // number of entries that will be allocated is never more than the minimum
> 119:   // of size and bulk_allocate_limit, but may be less than either.  Possibly

It took me a few seconds to understand this sentence. Maybe replace it with "postcondition: result <= min(size, bulk_allocate_limit)" below?

-------------

PR: https://git.openjdk.java.net/jdk/pull/3264
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264424: Support OopStorage bulk allocation

Thomas Schatzl-4
In reply to this post by Kim Barrett-2
On Tue, 30 Mar 2021 11:55:59 GMT, Kim Barrett <[hidden email]> wrote:

> Please review this change to OopStorage to support bulk allocation.  A new overload for allocate is provided:
> size_t allocate(oop** entries, size_t size)
> The approach taken is to claim all of the available entries in the first available block, add as many of those entries as will fit in the buffer, and release any remaining entries back to the block.  Only the claim part needs to be done while holding the allocation mutex.  This is is optimized for clients that want more than just a couple entries, and minimizes time under the lock.  The maximum number of entries (the number of entries in a block) is provided as a constant for sizing requests, to avoid the release of unrequested entries.  An application that wants more than that, or a specific number that might not be available from the next block, needs to make multiple bulk allocation calls, but that's still going to be much faster than one at a time allocations.
>
> Testing:
> mach5 tier1
> New gtest for bulk allocation.
> I've been using this for a while as part of another in-development change that makes heavy use of this feature.

Lgtm.

-------------

Marked as reviewed by tschatzl (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3264
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264424: Support OopStorage bulk allocation [v2]

Kim Barrett-2
In reply to this post by Kim Barrett-2
> Please review this change to OopStorage to support bulk allocation.  A new overload for allocate is provided:
> size_t allocate(oop** entries, size_t size)
> The approach taken is to claim all of the available entries in the first available block, add as many of those entries as will fit in the buffer, and release any remaining entries back to the block.  Only the claim part needs to be done while holding the allocation mutex.  This is is optimized for clients that want more than just a couple entries, and minimizes time under the lock.  The maximum number of entries (the number of entries in a block) is provided as a constant for sizing requests, to avoid the release of unrequested entries.  An application that wants more than that, or a specific number that might not be available from the next block, needs to make multiple bulk allocation calls, but that's still going to be much faster than one at a time allocations.
>
> Testing:
> mach5 tier1
> New gtest for bulk allocation.
> I've been using this for a while as part of another in-development change that makes heavy use of this feature.

Kim Barrett has updated the pull request incrementally with one additional commit since the last revision:

  improve comments per ayang

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3264/files
  - new: https://git.openjdk.java.net/jdk/pull/3264/files/3ac5fec7..a0c7f91c

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3264&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3264&range=00-01

  Stats: 8 lines in 2 files changed: 1 ins; 1 del; 6 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3264.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3264/head:pull/3264

PR: https://git.openjdk.java.net/jdk/pull/3264
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264424: Support OopStorage bulk allocation [v2]

Kim Barrett-2
In reply to this post by Albert Mingkun Yang
On Tue, 6 Apr 2021 09:46:52 GMT, Albert Mingkun Yang <[hidden email]> wrote:

>> Kim Barrett has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   improve comments per ayang
>
> src/hotspot/share/gc/shared/oopStorage.cpp line 478:
>
>> 476:     block = block_for_allocation();
>> 477:     if (block == NULL) return 0; // Block allocation failed.
>> 478:     // Take exclusive use of this block for allocation.
>
> Maybe "Taking all remaining entries, so remove from list."? The "exclusive use" is ensured by the above `MutexLocker`, not `unlink`.

done

> src/hotspot/share/gc/shared/oopStorage.hpp line 119:
>
>> 117:   // Allocates multiple entries, returning them in the ptrs buffer.  The
>> 118:   // number of entries that will be allocated is never more than the minimum
>> 119:   // of size and bulk_allocate_limit, but may be less than either.  Possibly
>
> It took me a few seconds to understand this sentence. Maybe replace it with "postcondition: result <= min(size, bulk_allocate_limit)" below?

done

-------------

PR: https://git.openjdk.java.net/jdk/pull/3264
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264424: Support OopStorage bulk allocation [v3]

Kim Barrett-2
In reply to this post by Kim Barrett-2
> Please review this change to OopStorage to support bulk allocation.  A new overload for allocate is provided:
> size_t allocate(oop** entries, size_t size)
> The approach taken is to claim all of the available entries in the first available block, add as many of those entries as will fit in the buffer, and release any remaining entries back to the block.  Only the claim part needs to be done while holding the allocation mutex.  This is is optimized for clients that want more than just a couple entries, and minimizes time under the lock.  The maximum number of entries (the number of entries in a block) is provided as a constant for sizing requests, to avoid the release of unrequested entries.  An application that wants more than that, or a specific number that might not be available from the next block, needs to make multiple bulk allocation calls, but that's still going to be much faster than one at a time allocations.
>
> Testing:
> mach5 tier1
> New gtest for bulk allocation.
> I've been using this for a while as part of another in-development change that makes heavy use of this feature.

Kim Barrett has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:

 - Merge branch 'master' into bulk_alloc
 - improve comments per ayang
 - oopstorage bulk allocation

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3264/files
  - new: https://git.openjdk.java.net/jdk/pull/3264/files/a0c7f91c..6643db84

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3264&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3264&range=01-02

  Stats: 26753 lines in 510 files changed: 20294 ins; 3517 del; 2942 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3264.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3264/head:pull/3264

PR: https://git.openjdk.java.net/jdk/pull/3264
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264424: Support OopStorage bulk allocation [v3]

Kim Barrett-2
In reply to this post by Albert Mingkun Yang
On Tue, 6 Apr 2021 09:51:47 GMT, Albert Mingkun Yang <[hidden email]> wrote:

>> Kim Barrett has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
>>
>>  - Merge branch 'master' into bulk_alloc
>>  - improve comments per ayang
>>  - oopstorage bulk allocation
>
> Marked as reviewed by ayang (Committer).

Thanks for reviews @albertnetymk and @tschatzl

-------------

PR: https://git.openjdk.java.net/jdk/pull/3264
Reply | Threaded
Open this post in threaded view
|

Integrated: 8264424: Support OopStorage bulk allocation

Kim Barrett-2
In reply to this post by Kim Barrett-2
On Tue, 30 Mar 2021 11:55:59 GMT, Kim Barrett <[hidden email]> wrote:

> Please review this change to OopStorage to support bulk allocation.  A new overload for allocate is provided:
> size_t allocate(oop** entries, size_t size)
> The approach taken is to claim all of the available entries in the first available block, add as many of those entries as will fit in the buffer, and release any remaining entries back to the block.  Only the claim part needs to be done while holding the allocation mutex.  This is is optimized for clients that want more than just a couple entries, and minimizes time under the lock.  The maximum number of entries (the number of entries in a block) is provided as a constant for sizing requests, to avoid the release of unrequested entries.  An application that wants more than that, or a specific number that might not be available from the next block, needs to make multiple bulk allocation calls, but that's still going to be much faster than one at a time allocations.
>
> Testing:
> mach5 tier1
> New gtest for bulk allocation.
> I've been using this for a while as part of another in-development change that makes heavy use of this feature.

This pull request has now been integrated.

Changeset: 22b20f8e
Author:    Kim Barrett <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/22b20f8e
Stats:     132 lines in 4 files changed: 118 ins; 1 del; 13 mod

8264424: Support OopStorage bulk allocation

Reviewed-by: ayang, tschatzl

-------------

PR: https://git.openjdk.java.net/jdk/pull/3264