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

[v9] fix: prefer named functions, for loops in hot paths #2540

Merged
merged 4 commits into from
Sep 29, 2022

Conversation

AlaricBaraou
Copy link
Contributor

Following the discussion here https://discord.com/channels/740090768164651008/740093168770613279/1023859404111613962

I went to add a name function to remove this anonymous function from the performance tool report
ForEach

I also thought that we could replace the forEach with for loops where applicable.
With the for loop there isn't a need to use a named function at all in the end.

So I went through the files and replaced the forEach loop that looked safe to replace.

I'm expecting a gain of 1µs per frame with that

The perf gain is probably neglectable but it removes some noise from the performance and also use less memory for one of them that was creating a new array on each call ( cancelPointer )

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 28, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit badeacf:

Sandbox Source
example Configuration

@CodyJasonBennett
Copy link
Member

Looks good. Did the same for events so the flame graph is consistently traceable.

Is there a reason for targeting v9 or is this something you'd want in v8?

@AlaricBaraou
Copy link
Contributor Author

I just saw a comment from Paul asking to target V9
If it can also be available on V8 I'm all for it.

I initially made the change for V8, just made the PR if you want to use it. #2541

I didn't run the lint / tests on V8 either but the exemple seemed to run normally.

@CodyJasonBennett
Copy link
Member

No worries. I'm happy to backport this as a fix since it's affecting quality control.

@CodyJasonBennett CodyJasonBennett changed the title replace forEach to for loop [v9] fix: prefer named functions, for loops in hot paths Sep 29, 2022
@CodyJasonBennett CodyJasonBennett merged commit 3bb9da4 into pmndrs:v9 Sep 29, 2022
@CodyJasonBennett CodyJasonBennett mentioned this pull request Sep 29, 2022
18 tasks
@AlaricBaraou AlaricBaraou deleted the remove-forEach branch September 29, 2022 01:06
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.

2 participants