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

Introduce basic support for Vector API #2890

Merged
merged 3 commits into from
Sep 15, 2024
Merged

Conversation

SirYwell
Copy link
Member

Overview

Description

This is an experimental, internal feature to use the Vector API when enabled. The current implementation is designed to cover basic use cases (i.e. //gmask with single blocks, //set with single blocks, //replace with single blocks). This could be expanded, but as that needs to be done rather careful to not make performance worse, I left it to the very basics for now.

A quick example of running //replace dirt diamond_block on a selection of 2048x320x2048:

Before:
before

after

Note that the hasSection calls take up the same time in both profiles, same for initLayer. The difference is in the filter call, which results in many additional calls in the none-specialized variant vs pretty much only memory access and vector instructions in the specialized variant. (There might be ways to optimize without the Vector API here I guess, but the existing abstraction doesn't work well in either cases)

A few more notes on the benchmark:

  • I limited FAWE to use 4 threads only, because that's what many people actually have (and FAWE is just comically fast otherwise)
  • It might make sense to set the number of threads to the half of the actual hardware threads, as some platforms are known to not perform well with vector instructions otherwise - I don't know how much that might skew the result
  • I'm running a machine with AVX2 (256 bit vectors) only, machines with AVX3/AVX512 might perform even better
  • The difference is not really relevant on small edits

Submitter Checklist

@SirYwell SirYwell requested a review from a team as a code owner August 21, 2024 14:19
@github-actions github-actions bot added the Feature This PR adds a new feature label Aug 21, 2024
@dordsor21
Copy link
Member

Short rather than int?

@SirYwell
Copy link
Member Author

In its current state, it would be difficult to use IntVector, as we can only use ShortVector to load from char[]. It might make sense to design with other element types in mind. I think #2181 would probably make things easier there too. Alternatively using the Foreign Memory API would be less restrictive, but that's only a final feature since Java 22.

@SirYwell
Copy link
Member Author

(Also vector operations are probably one of the few places where element size actually has an impact on performance, so keeping it as small as possible makes sense)

@dordsor21
Copy link
Member

dordsor21 commented Sep 1, 2024

In its current state, it would be difficult to use IntVector, as we can only use ShortVector to load from char[]. It might make sense to design with other element types in mind. I think #2181 would probably make things easier there too. Alternatively using the Foreign Memory API would be less restrictive, but that's only a final feature since Java 22.

Yeah I suppose a lot of this would be best duplicated as the data array PR does so we can maintain the smaller size "optimisation" where possible

Also looks like some changes are needed to the CI/PR run config

@SirYwell
Copy link
Member Author

SirYwell commented Sep 1, 2024

Yeah I suppose a lot of this would be best duplicated as the data array PR does so we can maintain the smaller size "optimisation" where possible

Yes, but I intend to leave it like this for now, as it is experimental anyway.

Also looks like some changes are needed to the CI/PR run config

That should be fixed now, the javadoc task was failing as it requires the --add-modules flag too.

Copy link

Please take a moment and address the merge conflicts of your pull request. Thanks!

@NotMyFault NotMyFault merged commit ea5589b into main Sep 15, 2024
11 checks passed
@NotMyFault NotMyFault deleted the feature/experimental/simd-mask branch September 15, 2024 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature This PR adds a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants