Y.State performance improvements #222

Merged
merged 2 commits into from Aug 15, 2012

Conversation

Projects
None yet
5 participants
Contributor

rgrove commented Aug 15, 2012

Backwards-compatible, low risk micro-optimizations for Y.State. In some browsers, common operations like addAll() and getAll() are as much as 2x faster.

See the benchmarks at http://jsperf.com/y-state-micro-optimizations

I ran all YUI unit tests using grover and got only a single failure in Y.Pjax, which is unrelated to these changes: https://gist.github.com/f81bead2a75a1d500e6c

While testing, I discovered that attribute-core and attribute-extras were missing a necessary dependency on the oop module, so I went ahead and fixed that as well.

Running Locally

The built attribute modules and updated yui and loader build files are intentionally not included in order to keep the diffs clean. Before testing these changes, you'll need to build them:

$ cd src/attribute && ant all
$ cd ../yui && ant all

rgrove added some commits Aug 15, 2012

@rgrove rgrove Fix attribute dependencies.
Y.State uses Y.each(), which is part of the oop module, but attribute-core
and attribute-extras didn't include oop as a dependency.
b3839b1
@rgrove rgrove Y.State micro-optimizations. 8ff8c3d

This pull request passes (merged 8ff8c3d into 4724c6d).

sdesai was assigned Aug 15, 2012

Since we're dealing with micro-optimizations: Is this faster than d[name], or d.hasOwnProperty[name]. I was just doing some testing for event, where "in" ended up being slower in Webkit browsers than hasOwnProperty() for example [which makes sense, since there's less to look at]. Unfortunately was the opposite for Gecko. Wondering if we've evaluated d[name] is comparison to name in d.

Owner

rgrove replied Aug 15, 2012

That's strange. I didn't test in vs. property retrieval here since it seemed like a no-brainer. But in all the WebKit-based testing I did back when I was optimizing Y.mix(), in was significantly faster than hasOwnProperty().

Contributor

sdesai commented Aug 15, 2012

Looks fine to me. Will pull in, probably into the upcoming PR so it gets some eyeballs, although coverage for State is pretty good, so not much of a risk concern.

A couple of points:

a) I'll parse through the jsperf numbers/graphs, but in the meantime : Does it end up being slower anywhere?

b) It seems like one large chunk of benefit is probably from removing the function hop from addAll() to add().

This function hop was kept in intentionally, to keep the code clean [ to encapsulate whatever needed to be done for a single add, in add() - more of a factor when the data-structure was inverted - not so much anymore ]. Since this is on a critical path I think it's fine to compromise on encapsulation for performance, but worth a comment - I'll add that, no need for another pull request.

Owner

ericf commented Aug 15, 2012

Just a house keeping note, when you merge this into 3.x it will remain open until these commits are eventually merged into master. This is because the Pull Request was opened against master.

Given our new process, I think that this is fine and actually desirable for some changes as they remain visible for longer and during the preview release.

Contributor

sdesai commented Aug 15, 2012

I'm actually planning on pulling it into master, since it's pretty safe, and fits the 3.6.x criteria, if that's what master will be. I'll then merge it into 3.x so it'll be part of the PR.

Contributor

rgrove commented Aug 15, 2012

Thanks!

In my tests (see the jsperf), every change resulted in at least a minor improvement. Nothing is slower in the new code.

Removing the addAll() -> add() function hop was definitely a huge boost. I wouldn't have done it if there were anything special happening in add() or if I thought there was likely to be something special there in the future, but it's about as basic an operation as it can be, so I didn't see any value in retaining the extra hop just for the sake of encapsulation, especially in low-level foundation code like this.

Owner

ericf commented Aug 15, 2012

Okay perfect.

Contributor

sdesai commented Aug 15, 2012

Here are the snippets I was testing for event (prop access added - proves it's slower than the others and worth changing to in) : http://jsperf.com/hop-in/6

Notice the benefit of hasOwnProperty() usage vs. in for Webkit browsers, especially Chrome - again it kind of makes sense - don't have the cost of walking up the prototype chain to determine if the property exists or not. I'd be surprised if that benefit doesn't show up in other engines eventually also.

Re: addAll() -> add() - I agree on the need to do it. I'm just saying, if critical path performance wasn't a concern, it would have been a call to add() - which would be the only thing which knew about whether it was data[name][key] or data[key][name]. Hence worth the comment - that we're not using add() explicitly for perf, since this is called many many times, and we need to squeeze anything we can out of it.

On Aug 15, 2012, at 1:34 PM, Ryan Grove wrote:

Thanks!

In my tests (see the jsperf), every change resulted in at least a minor improvement. Nothing is slower in the new code.

Removing the addAll() -> add() function hop was definitely a huge boost. I wouldn't have done it if there were anything special happening in add() or if I thought there was likely to be something special there in the future, but it's about as basic an operation as it can be, so I didn't see any value in retaining the extra hop just for the sake of encapsulation, especially in low-level foundation code like this.


Reply to this email directly or view it on GitHub.

Contributor

rgrove commented Aug 15, 2012

@sdesai: I think your jsperf was showing inaccurate results due to compiler optimizations on the noop code. When I modify the tests to actually do something inside the enumeration, the results look a lot more sane: http://jsperf.com/hop-in/7

hasOwnProperty() still ends up slightly faster, but not by much. Good to know though!

Contributor

sdesai commented Aug 15, 2012

That's interesting, although funky - seems like it would apply the same optimizations to all sets of code [ but who knows what they key in on ]. I actually see a noticeable difference when applied to actual Event code.

That is, when I profile these constructs in Chrome, using the native profiler, inside actual event code, I see similar gains when using hasOwnProperty vs in for the use case defined in the above perf tests.

For example (ballpark numbers), in the code we use to mix payload properties into the event facade, across 10000 fires [ 120ms is about 25% of the cost of the fire ]:

Equivalent of Test 1 [no hop] : 120ms
Equivalent of Test 2 [if-hop instead of if-in]: 60ms [ so basically cutting down the cost of a fire by 12% ]
Equivalent of Test 3 [2 hop]: 120ms

Not sure if it's something about the test harness or not. The profiler numbers are pretty consistent - to the point of having me back away from removing the hasOwnProperty checks.

Anyway, as mentioned, good to know the noop angle also. Will try out some standalone benchmarks also.

@yuibuild yuibuild merged commit 8ff8c3d into yui:master Aug 15, 2012

1 check passed

default The Travis build passed
Details

sdesai was unassigned by jlecomte Mar 13, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment