-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Optimize bottlenecks in 2.0 code #8094
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
Conversation
src/core/main.js
Outdated
const boundFunction = p5.prototype[property].bind(this); | ||
Object.defineProperty(window, property, { | ||
configurable: true, | ||
enumerable: true, | ||
get() { | ||
return boundFunction; | ||
}, | ||
set: createSetter() | ||
}); | ||
} else if (isConstant) { | ||
// For constants, cache the value directly | ||
Object.defineProperty(window, property, { | ||
configurable: true, | ||
enumerable: true, | ||
get() { | ||
return constantValue; | ||
}, | ||
set: createSetter() | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for this part, we can branch it and optimize more when FES is disabled, since the getter is just returning the value and the setter is just binding the FES overwrite message (which do we need provided we have the sketch verifier?), we can check if FES is not enabled then do a more direct binding.
if(p5.disableFriendlyErrors){
Object.defineProperty(window, property, {
configurable: true,
enumerable: true,
value: boundFunction
});
}else{
Object.defineProperty(window, property, {
configurable: true,
enumerable: true,
get() {
return boundFunction;
},
set: createSetter()
});
}
From my test, it should be equivalent in performance to instance mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My tests also consistently have 2.x being slightly faster than 1.x with these relevant optimizations or just comparing instance mode. I mainly use console.time()
console.timeEnd()
just so that I'm not using anything p5.js internal to do the measurement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea! added some more branches for that.
@davepagurek I was just testing this and I found that 4e145ca has broke parameter validation as it currently only works with I think functions within the core modules only. I'm planning on changing the way parameter validation function decoration works as I have figured out why it has so much performance hit but need to solve this first. Edit: Caching the bound function itself seems to be the problem as the cached bound function is detached from |
@limzykenneth could this possibly be an ordering issue? FES currently does the function validation wrapping in presetup, could we move the global binding to presetup too so that the order is like:
...since addons presumably shouldn't be directly using globals anyway, and user sketch code should only be happening in setup onwards. |
Resolves #8017
Resolves #8013
This implements some of the changes discussed in the above two issues.
Previously, this was the performance profile of time spent in functions:

After these changes, it looks like this:

According to those, the largest problems were:
On this test sketch, starting with a base runtime of 420ms, here are the optimizations I made and the new runtimes afterwards:
p5.prototype
and constants that we make global: 315msget()
s inp5.Vector
(using.values
when._values
is the same): 299ms.map()
s inp5.Vector
constructor: 292mstypeof
s + caching bound functions in global non-p5.prototype
functions: 250msFor comparison, when using 1.11.0, the performance is 215ms. So this is still not quite at 1.x perf, but it's a lot closer.
Takeaways
Dynamic getters are generally slower than property accesses! We should consider whether they're really necessary when we use them if it's something that will be accessed a lot, e.g. a constant.
Not from these particular tests, but at work we experimented in the past with using a
Proxy
to wrap p5 in order to get multiple global mode sketches to be able to run in a shared environment without them knowing, but found base performance proxying all p5 methods to be significantly worse. Rewriting the sketch code to use direct accesses gave comparable performance to regular global mode, despite the proxy methods all being pretty simple. So Proxies aren't an across-the-board replacement for dynamic getters either without measuring. In general we should just try to make as much as possible non-dynamic when it isn't necessary.