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

review: perf: override AbstractMap#get in ElementNameMap #4849

Merged
merged 1 commit into from
Aug 20, 2022

Conversation

SirYwell
Copy link
Collaborator

@SirYwell SirYwell commented Aug 19, 2022

This was discovered after rebasing for #4848.

#4832 caused a severe regression due to the way get in AbstractMap works. Before, AbstractMap#entrySet() just delegated to the underlying map, but with the patch, it first has to build the map. For get, this isn't needed at all, therefore we should override the method.

This brings down this test

from ~1m 17s to ~52s. There might be more methods that should be overridden now, I'll investigate that. One that takes a lot of time is values() which is called heavily at the moment, but we can avoid many calls with #4848, so both PRs combined seem to achieve even better performance than before the regression.

@SirYwell SirYwell changed the title wip: perf: override AbstractMap#get in ElementNameMap review: perf: override AbstractMap#get in ElementNameMap Aug 19, 2022
@I-Al-Istannen
Copy link
Collaborator

I crunched some numbers. The initial regression from @slarse's PR was approximately a factor of 2 (not placing blame here by any means, I didn't see it either and the change was good).

I ran my javadoc indexer for various configurations and got:

Before slarse order fix

266s (user) and 36 wall clock time

After slarse order fix

683s (user) and 1:23 wall clock time

After this PR

440s (user) and 55s wall clock time

After this PR and hannes/perf/PackageFactory

249s (user) and 35s wall clock time

Both changes from @SirYwell address unfortunate consequences of the inherited AbstractMap methods I didn't foresee.
@SirYwell and I really need to come up with some continuous benchmark integration, it seems. I didn't expect the impact of the PR to be that high and should have probably measured it.

Copy link
Collaborator

@slarse slarse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good find, lgtm!

The initial regression from @slarse's PR was approximately a factor of 2 (not placing blame here by any means, I didn't see it either and the change was good)

@I-Al-Istannen I blame whomever came up with inheritance, it's just too easy to shoot yourself in the foot 🤷‍♂️

@slarse slarse merged commit 0d6f397 into INRIA:master Aug 20, 2022
@SirYwell SirYwell deleted the perf/ElementNameMap branch August 20, 2022 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants