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

Makes EnvName mutable to avoid allocating it for lookups #1295

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

franz1981
Copy link
Contributor

While working on #1294 I've discovered two interesting things:

  • EnvName::hashCode (which is compute intensive) it's performed twice due to a mix of if get == null, put: here I've enabled hashCode caching, despite it makes EnvName a bit fatter i.e. 24 bytes vs 16 :"(
  • EnvName is created multiple times on the fly, to perform matching - without ever storing it (!). And most of the times these are misses: I've than allow it to be mutable to avoid keeping on allocating them

@franz1981
Copy link
Contributor Author

A similar effect with a smaller increment could be obtained via int[] cache of hash codes; we can still perform hash code computation directly on the String and using the int[] as a sort of bloom filter using a single hash function:

  • if the check is positive we need to perform the allocation and go down the slow path of checking the map
  • if the check is negative no need to further check

That depends by the collision of keys vs the given hash function, so the effect is potential reduction of allocations

Comment on lines +108 to +119
EnvName envName = reusableEnvName;
if (envName == null) {
envName = new EnvName(propertyName);
this.reusableEnvName = envName;
} else {
envName.setName(propertyName);
}
try {
return envVars.getEnv().containsKey(envName);
} finally {
envName.reset();
}
Copy link
Member

Choose a reason for hiding this comment

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

This can cause issues if not thread-safe. While we use a simple HashMap, we never modify it after creation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can make #1295 (comment) once you apply your changes :P

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.

2 participants