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

Refactor PlatformManager for performance #2151

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

anonyco
Copy link

@anonyco anonyco commented Jul 23, 2022

Two improvements sure to provide a nice speedup to WorldEdit:

  1. Desynchronize <PlatformManager>.queryCapability, which is used in some pretty tight loops that would feel the benefits of less synchronized.
  2. Remove the use of EnumMap from PlatformManager.preferences, which further optimizes <PlatformManager>.queryCapability because EnumMaps do incur some overhead, mostly from checking to make sure the enum you passed it is really of the same class as the enum of the EnumMap.

Two improvements sure to provide a nice speedup to WorldEdit:
1. desynchronize `<PlatformManager>.queryCapability`, which is used in some pretty tight loops that would feel the benefits of less `synchronized`.
2. remove the use of `EnumMap` from `PlatformManager.preferences`, which further optimizes `<PlatformManager>.queryCapability` because `EnumMap`s do incur some overhead, mostly from checking to make sure the `enum` you passed it is really of the same class as the `enum` of the `EnumMap`.
@me4502
Copy link
Member

me4502 commented Jul 23, 2022

Refactors for performance that make the code substantially messier (the fact you didn't follow the style guide at all aside), without any proof of speed-up or evidence that it's necessary, won't be accepted.

Provide a microbenchmark and CPU profile that show why these changes make sense to do if you want to PR something like this.

I'm also not convinced it couldn't be done in a substantially cleaner manner, if it's true that this has a performance impact

All refactoring except for the removal of one `synchronized` was redacted.
@anonyco
Copy link
Author

anonyco commented Jul 23, 2022

I have significantly simplified these changes to the bare minimum: the removal of just the synchronized.

  1. I was unaware that there was a style guide. I read all the markdown and txt files, including CONTRIBUTING.md. Could you refer me to where to find the style guide?

  2. The results of my microbenchmark are below:

Before:
     Calculated BlockType.getRichName x10000 in 84128654ns
     Calculated BlockType.getRichName x10000 in 14269327ns
     Calculated BlockType.getRichName x10000 in 7806132ns

After:
     Calculated BlockType.getRichName x10000 in 60178043ns
     Calculated BlockType.getRichName x10000 in 7953745ns
     Calculated BlockType.getRichName x10000 in 4852244ns

Across the runs above, we see 40%-79% better performance for BlockType.getRichName with this small change.

  1. I am very unfamiliar with the available Java tools and do not know "CPU profile." I am not asking you to waste your time explaining every step to me, rather I would really appreciate it if you could give me a piece of text or a name of a software/tool that I could look up and research to learn more about what the common way to do this is in the WorldEdit project.

Many thanks for your time and attention to this.

@octylFractal octylFractal self-assigned this Jul 30, 2022
Copy link
Member

@octylFractal octylFractal left a comment

Choose a reason for hiding this comment

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

I would rather refactor the whole class to stop using synchronized at all, and instead take advantage of a ReadWriteLock (probably ReentrantReadWriteLock to start with). It'll increase performance everywhere and is not as potentially dangerous as unguarded reads from a thread-unsafe map. If you don't feel up to that, please close this PR and I will file an issue to do so at a later date.

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

3 participants