Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Further general optimizations. #389

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Further general optimizations. #389

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
859f415
Avoid accessing arguments array without length check.
cscott Dec 16, 2015
b9c8385
Ensure that ES.* abstract operations are able to be inlined.
cscott Dec 16, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 12 additions & 8 deletions es6-shim.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,9 @@

var $String = String;

var ES = {
// Assign these functions to a prototype to give v8 a hint that it
Copy link
Collaborator

Choose a reason for hiding this comment

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

how does this impact other engines? v8-specific optimizations have hurt other engines' performance in the past, with bluebird for example.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can't imagine it would hurt performance, since it's just a throwaway assignment. I'll try to double-check, but I don't know of any equivalent low-level IR tools on the firefox side. (jsperf can be highly misleading.)

// should optimize them as constant functions (and inline them).
var ES = (function () {}).prototype = {
// https://people.mozilla.org/~jorendorff/es6-draft.html#sec-call-f-v-args
Call: function Call(F, V) {
var args = arguments.length > 2 ? arguments[2] : [];
Expand Down Expand Up @@ -1096,7 +1098,9 @@

var ArrayPrototypeShims = {
copyWithin: function copyWithin(target, start) {
var end = arguments[2]; // copyWithin.length must be 2
// copyWithin.length must be 2, so we can't add `end` to the arguments
Copy link
Collaborator

Choose a reason for hiding this comment

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

this comment isn't necessary, we should definitely be consistently checking arguments.length before accessing an index - which we already do in many places.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, I was just repeating and expanding the comment which was there originally, which was trying to explain why this couldn't be simply declared as copyWithin(target, start, end). But in theory our test cases should actually be verifying the length property of the function, so the comment should be unnecessary...

// directly.
var end = arguments.length > 2 ? arguments[2] : void 0;
var o = ES.ToObject(this);
var len = ES.ToLength(o.length);
var relativeTarget = ES.ToInteger(target);
Expand Down Expand Up @@ -1243,7 +1247,7 @@
if (!arrayFromHandlesUndefinedMapFunction) {
var origArrayFrom = Array.from;
overrideNative(Array, 'from', function from(items) {
if (arguments.length > 0 && typeof arguments[1] !== 'undefined') {
if (arguments.length > 1 && typeof arguments[1] !== 'undefined') {
return ES.Call(origArrayFrom, this, arguments);
} else {
return _call(origArrayFrom, this, items);
Expand Down Expand Up @@ -2013,7 +2017,7 @@
// some environments don't have setTimeout - no way to shim here.
if (typeof setTimeout !== 'function' && typeof setTimeout !== 'object') { return; }

ES.IsPromise = function (promise) {
var IsPromise = function (promise) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why wouldn't we want this on the ES object?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because mutating an object after you've defined it ensures that IsPromise won't be optimized. The number of slots is fixed at definition time. But even if you add IsPromise: null to the definition of ES to hold the space, this ends up being initialized as a property reference stored in the ES object, not a "constant function reference" stored in the object's map (which can then be inlined after a hoisted type check).

This is described a bit by http://mrale.ph/blog/2012/09/23/grokking-v8-closures-for-fun.html

if (!ES.TypeIsObject(promise)) {
return false;
}
Expand Down Expand Up @@ -2421,7 +2425,7 @@
if (!ES.TypeIsObject(C)) {
throw new TypeError('Bad promise constructor');
}
if (ES.IsPromise(v)) {
if (IsPromise(v)) {
var constructor = v.constructor;
if (constructor === C) { return v; }
}
Expand All @@ -2439,10 +2443,10 @@

then: function then(onFulfilled, onRejected) {
var promise = this;
if (!ES.IsPromise(promise)) { throw new TypeError('not a promise'); }
if (!IsPromise(promise)) { throw new TypeError('not a promise'); }
var C = ES.SpeciesConstructor(promise, Promise);
var resultCapability;
var returnValueIsIgnored = (arguments[2] === PROMISE_FAKE_CAPABILITY);
var returnValueIsIgnored = (arguments.length > 2 && arguments[2] === PROMISE_FAKE_CAPABILITY);
Copy link
Collaborator

Choose a reason for hiding this comment

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

might as well remove these unnecessary parens also

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm, I guess---although I find having two different equals signs on a line without explicit grouping impairs readability.

if (returnValueIsIgnored && C === Promise) {
resultCapability = PROMISE_FAKE_CAPABILITY;
} else {
Expand Down Expand Up @@ -3302,7 +3306,7 @@
if (!ES.IsConstructor(constructor)) {
throw new TypeError('First argument must be a constructor.');
}
var newTarget = arguments.length < 3 ? constructor : arguments[2];
var newTarget = arguments.length > 2 ? arguments[2] : constructor;
if (!ES.IsConstructor(newTarget)) {
throw new TypeError('new.target must be a constructor.');
}
Expand Down