-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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.
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
Optimise __extends #9925
Comments
I'm pretty sure moving the So if you are registering a speed improvement, I'd guess it's all from the |
It is - the |
This part also seems odd...
Given that I can only imagine this would be via class expressions that are repeatedly executed on some hot path. If there was some way to avoid continual re-evaluation of class expressions (eg by moving them out to a normal module-level class declaration, perhaps with some extra constructor parameters), I imagine |
The cause of me looking at it isn't really relevant - was just the reporting method I was using and it grouping things together - hence why I saw the two minor issues in it (it took longer writing this issue out that changing and testing the code lol) - it's also why it's really hard to measure the speed difference - for something that's called only once per class it's the iterating that takes the longest time as nothing else is that expensive - and even that expense depends on how much is actually inside the class and tree (mine is pretty big, hence the 10%). Saving 50ms is worth it for me, not sure it'd make any difference for 99% or more of the projects using TS, but better code is always worth talking about ;-) |
Bear in mind that function declarations are hoisted, so just because you lexically nest it inside an |
|
Got me there :) |
You were completely right though, hence I changed the issue code :-P |
Just a hypothetical.... If you had a project that creates 1,000,000 instances across 10 different classes, then OTOH, if your project has |
Seconding @yortus 's comments that perf of |
The primary goal of this code is to be more "correct", the speedup is a side effect of that - the final in-memory classes used will be identical so it's purely a case of changing how it gets there slightly. I'd hate to see any code with a million classes in it! |
One problem here is that we need the You might say "Easy fix, you can emit different |
Good point - though it would be a problem if an ES5 class was loaded before an ES3 one I think - but do ES6+ classes even get this included? Having a slightly longer version that checks would fix it - but not sure that the extra code would be worth it: var __extends = (this && this.__extends) || function (d, b) {
if (b instanceof Object) {
if (Object.keys) {
Object.keys(b).forEach(function(p){
d[p] = b[p];
});
} else {
for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p];
}
var __=function() { this.constructor = d; };
__.prototype = b.prototype;
d.prototype = new __();
} else {
d.prototype = Object.create(null);
}
}; |
interface Object {
// Type system in typescript is too weak.
assign( target : any , ...sources : any[] ) : any
}
/// Inheritence with static constructor support
var __extends = ( Sub , Sup ) => {
/// Inherit static properties
Object.assign( Sub , Sup )
/// Create inherited prototype without parent constructor calling
Sub.prototype = Object.create( Sup.prototype , {
constructor : {
configurable : true ,
writable : true ,
value : Sub ,
}
} )
/// Call static constructor
if( Sub.initializer ) Sub.initializer()
} |
You can boldly include polyfills for |
I was trying to find places to optimise a large project, and realised that the longest running function was actually the __extends call, which prompted me to look at the current code.
Current code
Two things immediately jumped out at me with the current code:
Problems
Potentially unused code (pre ES5)
The first problem was that the
__()
function was being created, when it might not be used (though chances of that are probably very small).The Solution would be to move the check into an if/else. Related to this is that
null
doesn't have any properties, so the property copying should be in the "not null" section.Inefficient iteration (for ES5+)
The second problem is purely performance related with the property copying.
for(var p in b)
will iterate over the entire prototype tree, butif(b.hasOwnProperty(p))
will then skip any that aren't directly on the object/class itself. (see Enumerability and ownership of properties)The solution would be to use
Object.keys(b)
and iterating an array, which will work only on the object/class itself (and not it's prototype tree), it also automatically skips the same keys we'd be skipping anyway - so taking most of what we're wanting to the browser / native code instead of JS.Unlike
for...in
, Object.keys fails when called onnull
,undefined
or any other non-Object, so this requires some extra code to help prevent that happening.From here we see that
Object.keys()
is an ES5+ function.Suggested code:
Results
It's very hard to get reliable timings for the code on Chrome Profiling, but on average this looks like roughly a 10% speedup at startup for my project (38 classes in a nested tree up to six deep). There is no major benefit to this, though it does make the code slightly "safer".
The ES5+ code could potentially cause different behaviour if a class tries to inherit from a non-class (such as a number or string), though I'm not sure that is possible to begin with..
Language Feature Checklist
The text was updated successfully, but these errors were encountered: