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

Optimize Player#canSee #11595

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

Conversation

MrKinau
Copy link

@MrKinau MrKinau commented Nov 8, 2024

Replaces the invertedVisibilityEntities HashMap with an OpenHashMap for better performance in Player#canSee.

@MrKinau MrKinau requested a review from a team as a code owner November 8, 2024 19:48
@electronicboy
Copy link
Member

electronicboy commented Nov 8, 2024

These collections are not designed for performance, they're designed for reducing memory overheads; I think that in these high read low write situations they tend to perform better, however

(The lack of benchmarking makes it pretty hard to say if this is a real win, however)

@Malfrador
Copy link
Member

canSee performance can be a limitation if you have a lot of otherwise cheap entities (e.g. displays), so generally making improvements here seems sensible to me.

However some benchmarks would be interesting. Did you do any?
The OpenHashMap won't be automatically faster in all cases and just blindly replacing it without proper benchmarking isn't great.

@MrKinau
Copy link
Author

MrKinau commented Nov 9, 2024

However some benchmarks would be interesting. Did you do any?

I did see some non-optimal spark reports with canSee using quite some time of tick and tried to improve it. There may be more room for improvement, but just this small change seems to be a good addition to reduce the time it needs for the containsKey.
I do not feel comfortable sharing the spark reports publicly, but I'll attach some images. Those spark reports may not be 100% comparable, but they are taken on the same server and ig only the player count / player activity should make a difference.

Edit: The reports are not comparable, main issue was the consistently increasing invertedVisibilityEntities, which got fixed by: #11598

HashMap
Fastutil OpenHashMap

@kennytv
Copy link
Member

kennytv commented Nov 9, 2024

In local benchmarks, misses are about 20% faster, but hits more than twice as slow once you go past a few dozen entries of hidden entities. Lynx suggested reducing the number of calls to the method from ChunkMap instead.

switching the collection here might still be a good tradeoff though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Awaiting review
Development

Successfully merging this pull request may close these issues.

5 participants