Skip to content

Add specialized Vector4.translate methods based on argument types #345

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

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

Conversation

osa1
Copy link
Member

@osa1 osa1 commented Apr 16, 2025

Matrix4.translate is used by Flutter and it often appears as an overhead when
profiling Flutter apps compiled to Wasm.

The function currently takes a dynamic argument and takes different code
paths based on the type.

This is inefficient when the call site already knows the argument's type.

In general, when we have a performance-critical generic function that checks
types of the arguments, it makes sense to introduce specialized versions of the
function based on precise argument types that it handles, so that callers can
call the more efficient specialized versions and avoid the type test overheads.

This PR introduces such functions for Vector4.translate based on argument types:

  • Vector4.translateByDouble(double x, double y, double z)
  • Vector4.translateByVector3(Vector3 v3)
  • Vector4.translateByVector4(Vector4 v4)

Once these functions are released, we can update Flutter to call these to
improve performance.

dart2wasm benchmarks:

Matrix4.translateByDoubleGeneric(RunTime): 6.266987466025068 us.
Matrix4.translateByVector3Generic(RunTime): 10.928450821971301 us.
Matrix4.translateByVector4Generic(RunTime): 13.0875 us.
Matrix4.translateByDouble(RunTime): 6.204236040468909 us.
Matrix4.translateByVector3(RunTime): 6.122987754024492 us.
Matrix4.translateByVector4(RunTime): 5.843739773455397 us.

dart2js benchmarks:

Matrix4.translateByDoubleGeneric(RunTime): 6.119968380163369 us.
Matrix4.translateByVector3Generic(RunTime): 9.309969742598337 us.
Matrix4.translateByVector4Generic(RunTime): 10.578705040503579 us.
Matrix4.translateByDouble(RunTime): 6.101644293970922 us.
Matrix4.translateByVector3(RunTime): 6.071987856024288 us.
Matrix4.translateByVector4(RunTime): 6.8984827537931155 us.

AOT benchmarks:

Matrix4.translateByDoubleGeneric(RunTime): 4.126535341830823 us.
Matrix4.translateByVector3Generic(RunTime): 6.292811414377171 us.
Matrix4.translateByVector4Generic(RunTime): 6.593214665267003 us.
Matrix4.translateByDouble(RunTime): 4.082966 us.
Matrix4.translateByVector3(RunTime): 5.4163575 us.
Matrix4.translateByVector4(RunTime): 5.4935925 us.

JIT benchmarks:

Matrix4.translateByDoubleGeneric(RunTime): 4.127742 us.
Matrix4.translateByVector3Generic(RunTime): 8.169820863171763 us.
Matrix4.translateByVector4Generic(RunTime): 7.588828013964982 us.
Matrix4.translateByDouble(RunTime): 4.092894 us.
Matrix4.translateByVector3(RunTime): 4.088788 us.
Matrix4.translateByVector4(RunTime): 4.72452834433957 us.

@coveralls
Copy link

coveralls commented Apr 16, 2025

Coverage Status

coverage: 26.149% (-0.2%) from 26.372%
when pulling 6814af2 on osa1:faster_vector4_translate
into 39cafd4 on google:master.

@osa1 osa1 marked this pull request as ready for review April 16, 2025 11:44
@osa1
Copy link
Member Author

osa1 commented Apr 16, 2025

Somehow I can't add reviewers to the PR.

@mkustermann @eyebrowsoffire could you have a look?

@spydon
Copy link
Collaborator

spydon commented Apr 16, 2025

Maybe it's time to make a breaking release soon instead? Then I would remove all the dynamic arguments from all the classes, and we would also follow semver for #270

@osa1
Copy link
Member Author

osa1 commented Apr 16, 2025

This change doesn't need to be a breaking change. We can add a deprecation to translate though as I would think that it's too slow for what it's intended for.

@spydon
Copy link
Collaborator

spydon commented Apr 16, 2025

This change doesn't need to be a breaking change. We can add a deprecation to translate though as I would think that it's too slow for what it's intended for.

Yeah, it wouldn't be a breaking change right now. But if following semver, marking it as deprecated is just the first step towards a breaking change, when removing the deprecated method major should be bumped (https://semver.org/#how-should-i-handle-deprecating-functionality).

I'm not too fond of all the byType methods, but I also can't come up with a better solution, and it is definitely better than taking dynamic as an argument.

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.

3 participants