Skip to content
New issue

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

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

Already on GitHub? # to your account

Improve remapper cache #31

Merged
merged 1 commit into from
Jan 20, 2023
Merged

Improve remapper cache #31

merged 1 commit into from
Jan 20, 2023

Conversation

fmeum
Copy link
Contributor

@fmeum fmeum commented Jan 8, 2023

The previous remapper implementation contained some branches in which the returned values weren't cached. Most importantly, when a name wasn't mapped, the null value returned in that case is persisted as null in the map and thus wasn't distinguishable from a cache miss with get(). This is worked around by using computeIfAbsent.

Improves the JUnit benchmark's runtime by ~2%. The impact is larger when a larger fraction of identifiers is mapped identically by rules.

The previous remapper implementation contained some branches in which
the returned values weren't cached. Most importantly, when a name wasn't
mapped, the `null` value returned in that case is persisted as `null` in
the map and thus wasn't distinguishable from a cache miss with `get()`.
This is worked around by using `computeIfAbsent`.

Improves the JUnit benchmark's runtime by ~2%. The impact is larger when
a larger fraction of identifiers isn't mapped by rules.
@fmeum fmeum marked this pull request as ready for review January 8, 2023 14:56
Copy link
Collaborator

@johnynek johnynek left a comment

Choose a reason for hiding this comment

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

seems like a cleaner design.

@johnynek johnynek merged commit 55e3d3b into bazeltools:master Jan 20, 2023
johnynek pushed a commit that referenced this pull request Oct 3, 2023
@johnynek johnynek mentioned this pull request Oct 3, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants