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

Performance patch #2676

Merged
merged 1 commit into from
Dec 11, 2023
Merged

Performance patch #2676

merged 1 commit into from
Dec 11, 2023

Conversation

Jpx3
Copy link
Contributor

@Jpx3 Jpx3 commented Dec 10, 2023

  • Convert a getDefault() != null check to a designated method hasDefault()
  • Added a thread-safe and weak cache to the latter

* Convert a getDefault() != null check to a designated method hasDefault()
* Added a thread-safe and weak cache to the latter
@dmulloy2
Copy link
Owner

nice! would be interested to see the performance numbers if you have them

@Jpx3
Copy link
Contributor Author

Jpx3 commented Dec 10, 2023

grafik

@dmulloy2
Copy link
Owner

@Jpx3 thank you! is that before or after?

@derklaro
Copy link
Contributor

How about using guava new MapMaker().weakKeys().makeMap()? This would create a concurrent map (removes the need to compute the value for each thread) with weak keys. Just note that this technically creates a ConcurrentWeakIdentityMap - not sure if this is really relevant in this case.

@dmulloy2
Copy link
Owner

yeah that's a good point. is there any advantage to keeping it thread-local? i'd imagine classes are instantiable or not regardless of thread

@Jpx3
Copy link
Contributor Author

Jpx3 commented Dec 10, 2023

Having it thread-local eliminates any locking by the map. This makes it thread-safe by design while still retaining its speed. And since we are talking about some class references and a few booleans, having some extra copies per thread doesn't matter that much. It will make the problem 50 times faster, but if you want to spend 4 hours thinking how to make it 51 times faster, we can elaborate this discussion 😅

@dmulloy2
Copy link
Owner

fair enough!

@dmulloy2 dmulloy2 merged commit 2448d83 into dmulloy2:master Dec 11, 2023
2 of 3 checks passed
@derklaro
Copy link
Contributor

We're basically moving this problem behind the "plugins (or the server itself) start using virtual threads and users complain about performance again" barrier 😄

@Janmm14
Copy link

Janmm14 commented Apr 7, 2024

We're basically moving this problem behind the "plugins (or the server itself) start using virtual threads and users complain about performance again" barrier 😄

I would hope at that point java virtual threads have a solution to handle this.

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.

None yet

4 participants