Closed ofZach closed 8 years ago
note @shiffman may have more insight here, we were going back and forth about this on the SFPC slack -- since we are doing some pixel operations in class the slowness of these functions was really felt (ie, it's the difference between 2 fps and 30 fps on an updating image of static noise, etc.
Very interesting problem! I tried to isolate the problem but just couldn't as it seem the very act of assigning random()
to map to Math.random()
is where the performance hit occurs. I suspect the way the browser optimise these things could have something to do with it and here's my findings:
On Safari: ~10 times the difference
On Firefox: Erm...pretty bad...
On Chrome: Much closer, ~1.5 – 2 times difference but native functions slower than other browsers
Here is some additional info that I discovered with some tests. If I simply call Math.random()
from another function it's fast:
function setup() {
// fast!
var now = millis();
for (var i = 0; i < 1000000; i++) {
var r = myRandom();
}
console.log(millis() - now); // ~9 millis
}
function myRandom() {
return Math.random();
}
But if I attach myRandom()
to the p5 prototype it's quite a bit slower.
function setup() {
// fast!
var now = millis();
for (var i = 0; i < 1000000; i++) {
var r = myRandom();
}
console.log(millis() - now); // ~150 millis
}
p5.prototype.myRandom = function() {
return Math.random();
}
yeah, I think what we're seeing here is that in JS, both function calls and object lookup are expensive. so it does add time to make a call to a p5 function that calls an underlying JS function, and even more time if you are accessing the property of p5.prototype
, rather than the one attached to window
. in addition, we're doing extra checking to handle multiple inputs and prevent strange output, see random() for example.
my suggestion is when going for highest performance, to use the native Math
and built-in JS functions over p5 ones whenever possible.
I'll take another look at these also and see if there's any extra fat in there we can cut.
more here:
Ditto to this being an interesting problem.
As far as I can tell, most of the slowdown is because random()
is actually a bound version of p5.prototype.random
. See core.js, line 467. (Ref for bind being slower: stackoverflow)
That binding doesn't happen with p5 in instance mode, so you should be able to speed up most p5 method calls purely by switching over to using p5 in instance mode. (Of course...not a helpful teaching solution.)
100000000 iterations, Chrome, Windows
Math.random: 1193.92ms
p5 random: 2580.10ms
Calling a bound p5.random.prototype: 2611.76ms
Calling p5.random.prototype directly: 1535.50ms <- No bind involved
p5 random in instance mode: 1571.19ms <- No bind involved
Edit: These results are all from using p5.min.js. It's worth noting using the unminified p5.js adds a ~10x slowdown (14165.22ms
in unminified vs 2580.10ms
in minified). That's because in unminified p5, the global properties are looked up via a Object.defineProperty getter (core.js line 593). So if speed is a problem, best to use the minified p5.
var iterations = 100000000;
function setup () {
// -- MATH.RANDOM --
var start = performance.now();
for (var i = 0; i < iterations; i += 1) {
Math.random();
}
var elapsed = performance.now() - start;
console.log("Math.random: " + elapsed.toFixed(2) + "ms");
// -- GLOBAL P5 RANDOM --
var start = performance.now();
for (var i = 0; i < iterations; i += 1) {
random();
}
var elapsed = performance.now() - start;
console.log("Global Mode p5 Random: " + elapsed.toFixed(2) + "ms");
// -- DIRECTLY CALLING P5.PROTOTYPE.RANDOM --
var directRandom = p5.prototype.random;
var start = performance.now();
for (var i = 0; i < iterations; i += 1) {
directRandom();
}
var elapsed = performance.now() - start;
console.log("p5.random.prototype: " + elapsed.toFixed(2) + "ms");
// -- DIRECTLY CALLING P5.PROTOTYPE.RANDOM --
var boundRandom = p5.prototype.random.bind({});
var start = performance.now();
for (var i = 0; i < iterations; i += 1) {
boundRandom();
}
var elapsed = performance.now() - start;
console.log("Bound p5.prototype.random: " + elapsed.toFixed(2) + "ms");
}
// -- INSTANCE MODE P5 RANDOM --
new p5(function (p) {
p.setup = function () {
var start = performance.now();
for (var i = 0; i < iterations; i += 1) {
p.random();
}
var elapsed = performance.now() - start;
console.log("Instance Mode p5 Random: " + elapsed.toFixed(2) + "ms");
}
});
As far as I can tell, most of the slowdown is because
random()
is actually a bound version ofp5.prototype.random
. See core.js, line 467.
There is no reason that .random would need to be bound, given that it doesn't ever utilize this
; maybe we should let methods flag themselves as _contextNeeded
or something so that we can conditionally skip the .bind call when creating the global method
I tried experimenting with some bind alternatives (for example, the bind implementation from fast.js), but nothing really helped substantially on Chrome for this situation.
Right - each function could have something like p5.prototype.random._contextNeeded = false
underneath the function declaration. That would certainly speed things up, though it has a chance of causing some development pains.
Re: random not needing a context. I remember this open issue from a while back (#1135) - when seeding the random generator, the generator function is shared across all sketch instances. So, generating seeded random numbers in one sketch instance affects the other sketch instances. To make sketch instances independent, I could see an argument for random
actually needing a context when the generator is seeded.
I tried experimenting with some bind alternatives (for example, the bind implementation from fast.js), but nothing really helped substantially on Chrome for this situation.
I suspect that have to do with very long for
loops not being as optimised in Chrome (or rather V8), probably by choice from the V8 team. Just guessing though.
@limzykenneth hm, I didn't know that. I also tried some other things, but this was the fastest I could come up with:
var methodBind = function (pInst, methodName) {
return function () {
return p5.prototype[methodName].apply(pInst, arguments);
};
};
...
friendlyBindGlobal(p, methodBind(this, p));
100000000 iterations, Chrome, Windows
Math.random: 1234.54ms
p5 random (w/o above fix): 2580.10ms
p5 random (with above fix): 1822.40ms
p5 random in instance mode: 1571.19ms
is about ~1.42x faster than calling a bound function, but it's still not quite as fast as p5 instance mode. Cost of a function from @lmccart's refs plus the cost of an apply. If that speed boost seems worthwhile, I can do some testing and submit a PR.
Either way, maybe it would be useful to create a wiki/tutorial with performance tips. I'd be happy to contribute to that.
@mikewesthad a wiki page / tutorial with performance tips would be fantastic! and if the patch doesn't add significant complication and isn't too difficult to implement, I'd say that's worth it for the boost.
great to see the progress on this thread but just a quick note that everyone has focused on random()... but don't forget sin() ! I understand the complication with random() (needing a seeded RNG, etc) but couldn't sin() be made a lot faster ? Also, are there any other bound functions that could be sped up?
@mikewesthad The improvements seemed reasonable, would like to see it implemented here (and on potentially all bound functions as @ofZach mentioned, but we should test each of them individually first I think).
Wiki sounds great as well, it can also serve as a simple starter tutorial on optimising code performance.
I'll work on a patch and starting a wiki when I get some time this week.
@ofZach & @limzykenneth, sorry if I didn't make that clear, the change would be for all bound methods that are being put into the global scope. Every call from the global scope into a p5 method should be faster (minus the event handlers, which aren't bound). How much of a relative speed boost would depend on the specific method.
Unfortunately the trig functions also need a context - to look up the angle mode. But those methods that are just calling the Math lib equivalent (e.g. abs
) don't need a context. That brings things back to @kadamwhite's suggestion.
Anyway, here's what happens with sin:
100000000 iterations, Chrome, Windows
Sin in radians with values from 0 to 3.14:
Math.sin: 2301.04ms
p5 sin (w/o bind fix): 3315.28ms
p5 sin (with bind fix): 2827.04ms
p5 sin in instance mode: 2459.42ms
Sin in radians with values from 0 to 314:
Math.sin: 3597.92ms
p5 sin (w/o bind fix): 4717.18ms
p5 sin (with bind fix): 4058.47ms
p5 sin in instance mode: 3817.85ms
So around 1.17x speed boost with the fix. Unfortunately, if you use degrees for angle mode, the speed boost is pretty minimal because the method itself take longer to execute (saving ~500ms doesn't mean as much relative to 5000ms as it does relative to 3000ms).
@ofZach I know these aren't the best for teaching, but are the other tips useful for your students who are running into performance issues: using p5.min.js over p5.js and/or using p5 in instance mode? Both of those drastically speed up bound method calls.
Just a quick note that I am seeing a 10x different (osx / chrome) between these:
Math.sin: 2301.04ms
p5 sin (w/o bind fix): 3315.28ms
I wonder if it makes sense to check these fixes on OSX as well? I feel like the windows platform might not be exposing the dramatic discrepancies and it's prob good to get a holistic view on improvements.
I feel my general advice to students would be to use Math.XX everywhere when possible. This is mostly because we are doing pixel / image processing things and the hits you get in the for loop are pretty dramatic and from my experience the difference between 2fps and 30fps...
It might be good to add performance notes to the reference for these bound functions if there's a succinct write up to point to -- I feel like for most people they won't notice this, but if the difference remains dramatic it's useful to point it out. (as a side note, the same thing happened recently with ofGetElapsedTimef() due to a change, it became dramatically slower which made itself evident in huge for loops, etc)
@ofZach any chance you are using are you using the unminified p5? Unminified causes a 10x slowdown (on windows/chrome) compared to minified due to the extra layer of going through a getter function. Regardless, going with Math.XX makes sense.
Yeah, definitely makes sense to check cross-platform and cross-browser.
I am just using this
@ofZach Ah! That explains it. The p5 editor uses the unminified p5 file. I'm guessing Win/OSX are pretty close in performance then since p5's sin/random take 10x time on Win/Chrome when using unminified p5.
I took a crack at an initial patch in #1519; it could use testing across browsers and environments, I've only done some cursory spot checks.
Very interesting discussion. Since this 'binding' is not exposed to the p5 end users, isn't it possible instead of using Function.prototype.bind
, use a custom one, stripping out all the overhead for handling edge cases? Something like:
function p5CustomBind(scope) {
var fn = this;
return function () {
return fn.apply(scope);
};
}
@sepans, yep - exactly, that's what I proposed, and I am working on. @kadamwhite's PR plus a custom bind should give a boost to most methods, whether they need a p5 context or not. (Though the p5 editor's performance hit is another issue and I'm not sure it would be fixed by these.)
is there a reason for the editor to use non minimized p5.js ?
also, It seems like it should be flagged if there's a big speed difference between minified and non-minified p5.js. Also, is there a way to reduce this? (I was always under the impression that there wasn't much speed difference, maybe that's a misconception: http://stackoverflow.com/questions/1181447/does-minified-javascript-improve-performance)
Currently the language on the getting started says:
(compressed for faster page loading)
Maybe it could be tweaked like:
(compressed for faster page loading and faster performance)
with the faster performance linking to some tips for speed? Just a thought since this kind of caught me off guard...
[_EDIT:_ @mikewesthad your next post clarified this]
@mikewesthad Great! I might have missed something but as I understood it in https://github.com/processing/p5.js/pull/1519 only functions which don't use p5 context (and flagged with '_context = false') are spared from being bound. What I meant was binding every function with the lite custom bind here (e.g.
friendlyBindGlobal(p, p5.prototype[p].p5CustomBind(this));
). If avoiding Function.protptype.bind
overhead really helps, this way methods like random
which use context will also be faster.
@ofZach the thing about min vs non-min was something I only observed when I started looking into the p5 source at the start of this issue. I was surprised too. It's not a JS-specific issue, it's a p5-specific issue. I'm not sure it was a known issue until now. I mentioned it briefly in this thread a couple days ago, but this thread is pretty long, so here's some more detail:
In the non-min p5, the global properties are looked up via a Object.defineProperty getter/setter (core.js line 593). As far as I can tell, it's set up that way so that if someone tries to override a p5 global (like random
or sin
), a nice warning can be logged to the console. This seems like a nice feature, but it causes a 10x slowdown. Calling Math.random
vs Math.random
via a getter, it's 1,238.78ms
to 12,537.6ms
(code below). I'm guessing the editor is set up to use the non-min for the ability to log a warning.
It might be time to break this issue into some smaller targeted issues... So to summarize the salient points now:
I don't have time at the moment to look into item 1, but can come back later if someone hasn't already jumped on it.
var iterations = 100000000;
// -- MATH.RANDOM --
var start = performance.now();
for (var i = 0; i < iterations; i += 1) {
Math.random();
}
var elapsed = performance.now() - start;
console.log("Math.random: " + elapsed.toFixed(2) + "ms");
// -- MATH.RANDOM VIA GETTER --
Object.defineProperty(window, "getRandom", {
configuable: true,
enumerable: true,
get: function () {
return Math.random;
}
});
var start = performance.now();
for (var i = 0; i < iterations; i += 1) {
getRandom();
}
var elapsed = performance.now() - start;
console.log("Math.random via Getter: " + elapsed + "ms");
@sepans - missed your comment. No worries, this thread is getting to novel-length. There were two bind fixes being proposed: @kadamwhite's fix of removing bind when not needed; and my fix of replacing native bind with a custom function (this comment)
I'm becoming increasingly convinced that the getters are more significant than the bind, but I'll leave it for independent review of #1519 to decide :)
If the performance problem is with minified vs unminified I think the lower performance is expected of the unminified version not because of the difference between minified and unminified JS (should not be that much difference in execution) but probably because of the extra logging and checks that is removed from the minified version. These are also part of the friendly error system which will only be present on the unminified system. That's what I suspect anyway.
Quite the long discussion this one.... 😉
I don't know if this is plausible but is it possible instead of having user friendly logs and checks in the main source code, have another script that injects/decorates them into the source code?
In that case having those checks is a matter of adding another <script>
tag into the html, or in case of the p5 editor, dynamically loading it when user sets it in the settings, regardless of p5 code being minified or not. This could potentially solve the 10x performance problem for the p5 editor.
Great discussion everyone! I've taken @mikewesthad's breakdown of points and opened some new issues we can follow up on:
The original issue (from @ofZach) of the large (10x) slowdown happens when using the non-min p5.js in global mode. That is likely due to the getters/setters being used when accessing p5 global properties. (In the p5-editor, there might be something else causing the slowdown as well?) This now seems like the pressing issue.
- friendly errors and un/minified continued in #971
- evaluate performance effects of
.bind
used to attached global properties vs the object property getters/setters #1519Some p5 methods don't need any binding at all in global mode. @kadamwhite's PR removing binding from methods that don't need it will yield some speed boost. I'm guessing based on the results in this thread, maybe 1.5x. But this won't fix the above 10x slowdown.
1519 addresses this, to be merged in if determined the performance optimizations are relatively worth it
Binding in p5 can be replaced with a custom bind to produce some modest speed improvements for methods that do need binding. I've got a patch for that. Again, won't fix the 10x slowdown.
- binding follow up in #1522
Wiki/tutorial on performance as a starting point for anyone who needs to squeeze more power out of p5.
- wiki follow up in #1523
We need some way of testing p5 performance cross-browser and cross-platform.
- performance testing in #1524
If I've covered everything, I'd like to close this issue so we can continue more focused conversations in the proper places. If I've missed some issue or there's more to elaborate on any of them, please feel free to add comments to the individual issue threads or open new ones.
@lmccart looks like everything to me - thanks for opening those issues
Test code:
I find about a 10x speed improvement using Math.random() and Math.sin()