[BUGFIX beta] Optimization in .keys

[marijaselakovic]

Hi,

I spotted two locations where for-in loops can be refactored to Object.keys and for loop (similarly as in 53b9f19).

Results of running tests suites in Chrome 42 with original version:

Tests completed in 58335 milliseconds.13213 assertions of 13213 passed, 0 failed.

And with modified version:

Tests completed in 54474 milliseconds.13213 assertions of 13213 passed, 0 failed.

If you use some performance benchmark it would be interesting to try.

[rwjblue]

Can you prefix this commit with [BUGFIX beta] so that we can pull that into the beta branch?

[stefanpenner]

This can be pre allocated

[marijaselakovic]

var ret = Object.keys(keys), without for loop?

[rwjblue]

Yes, I believe that would work.

[stefanpenner]

Its worth noting, that for of vs Object.keys isn't always cut and dry. Lots of really annoying trade-offs.

[rwjblue]

This looks good. Can you squash all the commits down into one?

[stefanpenner]

@krisselden can we safely omit the own clause here ?

[rwjblue]

@stefanpenner - Doesn't Object.keys only return own properties? In other words, the proposed change does not change the semantics of what it was doing before...

[stefanpenner]

semantically the output values would be the same (although sort order can differ depending on many things...), but that isn't why i asked. I should have been clearer as to why.

If props is always a fast object and never has inherited properties (which maybe we cant guarentee) then for (let key in props) { ret[key] = true; } would be by far the fastest option.

Object.keys is the safe bet here though

[rwjblue]

Ahh, I see what you mean, thanks for explaining!

[stefanpenner]

I just thought of it while reading, and thought maybe we should have a quick discussion.

[marijaselakovic]

Ok, I squashed all commits into single one. Regarding semantic equivalence, indeed for in+hasOwnProperty check is same as Object.keys. Regarding performance of both cases, if you try to track compiler optimizations in for example V8, you can easily see that V8 disables optimization of for-in loop and reports the following: > ForInStatement is not fast case

In general, the actual performance improvement depends on the input. If .keys function iterates over objects with large number of properties, Object.keys might be performance wise.

[rwjblue]

@stefanpenner - I'm going to land this for now (it is a clear improvement), and continue following up with @krisselden about the remaining question.

[rwjblue]

Thank you @marijaselakovic!

[stefanpenner]

@marijaselakovic I'm confused, how does that differ from my comment. If we know the input is always on the happy path, for in is actually better. My question was largely, can we be sure of that and if not can we make changes to ensure that.

[marijaselakovic]

The case I am aware of, when for in and Object.keys perform equally in terms of performance is when you have object without inherited properties and properties that are not dynamically generated, so situation like: var obj={a:1,b:2,c:3}; I am not aware of the case when for in loop is actually better.