Do not reset temp vars on Context init #207

Merged
merged 2 commits into from Jun 29, 2015

Projects

None yet

3 participants

@kpdecker
Contributor

We know that they will not exist at this point so the overhead of iterating and running the has own property check is unnecessary.

This led to 4-16x speed increases for the Babel generator tests defined here:
http://kpdecker.github.io/six-speed/

@kpdecker kpdecker Do not reset temp vars on Context init
We know that they will not exist at this point so the overhead of iterating and running the has own property check is unnecessary.

This led to 4-16x speed increases for the Babel generator tests defined here:
http://kpdecker.github.io/six-speed/
0759fa6
@facebook-github-bot

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@facebook-github-bot

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@kpdecker kpdecker added a commit to kpdecker/regenerator that referenced this pull request Jun 25, 2015
@kpdecker kpdecker Reset rather than delete sent field on next
Delete was causing deoptimization due to backing class change and was unnecessary as this field was explicitly initialized to undefined and was not being checked for existence as best I can tell.

When combined with #207 saw a 40x throughput improvement under FF, from 39k operations/sec to 1.6M ops/src on http://kpdecker.github.io/six-speed/
7008d2a
@benjamn benjamn and 1 other commented on an outdated diff Jun 25, 2015
@@ -460,10 +460,12 @@
// Pre-initialize at least 20 temporary variables to enable hidden
// class optimizations for simple generators.
@benjamn
benjamn Jun 25, 2015 Member

Since performance was the only reason I originally bothered pre-initializing these fields, and you've demonstrated (with actual data) that it doesn't help, why don't we just replace this code with

for (var tempIndex = 0, tempName;
     hasOwn.call(this, tempName = "t" + tempIndex);
     ++tempIndex) {
  this[tempName] = null;
}

That way, on the first run, there would be only one hasOwn.call, and no assignments. Does that make sense?

@kpdecker
kpdecker Jun 25, 2015 Contributor

I had assumed that temp variable usage was not guaranteed to be in order. I.e. t1 could exist and not t0, if this isn't the case then your version would be much faster than the original but potentially slightly slower than the flagged version.

@benjamn
benjamn Jun 25, 2015 Member

Ah yes, while it's true that temp variable names are generated in depth-first AST order, code execution is likely to be non-linear. This makes me think the original approach was always slightly broken if there were more than 20 temp variables…

How about something like this?

for (var name in this) {
  // Not sure about the optimal order of these conditions:
  if (name.charAt(0) === "t" &&
      hasOwn.call(this, name) &&
      !isNaN(+name.slice(1)) {
    this[name] = undefined;
  }
}
@kpdecker
kpdecker Jun 25, 2015 Contributor

Performance on that is quite bad when used without the flag, causing the performance relative to my "ES5 baseline" to drop from 1/4 to 1/26th. Perhaps that combined with the flag is the best option?

@benjamn
benjamn Jun 25, 2015 Member

Yeah, ok, let's keep the flag.

@kpdecker
kpdecker Jun 29, 2015 Contributor

I've updated the PR per this discussion.

@benjamn benjamn merged commit 5386919 into facebook:master Jun 29, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@benjamn
Member
benjamn commented Jun 29, 2015

Thanks!

@benjamn benjamn added a commit to benjamn/babel that referenced this pull request Jun 29, 2015
@benjamn benjamn Upgrade Regenerator to 0.8.32.
This includes some major runtime performance improvements:
facebook/regenerator#207
facebook/regenerator#208

And also some bug fixes relating to async generator functions:
facebook/regenerator@29d81b6
facebook/regenerator@7d2a052
facebook/regenerator@5b9dd10
b9f6169
@benjamn benjamn referenced this pull request in babel/babel Jun 29, 2015
Merged

Upgrade Regenerator to 0.8.32. #1868

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