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

Enable BCU asserts by default #7150

Merged
merged 2 commits into from
Oct 2, 2019

Conversation

theresa-m
Copy link
Contributor

@theresa-m theresa-m commented Sep 18, 2019

Fixes: #7143

Signed-off-by: Theresa Mammarella Theresa.T.Mammarella@ibm.com

@pshipton
Copy link
Member

We intentionally reduced most re-occurring tracepoints that occur during startup by setting them to level 2. Level 2 tracepoints are enabled after the JIT determines the JVM is finished the startup phase of rapidly loading classes. Although it somewhat defeats the purpose of the change, I don't expect these tracepoints can be level 1 without affecting startup time. But this can be verified with perf testing. It may also be beneficial to get the tracepoint counts with these tracepoints enabled, and see if any are extremely heavily used. We probably don't want to enable the heavily used ones at all.

@theresa-m
Copy link
Contributor Author

From my results I'm getting a 0.7% increase in startup time and 0.9% increase in footprint.

I will collect some tracepoint counts and see how perf changes if I remove the top offenders.

@theresa-m
Copy link
Contributor Author

Trc_BCU_Assert_True is used the most by far (tried with java -version as well as daytrader).

Uses of each trace statement running daytrader:
Trc_BCU_Assert_ShouldNeverHappen | 0
Trc_BCU_Assert_Equals | 506
Trc_BCU_Assert_NotEquals | 825
Trc_BCU_Assert_True | 4360
Trc_BCU_Assert_False | 2
Trc_BCU_Assert_TrueTreeVerify | 0
Trc_Assert_BCU_mustHaveVMAccess | 0
Trc_BCU_Assert_LessThan | 786
Trc_BCU_Assert_NotGreaterThan | 348
Trc_BCU_Assert_SupportedByteCode | 155
Trc_Assert_BCU_mustHaveExclusiveVMAccess | 0
Trc_BCU_Assert_InternVerificationFailure | 0

If I change Trc_BCU_Assert_True back to level 3 and leave the rest as level 1 the performance hit reduces to 0.4% increase in startup time and 0.03% difference in footprint.
Unfortunately this would not help for cases such as #7104 for which the assert was Trc_BCU_Assert_True.

@DanHeidinga
Copy link
Member

@theresa-m Is this is the only use of the Trc_BCU_Assert_True tracepoint? If it's not, can you introduce a custom traceassert at level 1 and redo the experiments?

@theresa-m
Copy link
Contributor Author

theresa-m commented Sep 24, 2019

Reran with https://github.com/eclipse/openj9/blob/9d35d06dcbf62bb83d05b93dc980e201f6aeae9e/runtime/bcutil/ROMClassBuilder.cpp#L761 as level 1 and the other Trc_BCU_Assert_True as level 3. The maxRequiredSize check is run infrequently. Results are 0.15% difference in startup -0.39% footprint difference from a baseline.

edit: closing issue was accidental. reopened :)

@theresa-m theresa-m closed this Sep 24, 2019
@theresa-m theresa-m reopened this Sep 24, 2019
@pshipton
Copy link
Member

Something like tWAS which loads many more classes during startup may be more affected.

The other possibility is to make it level 2. This won't catch problems during the startup phase, but anything loaded afterwards will be caught. There is always the option to enable the assert tracepoint(s) via the command line during startup, although typically nobody will remember to do so.

@theresa-m
Copy link
Contributor Author

After running twas with all Trc_BCU_Assert_True set at level 3 except the maxRequiredSize check the % difference in startup is 0.22%

@pshipton
Copy link
Member

Seems fine to me, but pls write up a summary and ping Vijay to get approval.

@theresa-m
Copy link
Contributor Author

@vijaysun-omr after running perf comparisons with this pr I am seeing the following results:
tWas startup: 0.22%
Liberty w/Daytrader startup: 0.15%
Liberty w/Daytrader footprint: -0.39%
Does it look okay to you?

@theresa-m theresa-m marked this pull request as ready for review September 27, 2019 19:00
@vijaysun-omr
Copy link
Contributor

vijaysun-omr commented Sep 27, 2019

@theresa-m yes those regressions are within the acceptable range.
fyi @mpirvu @dsouzai

@pshipton
Copy link
Member

jenkins test sanity win jdk8

@pshipton
Copy link
Member

pshipton commented Sep 28, 2019

New tracepoints need to be added at the end, to avoid changing existing tracepoint numbers. Besides affecting compatibility for formatting tracepoint files, you can see it's also caused a number of test failures.

@theresa-m theresa-m force-pushed the default_asserts_7143 branch 2 times, most recently from eb5171d to 539466f Compare October 1, 2019 21:12
Signed-off-by: Theresa Mammarella <Theresa.T.Mammarella@ibm.com>
Signed-off-by: Theresa Mammarella <Theresa.T.Mammarella@ibm.com>
@theresa-m theresa-m force-pushed the default_asserts_7143 branch from 539466f to ef81bf1 Compare October 2, 2019 14:21
@theresa-m theresa-m requested a review from pshipton October 2, 2019 19:06
@pshipton
Copy link
Member

pshipton commented Oct 2, 2019

jenkins test sanity win jdk8

@pshipton
Copy link
Member

pshipton commented Oct 2, 2019

jenkins test sanity,extended osx jdk8,jdk11

@pshipton pshipton self-assigned this Oct 2, 2019
@pshipton pshipton merged commit 954e6b6 into eclipse-openj9:master Oct 2, 2019
@theresa-m theresa-m deleted the default_asserts_7143 branch November 1, 2019 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable BCU asserts by default
4 participants