Skip to content

2.0 perf updates #7543

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

Merged
merged 13 commits into from
Feb 13, 2025
Merged

2.0 perf updates #7543

merged 13 commits into from
Feb 13, 2025

Conversation

davepagurek
Copy link
Contributor

@davepagurek davepagurek commented Feb 12, 2025

Resolves #7539

  • Switch to shallow cloning in push/pop + copying before modification of state values
  • Refactors push/pop to only update changed properties
  • Creates a States class to keep track of what values have been set
  • Switches from states.key = value to states.setValue('key', value) because using a getter/setter was noticeably slower
  • Makes sure p5.Matrix values are always Float32Arrays
  • Removes structuredClone from color maxes
  • Puts cached stroke/fill style values into states to avoid having to read them from the canvas again on pop

To do:

  • Make serialize for fills faster

@davepagurek davepagurek marked this pull request as ready for review February 13, 2025 03:02
@davepagurek
Copy link
Contributor Author

@limzykenneth This should be testable now!

I had to update some screenshots for one text assignment test, but they were only failing on CI. Looks like some text was very close to wrapping and some change made it just different enough to not wrap on CI. I updated the font size for that test to not be right on the edge like that, which feels less clean than I'd like, but... I'm no closer to figuring out what shall detail changed than I was an hour ago :')

@davepagurek
Copy link
Contributor Author

ok TIL that test is failing on dev-2.0 too currently. I think that test flake is unrelated to this PR, and possibly even caused by unrelated CI dependency changes? but anyway that makes me feel more confident that this PR didn't break it!

@dhowe
Copy link
Contributor

dhowe commented Feb 13, 2025

still seeing this issue on PRs

@limzykenneth
Copy link
Member

I was thinking since the class have some similarity to Map would it be worth to implement it as an inherited class from Map but I think that probably will complicate things and not give much benefit.

It seems the test is also passing here now so I guess if we merge this it should fix the test on the branch and other PRs as well?

Still not efficient enough as color object are frequently created
and each will need to be serialized once regardless of how many
times it is used.
@davepagurek
Copy link
Contributor Author

@limzykenneth feel free to merge when the color serialization is ready!

@limzykenneth
Copy link
Member

Still trying to figure out a more comprehensive cache for color object in general. That's because with each color object created, it will need to be serialized once first before it will be memoized but with each call to any function that takes simple number arguments, it will create a one time used color object which erases the memoization benefit.

If you have an idea around here do share, I'll probably spend a bit more time on it and if I still have nothing I'll merge this for now.

@limzykenneth
Copy link
Member

For some reason I'm not getting the full 60 fps on my linux PC (getting around 20 fps) not even with 1.11.3 (which gets about 30 fps), while on my Macbook Pro I'm able to get full 60 fps. Surely are Macs that good now?

Anyway, that's beside the point. We can continue looking into other avenue for more optimizations but if it isn't too hard to get 60 fps on the original sketch it should be ok for now.

@limzykenneth limzykenneth merged commit 3a74317 into dev-2.0 Feb 13, 2025
2 checks passed
@limzykenneth limzykenneth deleted the perf branch February 13, 2025 23:04
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