Skip to content
This repository has been archived by the owner on Aug 20, 2018. It is now read-only.

Optimize linking of classBinding fields #1455

Merged
merged 4 commits into from Apr 24, 2015

Conversation

marco-c
Copy link
Contributor

@marco-c marco-c commented Apr 24, 2015

No description provided.


for (var i = 0; i < fieldNames.length; i++) {
var fieldName = fieldNames[i];
var fieldSignature = instanceSymbols[fieldName];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I'm unsure if it'd make a difference for perf, but you should be able to iterate instanceSymbols property names directly:

for (var fieldName in instanceSymbols) {
  var fieldSignature = instanceSymbols[fieldName];
  
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did it with Object.keys because I recall something about for ... in being slow, although a simple test (http://jsperf.com/forinobjectkeys) shows that for .. in is faster.

@mykmelez
Copy link
Contributor

This jsperf test is also relevant (although it doesn't measure exactly the same thing).

I've seen admonitions against using for…in, for example in this StackOverflow thread and this Jon Raasch blog post. But they typically address the different use case of iterating arrays, which is not only less performant but also runs the risk of iterating non-indexed properties.

The blog post also suggests that the JS engine "has to set up a function for each" iteration of a for…in loop, although I don't think that's correct.

mykmelez added a commit that referenced this pull request Apr 24, 2015
Optimize linking of classBinding fields
@mykmelez mykmelez merged commit 83076b5 into mozilla:master Apr 24, 2015
@marco-c marco-c deleted the dont_export_Bindings branch April 24, 2015 18:09
@mbebenita
Copy link
Contributor

The problem with for each is memory allocation. You need to snapshot all the key values before iterating over them.

@mykmelez
Copy link
Contributor

The problem with for each is memory allocation. You need to snapshot all the key values before iterating over them.

We aren't using for each…in, we're using for…in. So iterating over keys, not values. Does that have the same problem?

@mbebenita
Copy link
Contributor

var y = {a: 0, b: 0};
for (var x in y) {
  print (x);
  y.c = 2;
}

prints:

a
b

The JS engine needs to snapshot the list of keys before iterating. In theory, this could be optimized, but @shu says that it's probably not.

@mykmelez
Copy link
Contributor

The original implementation called Object.keys() to get the keys to iterate, effectively taking a snapshot of the list of keys itself instead of relying on the JS engine to do it. Is that any better?

(Either way, I think this is academic, given that the objects we're iterating are small. But I'm still curious.)

@mbebenita
Copy link
Contributor

I was just commenting on it in general, not comparing the two approaches. It seems like they would behave similarly. These days I just try to stick to C style loops.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants