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

feat: improve fawe limits (#2773) #2858

Merged
merged 3 commits into from
Sep 17, 2024
Merged

Conversation

dordsor21
Copy link
Member

@dordsor21 dordsor21 commented Jul 28, 2024

  • add FaweLimit implementations for increasing concurrency levels
  • allow FaweLimit to perform processing (and forcefully disable as required) to capture [tile] entities
  • Use BlockVector3#set(Extent orDefault) where appropriate to reduce block checks
  • fixes max-blockstates config limit is not enforced on //set #2679
  • take 2 looks like it should work fine

@dordsor21 dordsor21 requested a review from a team as a code owner July 28, 2024 18:12
@github-actions github-actions bot added Bugfix This PR fixes a bug Feature This PR adds a new feature labels Jul 28, 2024
@dordsor21
Copy link
Member Author

dordsor21 commented Jul 28, 2024

See dea1fcf for fix

@SirYwell
Copy link
Member

So, changing from Extent#method(BV3) to BV3#method(Extent) might skip the LimitExtent, but only when processing? If so, that's fine, and should bring a nice performance improvement in general for everything that reads from the world (some patterns, most masks).

I'm not sure about the different limit implementations. Are they needed? How relevant would be the overhead when just having a limit using Atomic* types? We could also probably use VarHandles to have more control over memory ordering while also reducing objects needed in that case. Does the ProcessorFaweLimit actually improve performance? The lambda in updateAndGet might be called multiple times, while also writing to a volatile variable...

Similarly, the SynchronousSettingExpected annotation and the boolean flags feel a bit fragile and more like band-aid. Those are also only needed because of the different limits, right?

@dordsor21
Copy link
Member Author

In fairness where we're likely to see the speed matter is concurrent-capable operations that are already fast, i.e. set and replace, where we'd be using the concurrent limit anyway so it's probably worth just collapsing to atomic.

I don't think VarHandles can offer us much more than we're getting with Atomic. We need to be sure of both the read and write at the same time and the read only matters "once" as the edit is consequently exited so I would say the simplicity of Atomic is worth it. The memory overhead is also next to nothing

@dordsor21 dordsor21 force-pushed the fix/full-chunk-section-filter branch from dea1fcf to 08f401f Compare September 14, 2024 20:29
Copy link
Member

@SirYwell SirYwell left a comment

Choose a reason for hiding this comment

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

Looks good. One thing I noticed is that ABlockMask#test(Extent, BV3) still goes through the extent. I wonder if we actually need that method at all or if the default is good enough, or if it is safe to go the vector.getBlock(extent) route there too. E.g. a //replace stone diamond_block on a 2048x320x2048 area takes ~26s vs ~17s due to that.

@dordsor21 dordsor21 requested a review from a team September 15, 2024 09:51
@NotMyFault NotMyFault removed the Bugfix This PR fixes a bug label Sep 15, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
 - add FaweLimit implementations for increasing concurrency levels
 - allow FaweLimit to perform processing (and forcefully disable as required) to capture [tile] entities
 - Use `BlockVector3#set(Extent orDefault)` where appropriate to reduce block checks
 - fixes #2679
 - fixes #1874

(cherry picked from commit 6052fc3)
 - cannot remember why I made this change in the first place in #2773 but this fixes edits having "empty middles"
 - doesn't seem to have broken anything in testing
@dordsor21 dordsor21 force-pushed the fix/full-chunk-section-filter branch from 08f401f to 43b0e40 Compare September 15, 2024 15:25
@github-actions github-actions bot added the Bugfix This PR fixes a bug label Sep 15, 2024
@dordsor21 dordsor21 merged commit d9d5b7b into main Sep 17, 2024
11 checks passed
@dordsor21 dordsor21 deleted the fix/full-chunk-section-filter branch September 17, 2024 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bugfix This PR fixes a bug Feature This PR adds a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

max-blockstates config limit is not enforced on //set
3 participants