Fixes #9000 - #9014
Fixes #9000#9014NalinDalal wants to merge 17 commits into
Conversation
|
To show that this fixes the original issue, please report results from the linked benchmark test, at minimum. Thanks! |
perminder-17
left a comment
There was a problem hiding this comment.
The results are good, just one concern for the cleaner code.
| }); | ||
|
|
||
| return coords; | ||
| this.#rgbaCache.set(cacheKey, result); |
There was a problem hiding this comment.
I don't think we need this Map(). The only heavy step is the 'srgb' conversion you did above, so we can just cache that part, less complexity and cleaner code.
| } | ||
|
|
||
| _getRGBA(maxes=[1, 1, 1, 1]) { | ||
| const cacheKey = maxes.join(','); |
There was a problem hiding this comment.
maxes.join(',') allocates a new string on every call inside loops. Using a fast path key check for [255, 255, 255, 255] avoids string allocations during pixel loops.
|
|
||
| coords = coords.map((coord, i) => { | ||
| let coords; | ||
| if (this.mode === RGB) { |
There was a problem hiding this comment.
Use this._color.space.id === 'srgb' instead of this.mode === RGB makes the fast-path work for any color object backed by sRGB. It ensures that any color whose underlying color space is sRGB will bypass colorjs conversion and structuredClone, even if the color mode was initialized via another path.
|
All feedback are addressed and solved, thanks for your input @Ayush4958 and @perminder-17 |
ksen0
left a comment
There was a problem hiding this comment.
Nice work, thanks ! And thanks to reviewers!
limzykenneth
left a comment
There was a problem hiding this comment.
If I read the changes correctly, is it essentially memoizing the _getRGBA function? The implementation should be ok and functional but having a #invalidateSrgbCache can be a bit brittle in that we need to make sure that #invalidateSrgbCache is called on all possible code paths (including in the future) that changes the color somehow. If it is just some form of memoizing, can we create a memoization whose key is the color coordinates? Similar to what toString() uses ${this._color.space.id}-${this._color.coords.join(',')}-${this._color.alpha}-${format}?
There was a problem hiding this comment.
This file shouldn't be committed as it does not match the required format of the benchmark harness.
…es; remove all calls to #invalidateSrgbCache
limzykenneth
left a comment
There was a problem hiding this comment.
Looks good for now. If you can do a test with benchmarking to see if it affect performance or not that would be great. After that we can merge.
| if (srgbCoordsCache.size > 1000) { | ||
| srgbCoordsCache.delete(srgbCoordsCache.keys().next().value); | ||
| } |
There was a problem hiding this comment.
Good to have this. I wonder if 1000 is the right number? What would a typical number of colors set() would manipulate and what would be a sensible upper bound without it leaking memory?
There was a problem hiding this comment.
Typical sketches use far fewer than 1000 unique colors. However, image processing or complex generative art could exceed this. Each entry is small (~32 bytes), so 1000 entries ≈ 32KB max. I chose 1000 as a conservative default that covers most cases without significant memory impact. Would you prefer a different value or should we make it configurable?
There was a problem hiding this comment.
We can keep it as 1000 for now if the above benchmark still holds.
Fresh benchmark results for commit c15b88a (key-based memoization):p5.js Color Memoization Benchmark (commit c15b88a)
|
|
@NalinDalal I've just tried to validate the performance with @SableRaf sketch in #9000 and found that this PR performs worse than 2.3.2: https://editor.p5js.org/limzykenneth/sketches/a3vi9QIRy the two versions can be commented in and out in |
ohh, seems I need to iterate over it again, will update you soon. |
|
Removed:
Kept:
Why the cache was removed:
Removing the cache improved performance by ~7-8% for the All unit test passed |
|
Now the improvements seems very minimal (I can't observe the difference in the example sketch) and could have been fluctuations/inaccuracy in the measurement. I think the core issues need to be re-examined on where the performance hit is and how it can be addressed. |
|
Actually I think the problem here is that current head enables FES even when using minified version (I have still yet to finish work on the FES stuff) which causes the slow down because the bottle neck is now on FES calls. If we set |
|
Re-measured after the FES exclusion in main (#9212 / 65b6f76). Both sides here include that commit, so FES isn't a variable. vitest bench, the bench added in this PR, 100×100 buffer × 50 frames, chromium. #processing/p5.js@e1b320b (merge-base) vs #NalinDalal/p5.js@6067a9e (head). FES off (
FES on (default in the bench):
~3.8× with FES off, ~3.7× with it on, consistent across both browser projects and repeat runs (rme ±0.8–3% on the FES-off rows). Two caveats on the numbers. The absolute figures are much higher than the issue's because this harness builds a On the earlier "performs worse than 2.3.2" report: my measurement had FES on both sides, and disabling it moves both branches only slightly (1.11 → 1.32 Hz baseline, 4.34 → 4.92 Hz here) without changing the ratio. So FES doesn't appear to be the dominant cost in this path. Happy to be wrong about that, if you still see a regression I'd like to see the numbers, since I can't reproduce it. |
Mirrors the minified build, where FES is excluded from the bundle, so the _getRGBA cost can be measured without Zod validation on top of it. Sets and restores the flag inside the timed callback to avoid leaking into other benchmark files sharing the process.
|
@Ayush4958 good catch on the string allocation, that one's worth having. On There's also a correctness reason to keep Happy to switch if you think the space-based check is safer , just want to make sure I'm not breaking the P3 case. |

Resolves #9000
set()onp5.Graphicswas significantly slower in 2.x vs 1.11.x because_getRGBAdid a fullto('srgb')+structuredClone+map()on everycall, even when the color was already in RGB mode and being reused in a
tight loop.
Changes:
_getRGBAresults#rgbaCacheMap onColor, keyed bymaxesstringmaxesreturn a shallow copy of the cached array
setRed,setGreen,setBlue,setAlpha,and the
_colorsetterthis.mode === RGB, coords are already sRGB 0-1, so skip theto('srgb')conversion and use[...this._color.coords, this._color.alpha]directly
AI usage disclosure: I used Kilo (an AI coding assistant) to help
implement and review this change. I understand and take responsibility for
every line of code in this PR. See [AI_USAGE_POLICY.md].
PR Checklist
npm run lintpasses