RFR: 8261551: Remove special CDS handling in Metaspace::allocate

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

RFR: 8261551: Remove special CDS handling in Metaspace::allocate

Ioi Lam-2
Please review this simple patch to remove the special handling of OOM conditions inside `Metaspace::allocate()` for CDS archive dumping. Now the OOM exception will be thrown normally and will be handled by `MetaspaceShared::preload_and_dump()` and `ClassListParser::parse()`.

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

Commit messages:
 - 8261551: Remove special CDS handling in Metaspace::allocate

Changes: https://git.openjdk.java.net/jdk/pull/3185/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3185&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8261551
  Stats: 9 lines in 2 files changed: 0 ins; 7 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3185.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3185/head:pull/3185

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

Re: RFR: 8261551: Remove special CDS handling in Metaspace::allocate

Yumin Qi-3
On Thu, 25 Mar 2021 01:50:49 GMT, Ioi Lam <[hidden email]> wrote:

> Please review this simple patch to remove the special handling of OOM conditions inside `Metaspace::allocate()` for CDS archive dumping. Now the OOM exception will be thrown normally and will be handled by `MetaspaceShared::preload_and_dump()` and `ClassListParser::parse()`.

LGTM.

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

Marked as reviewed by minqi (Reviewer).

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

Re: RFR: 8261551: Remove special CDS handling in Metaspace::allocate

David Holmes-2
In reply to this post by Ioi Lam-2
On Thu, 25 Mar 2021 01:50:49 GMT, Ioi Lam <[hidden email]> wrote:

> Please review this simple patch to remove the special handling of OOM conditions inside `Metaspace::allocate()` for CDS archive dumping. Now the OOM exception will be thrown normally and will be handled by `MetaspaceShared::preload_and_dump()` and `ClassListParser::parse()`.

Hi Ioi,

This seems fine. But I noticed that preload_and_dump doesn't let exceptions escape so should not itself be a TRAPS method (and it can be called with main_thread rather than THREAD).

Thanks,
David

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

Marked as reviewed by dholmes (Reviewer).

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

Re: RFR: 8261551: Remove special CDS handling in Metaspace::allocate

Thomas Stuefe
In reply to this post by Ioi Lam-2
On Thu, 25 Mar 2021 01:50:49 GMT, Ioi Lam <[hidden email]> wrote:

> Please review this simple patch to remove the special handling of OOM conditions inside `Metaspace::allocate()` for CDS archive dumping. Now the OOM exception will be thrown normally and will be handled by `MetaspaceShared::preload_and_dump()` and `ClassListParser::parse()`.

+1. Nice to see this gone.

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

Marked as reviewed by stuefe (Reviewer).

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

Re: RFR: 8261551: Remove special CDS handling in Metaspace::allocate [v2]

Ioi Lam-2
In reply to this post by Ioi Lam-2
> Please review this simple patch to remove the special handling of OOM conditions inside `Metaspace::allocate()` for CDS archive dumping. Now the OOM exception will be thrown normally and will be handled by `MetaspaceShared::preload_and_dump()` and `ClassListParser::parse()`.

Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:

  remove TRAPS from MetaspaceShared::preload_and_dump()

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3185/files
  - new: https://git.openjdk.java.net/jdk/pull/3185/files/5708f5ef..b624b7e5

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

  Stats: 4 lines in 3 files changed: 1 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3185.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3185/head:pull/3185

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

Re: RFR: 8261551: Remove special CDS handling in Metaspace::allocate [v2]

Ioi Lam-2
In reply to this post by David Holmes-2
On Thu, 25 Mar 2021 06:43:03 GMT, David Holmes <[hidden email]> wrote:

> Hi Ioi,
>
> This seems fine. But I noticed that preload_and_dump doesn't let exceptions escape so should not itself be a TRAPS method (and it can be called with main_thread rather than THREAD).
>
> Thanks,
> David

Thanks for noticing. I removed `TRAPS` from `MetaspaceShared::preload_and_dump()` in the updated version, since it's somewhat related to this issue.

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

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

Re: RFR: 8261551: Remove special CDS handling in Metaspace::allocate [v2]

David Holmes-2
In reply to this post by Ioi Lam-2
On Thu, 25 Mar 2021 17:12:46 GMT, Ioi Lam <[hidden email]> wrote:

>> Please review this simple patch to remove the special handling of OOM conditions inside `Metaspace::allocate()` for CDS archive dumping. Now the OOM exception will be thrown normally and will be handled by `MetaspaceShared::preload_and_dump()` and `ClassListParser::parse()`.
>
> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
>
>   remove TRAPS from MetaspaceShared::preload_and_dump()

Marked as reviewed by dholmes (Reviewer).

src/hotspot/share/memory/metaspaceShared.cpp line 615:

> 613: // Preload classes from a list, populate the shared spaces and dump to a
> 614: // file.
> 615: void MetaspaceShared::preload_and_dump() {

I would have kept the Thread argument to avoid the implicit Thread::current() in EXCEPTION_MARK. But it is a margin call either way.

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

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

Re: RFR: 8261551: Remove special CDS handling in Metaspace::allocate [v2]

Ioi Lam-2
In reply to this post by Thomas Stuefe
On Thu, 25 Mar 2021 06:50:24 GMT, Thomas Stuefe <[hidden email]> wrote:

>> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   remove TRAPS from MetaspaceShared::preload_and_dump()
>
> +1. Nice to see this gone.

Thanks @tstuefe @dholmes-ora @yminqi for the review.

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

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

Integrated: 8261551: Remove special CDS handling in Metaspace::allocate

Ioi Lam-2
In reply to this post by Ioi Lam-2
On Thu, 25 Mar 2021 01:50:49 GMT, Ioi Lam <[hidden email]> wrote:

> Please review this simple patch to remove the special handling of OOM conditions inside `Metaspace::allocate()` for CDS archive dumping. Now the OOM exception will be thrown normally and will be handled by `MetaspaceShared::preload_and_dump()` and `ClassListParser::parse()`.

This pull request has now been integrated.

Changeset: 41657b15
Author:    Ioi Lam <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/41657b15
Stats:     13 lines in 5 files changed: 1 ins; 7 del; 5 mod

8261551: Remove special CDS handling in Metaspace::allocate

Reviewed-by: minqi, dholmes, stuefe

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

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