Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove use of -H:-ParseOnce in GraalVM 22.2 #25944

Closed
galderz opened this issue Jun 3, 2022 · 8 comments · Fixed by #27462
Closed

Remove use of -H:-ParseOnce in GraalVM 22.2 #25944

galderz opened this issue Jun 3, 2022 · 8 comments · Fixed by #27462
Assignees
Labels
kind/enhancement New feature or request
Milestone

Comments

@galderz
Copy link
Member

galderz commented Jun 3, 2022

When ParseOnce was first introduced, we discovered that the overall memory consumption went up. In my most recent testing with 22.1, ParseOnce still added extra memory, but during the Native Image Committer Community Meeting 2022-06-02 meeting Christian mentioned that memory improvements have gone in 22.2 which should solve the problem of extra memory usage.

I will carry out tests with 22.2 to verify this.

@galderz galderz added the kind/enhancement New feature or request label Jun 3, 2022
@galderz galderz self-assigned this Jun 3, 2022
@jerboaa
Copy link
Contributor

jerboaa commented Aug 8, 2022

Related mandrel issue: graalvm/mandrel#316

@zakkak zakkak self-assigned this Aug 16, 2022
@zakkak
Copy link
Contributor

zakkak commented Aug 17, 2022

Quote from #25943 (comment)

Re: ParseOnce. At the end of June I carried some tests to verify its effects on 22.2 and indeed the memory usage had improved, so it no longer resulted in a max RSS penalty. So I think it's safe to remove disabling it.

I performed some more extensive testing which shows that this is still not always true. What seems to hold though is that -H:+ParseOnce no longer results in requiring more than 8GB of memory which was causing issues in github actions, so we could disable it and leave the option of using -H:-ParseOnce or not to the users. WDYT?

@galderz
Copy link
Member Author

galderz commented Aug 18, 2022

@zakkak Thanks for the exhaustive testing. It's important not to forget that there's some variability in the native build memory usage. In my local testing I've found that memory usage varies between different runs with the same configuration. E.g. for the Hibernate ORM quickstart:

  • With ParseOnce disabled, max RSS can be as low as 6.5GB and as high as 8.1GB, that's a 24% difference between the lowest and the highest.
  • With ParseOnce enabled, max RSS can be as low as 6.7GB and as high as 8.1GB, with a difference of 20%.

What I'm trying to say with this is that your ranges of -12% and +19% fall within the variability I see above. So, which ones take more or less max RSS might not be ParseOnce related.

@galderz
Copy link
Member Author

galderz commented Aug 18, 2022

Note that in my testing I don't pass in -Xmx into the native image build processes, so that's why the max RSS numbers can look higher than the ones found in the testsuite runs.

@zakkak
Copy link
Contributor

zakkak commented Aug 23, 2022

What I'm trying to say with this is that your ranges of -12% and +19% fall within the variability I see above. So, which ones take more or less max RSS might not be ParseOnce related.

True, my comparison is not that solid, but the fact that I am reproducing more or less the same results (in terms of RSS usage increase) with the last two times I ran similar tests (a couple of releases before 22.2) make me feel confident about this numbers.

In any case, I am OK with enabling ParseOnce (given that the CI is now happy with it) and letting the users disable it on demand if they really need to. Please see #27462

zakkak added a commit to zakkak/quarkus that referenced this issue Aug 23, 2022
We no longer need to disable `ParseOnce` as memory usage with it enabled
has improved in GraalVM 22.2.

See
https://docs.google.com/spreadsheets/d/15PJ1Qd7kgneuP61N1T2_AyJ3WBsbXpVHIPKbxgH1qfM/edit#gid=1672873268
for a comparison between using and not using `ParseOnce` with 22.2.

Relates to oracle/graal#3435 and
graalvm/mandrel#316

Fixes quarkusio#25944
@galderz
Copy link
Member Author

galderz commented Aug 24, 2022

In any case, I am OK with enabling ParseOnce (given that the CI is now happy with it) and letting the users disable it on demand if they really need to. Please see #27462

That sounds good. Having an option to disable it in case it becomes a problem would be a good escape hatch if someone encounters memory issues with this.

@jerboaa
Copy link
Contributor

jerboaa commented Aug 24, 2022

In any case, I am OK with enabling ParseOnce (given that the CI is now happy with it) and letting the users disable it on demand if they really need to. Please see #27462

That sounds good. Having an option to disable it in case it becomes a problem would be a good escape hatch if someone encounters memory issues with this.

Wouldn't that be covered by passing -Dquarkus.native.additional-build-args="-H:-ParseOnce"?

@quarkus-bot quarkus-bot bot added this to the 2.13 - main milestone Aug 24, 2022
@galderz
Copy link
Member Author

galderz commented Aug 25, 2022

@jerboaa Yes it is, I realised that after posting the comment.

fercomunello pushed a commit to fercomunello/quarkus that referenced this issue Aug 31, 2022
We no longer need to disable `ParseOnce` as memory usage with it enabled
has improved in GraalVM 22.2.

See
https://docs.google.com/spreadsheets/d/15PJ1Qd7kgneuP61N1T2_AyJ3WBsbXpVHIPKbxgH1qfM/edit#gid=1672873268
for a comparison between using and not using `ParseOnce` with 22.2.

Relates to oracle/graal#3435 and
graalvm/mandrel#316

Fixes quarkusio#25944
@gsmet gsmet modified the milestones: 2.13 - main, 2.12.1.Final Sep 5, 2022
gsmet pushed a commit to gsmet/quarkus that referenced this issue Sep 5, 2022
We no longer need to disable `ParseOnce` as memory usage with it enabled
has improved in GraalVM 22.2.

See
https://docs.google.com/spreadsheets/d/15PJ1Qd7kgneuP61N1T2_AyJ3WBsbXpVHIPKbxgH1qfM/edit#gid=1672873268
for a comparison between using and not using `ParseOnce` with 22.2.

Relates to oracle/graal#3435 and
graalvm/mandrel#316

Fixes quarkusio#25944

(cherry picked from commit 03ea11c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants