A question about some Phaser API changes

[rich]

Hi all,

 

So today I've been implementing Pixi 1.5 into Phaser and it's going really well. I have managed to cut down some of the classes considerably (Group for example is now 600 lines shorter than before, and lots more powerful!)

 

However Mat and I were chatting and doing some benchmarks and it turns out that Object.defineProperty is incredibly slow on mobile. This is the way in which you define getters and setters in JS. For example:

Object.defineProperty(Phaser.Sprite.prototype, 'angle', {    get: function() {        return Phaser.Math.wrapAngle(Phaser.Math.radToDeg(this.rotation));    },    set: function(value) {        this.rotation = Phaser.Math.degToRad(Phaser.Math.wrapAngle(value));    }});

The above allows you to do:

sprite.angle += 10;

.. and it "just works". However, it's incredibly slow on mobile. On Safari for example using defineProperty is about 95% slower than accessing a property directly. We're talking just 900 ops/sec - which really isn't a lot.

 

On desktop browsers compared to accessing a normal object property it comes in a lot slower too. There are lots of jsperf tests here which I've been running across mobile and desktop: http://jsperf.com/object-defineproperty-vs-definegetter-vs-normal/

 

I don't think I need to stop using getters/setters entirely though. For example for things that aren't called in core loops they are really nice (Sprite.inputEnabled for example). But in instances where I'm checking the values every single frame (x/y/frame/angle) it becomes a much bigger issue. For example I use them a lot in the geometry classes like Rectangle - which are often then checked in classes like Sprite.

 

So I'd like to cut back on these as much as possible, for any situation where it's a property you're likely to want to check or update every frame. However it means some things may have to become functions, for example:

sprite.angle(); // returns '10'sprite.angle(45); // sets '45'

This drops the ability to just increment the angle value directly, but would retain the way it handles degToRad and wrapping of the value for you. On Mobile Safari for example the above would run at around 5,500 ops/sec compared to 900 ops/sec as it does now. But even 5,500 pales compared to a direct angle property at 14,000 ops/sec - but then you don't get any of the 'niceness' built in that way.

 

What do people think? I would very much like to improve performance of Phaser and I think changes like this, across the board, could achieve that. There are other much bigger gains to be made too, in different areas, that may have more of an overall impact than anything done on this level, but it all adds up.

 

I just don't want to get away from the 'ease of use' that I've managed to inject into Phaser. But at the same time if we're going to drop a full-on physics system in there for example, then it makes a lot of sense that the rest of our house is in order first!

 

Thoughts?

[acron^]

I don't think anyone is going to argue against such a significant performance increase.

Like you say though, incrementing or decrementing in a core loop is common...

What about bulking it out a bit and adding some helper functions to the tune of...

sprite.angle.value(); // gets valuesprite.angle.setTo(45); // sets to 45, 'setTo' taken from Point sprite.angle.incr(); // increments by 1, saves us from 'sprite.angle.setTo(sprite.angle.value() + 1);'sprite.angle.add(10); // adds 10sprite.angle.decr();sprite.angle.minus(12);

[agmcleod]

+1. If there are some nasty performance issues with it on mobile, better to improve it now.

[Mefteg]

I'm for better performance.

Moreover, doing things like that :

sprite.angle(); // returns '10'sprite.angle(45); // sets '45'

is the way used by jQuery and I think people don't be disconcerted.

[qdrj]

I think it should be getAngle/setAngle

[Hsaka]

I'm for improving performance. I think as long as these things are properly documented, developers won't have any trouble with such changes.

[1-800-STAR-WARS]

I think it should be getAngle/setAngle

 

Agreed - what about exposing the angle property directly, but allowing for optional 'niceness' with getter/setter functions? Or would that just lead to confusion over which to use?

[Biggerplay]

For sponsors it's all about performance on mobile....

[rich]

 

I don't think anyone is going to argue against such a significant performance increase.

Like you say though, incrementing or decrementing in a core loop is common...

What about bulking it out a bit and adding some helper functions to the tune of...

sprite.angle.value(); // gets valuesprite.angle.setTo(45); // sets to 45, 'setTo' taken from Point sprite.angle.incr(); // increments by 1, saves us from 'sprite.angle.setTo(sprite.angle.value() + 1);'sprite.angle.add(10); // adds 10sprite.angle.decr();sprite.angle.minus(12);

 

Interesting - angle would become more like a component in this instance. There is the other option of course - I just drop 'angle' entirely and you have to do everything via Sprite.rotation using radians instead. But that's stepping away from how easy I wanted Phaser to be to use.

[jerome]

clues : http://docs.turbulenz.com/js_development_guide.html#performance-techniques

[OttoRobba]

Really interesting topic.

 

I, like others, am in favor of things that increase performance considerably - a 600% improvement at such small cost is more than worth it.

 

 

As much as I love get/set as pointed out by qdrj, I think acron has the right idea. Specially because, if you think about it, Phaser is inconsistent with some of the naming currently.

 

For example:

sprite.anchor.setTo(1,1);sprite.angle(45);

With the change, not only would performance increase but it would become consistent:

sprite.anchor.setTo(1,1);sprite.angle.setTo(45);

Just for kicks based on qdrj suggestion:

sprite.getAngle();sprite.setAngle(45);sprite.setAnchor(1,1);

Or even crazier:

sprite.get.angle();sprite.set.angle(45);sprite.set.anchor(1,1);

"What can a sprite get?
-angle
-anchor
-scale
et al..."

[rich]

angle is a number and anchor is a Point, it's not inconsistent when it's two totally different data types

[OttoRobba]

angle is a number and anchor is a Point, it's not inconsistent when it's two totally different data types

True. But from the perspective of the user, does it make a difference? ( I'm asking because I honestly don't know)

[rich]

I think so, yes. Most properties don't need any additional computation when you modify them, rotation and scale for example, so wrapping those in functions just to keep it consistent will actually decrease performance, because function access is significantly lower than normal property access(just not as bad as defineProperty access!)

[OttoRobba]

I think so, yes. Most properties don't need any additional computation when you modify them, rotation and scale for example, so wrapping those in functions just to keep it consistent will actually decrease performance, because function access is significantly lower than normal property access(just not as bad as defineProperty access!)

 

Ah yes, true. I get it now

(Coming to Javascript from Lua I sometimes get confused with some bits and pieces)

[Cameron Foale]

Just for another data point, that JSperf test gives us on an iPad 2 running iOS 7.0.4:

- Object.defineProperty: 747 ops/sec (98% slower)
- __defineGetter__: 749 ops/sec (98% slower)
- Normal: 35,880 ops/sec (fastest)
- Prototype: 702 ops/sec (98% slower)
- Setter: 22,412 ops/sec (38% slower)

This is a HUGE difference, holy smokes. Normal access is 48 times faster?

I made a quick modification to the test here, which pulls the set up out of the testing loop (jsperf actually runs the contents of "test" multiple times, so 765 ops per second is actually 765000 ops when you stick your own for loop in there)

http://jsperf.com/object-defineproperty-vs-definegetter-vs-normal/55

This changes (on one sample) the values to:

- Object.defineProperty: 765 ops/sec (98% slower)
- __defineGetter__: 770 ops/sec (98% slower)
- Normal: 37,172 ops/sec (fastest)
- Prototype: 34,554 ops/sec (7% slower)
- Setter: 37,012 ops/sec (fastest)
 

(the small change in Normal is probably due to switching around the for loop construct)

The big movers here are the Prototype and Setter, which come close to parity with "normal". defineProperty remains very slow on iOS. I don't know if the Prototype <code>get z()</code> approach is particularly widely supported though.

 

This implies that the actual cost of setting up the Prototype is the same as 28000 calls to "set", or perhaps that resetting the prototype every 1000 gets/sets messes with the JIT.

[rich]

If you want to see something really shocking, you should run the test under a webview on iOS. It's enough to make you cry.

 

Support for prototype get set is pretty good apparently: http://kangax.github.io/es5-compat-table/

 

Nothing before IE9, but that doesn't matter for Phaser. Could do with testing on Android 2.x though.

 

In my mind this means I ought to shift to using Prototype get/set everywhere instead of defineProperty, or Normal where absolutely possible.

[jcs]

I'd second that, as it gets us the best of both worlds. it would be a shame, IMO, to lose usability for a performance gain that could disappear at any time (I don't show much benefit on Chrome 32 / linux, so I think it is safe to bet that the javascript engines that currently do so poorly could be fixed to do significantly better).

[rich]

I've started making the required changes (in the 1.2 branch). For things like Rectangle it was a breeze. But for objects that extend other objects I've yet to find a way to do it, i.e.:

Phaser.Image.prototype = Object.create(PIXI.Sprite.prototype);Phaser.Image.prototype.constructor = Phaser.Image;

This is all good and well, but now the prototype is fixed and you have to add to it via:

Phaser.Image.prototype.bringToTop = function(child) {

And I can't see any way to add a get/set to that without using Object.defineProperty. I tried using Phaser.Utils.extend (which is basically a jQuery deep object copy) but you have to put the get/set into an object that may not know anything about the one it's about to be attached to, so all the references to things like 'this' are lost.

[jcs]

hmm, yes. __defineGetter__ and __defineSetter__ were the ways to dynamically add a property to an object, but they seem to have been deprecated in favor of Object.defineProperty. 

 

it seems like we're stuck with either not using properties or suffering through the performance problems of defineProperty'd properties...

 

I'd use a mixed approach myself:

 

- prefer to add them on the prototypes of objects that you own and can do it with

- prefer simple direct access or accessor functions where you don't AND where performance is important (i.e. called in game loop)

- prefer Object.defineProperty otherwise, as it is the ECMAScript5 standard and (hopefully) will receive any performance work by browser vendors in future

[Cameron Foale]

Just noticed there was a typo in the Prototype getter setup in the jsperf test, which meant it was setting a global var instead of a local property.

Fixed it, and it doesn't appear to have made any difference perf-wise.

 

Anyway, I've done some more exploring and I think this is probably wasting your time Rich.

 

Check out http://jsperf.com/ob...er-vs-normal/55 again.

 

Defining the properties on the *prototype* instead of on the *objects* brings defineProperty back to within 5% of direct access on iOS 7.

 

Here are the pertinent results, but please check the test code to make sure it's doing what I think it is!

 

- Normal: 37116 ops/sec (fastest)

- Object.defineProperty *on the object*: 749 ops/sec (unchanged, slowest)

- Object.defineProperty *on the prototype*: 35658 ops/sec (4% slower)

 

I'm guessing the define property on the object means it has to enumerate through the object, or call something crazy?

 

If this performance profile is the same on Android and other targets, and, given that Phaser uses properties on prototypes exclusively, I think it's fine as is

[rich]

Ahhh, now that makes more sense I just benched it on iOS6 and I'm actually seeing faster results from Object.defineProperty (on the prototype) than Normal! But it's only a tiny difference, so a margin of error value really. Interestingly 'configurable' comes out tops of all, but again it's a slim victory.

 

Here are some interesting stats:

 

iOS6 - Safari - Object.defineProperty on prototype: 12,610 ops/sec (Normal: 12,607)

iOS6 - WebView - Object.defineProperty on prototype: 2,914 ops/sec (even Normal access is only 2,922)

 

iOS7 - Safari - Object.defineProperty on prototype: 33,644 ops/sec (Normal: 34,991)

iOS7 - WebView - Object.defineProperty on prototype: 1,821 ops/sec (even Normal access is only 1,834)

 

The iOS6/7 devices are identical iPad Minis. The WebView results are depressing!

 

So you're right, after all this it does appear that I can just carry on as before

[Cameron Foale]

Man, WebView performance is awful! It actually got 30% worse in iOS7?

 

Can anybody run this benchmark on Android Browser/Android Chrome? http://jsperf.com/object-defineproperty-vs-definegetter-vs-normal/55

 

I'll stop tweaking the benchmark now so results stick around - I just had to do some checks to make sure it was actually calculating the right thing.

[jcs]

running against chrome on 2013 nexus 7 right now...

[jcs]

results look comparable, I think we can safely forget about this