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

DRY up tappable code for applyPlugins. #34

Closed
wants to merge 1 commit into from
Closed

DRY up tappable code for applyPlugins. #34

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 30 additions & 17 deletions lib/Tapable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,33 +53,46 @@ Tapable.mixin = function mixinTapable(pt) {
copyProperties(Tapable.prototype, pt);
};

Tapable.prototype.applyPlugins = function applyPlugins(name) {
Tapable.prototype.applyPlugins = function applyPlugins(name, param1, param2) {
if(!this._plugins[name]) return;
Copy link

Choose a reason for hiding this comment

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

Without knowing a lot about this code and how it's being used, I'd suspect that this this._plugins[name] is not going to be monomorphic (i.e. name will be different and probably this._plugins too). So repeating this load is going to be the expensive part.

Copy link
Member

Choose a reason for hiding this comment

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

@bmeurer here is just a few examples of apply plugins being used in two separate classes that extend Tapable.

https://github.com/webpack/webpack/blob/990563f9a2b3fb3e817b7e1b9d6841bcd1eaa51e/lib/Compilation.js#L48-L676

https://github.com/webpack/webpack/blob/990563f9a2b3fb3e817b7e1b9d6841bcd1eaa51e/lib/Parser.js#L1045-L1141

Copy link

Choose a reason for hiding this comment

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

Ok, based on this benchmark, I'd strongly suggest to go for F.p.apply and rest parameters instead, as indicated by the results for Chrome Canary:

image

Copy link

Choose a reason for hiding this comment

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

So I was curious about Chrome Stable (V8 6.0.286.52, same as Node 8.3, and this code is running on Node) and the results are bit different:

screen shot 2017-08-13 at 09 15 45

Copy link

Choose a reason for hiding this comment

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

Jep, just checked Chrome 61 (which is what Node 8 will eventually be based on in the end), and same picture there:

Chrome 61 results

Copy link

Choose a reason for hiding this comment

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

FYI: Ran this gist with latest Node 8.3.0 locally and I do indeed see results similar to what I observed with Canary:

Node 8.3.0 results

Copy link
Member Author

Choose a reason for hiding this comment

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

PR updated with optimized version

Copy link
Member

Choose a reason for hiding this comment

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

I see to performance benefit for the method with variable number of arguments.

But I don't see the benefit for the methods with fixed number of arguments (i. e. applyPlugins2). They are manually optimized for a specific number of arguments. Most "hot" hooks use them.

image

Anyway, I would love to take a more radical approach. This whole package is a bit old. What about a new major version with more performance and more features.

  • Never use arguments, compile fixed arguments function.
  • Create a function per hooks which makes it monomorphic.
  • Maybe something magic like new Function(`function x(${args}) ...`)
  • Different apply function depending on number of registered plugins
    • no plugins: noop apply for max performance
    • special version to 1 plugin: optimized
    • general version for more plugin
  • You need to register hooks before using them. You need to specify number of arguments and kind (sync/async/parallel/waterfall).
  • Instead of compiler.plugin("before-run", ...) compiler.hooks.beforeRun(...) to allow have typings for these hooks.
  • Each plugin can have a name compiler.hooks.beforeRun("HotModuleReplacementPlugin", () => ...)
  • Insert plugins at specify locations instead of always to the end. hooks.beforeRun({ name: ..., before: "HotModuleReplacementPlugin" }, () => ...)
  • Maybe a middleware api, if this doesn't affect performance: hooks.beforeRun.intercept(...)
    • Would be great of the progress display and profiling.
  • The plugin(...) method can be provided for backward compatibility for plugins.
    • We may generate plugin name from stack trace (depending on performance cost for this)
  • The old applyPluginsXXX methods can also be offered for backward compatibility, but trigger a deprecation.
  • Using a unknown hook name in the plugin or applyPluginsXXX automatically register a compat hook which handle them.
  • The compatibility methods convert dash-case to camelCase.
  • This would be a big breaking change, but I guess acceptable with the compatibility layer.

Copy link
Member

Choose a reason for hiding this comment

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

I created a new issue for this: #35

var args = Array.prototype.slice.call(arguments, 1);

var plugins = this._plugins[name];
for(var i = 0; i < plugins.length; i++)
plugins[i].apply(this, args);
};

switch (arguments.length) {
case 1:
for(var i = 0; i < plugins.length; i++) {
plugins[i].call(this);
}
break;
case 2:
for(var i = 0; i < plugins.length; i++) {
plugins[i].call(this, param1);
}
break;
case 3:
for(var i = 0; i < plugins.length; i++) {
plugins[i].call(this, param1, param2);
}
break;
default:
var args = array.prototype.slice.call(arguments, 1);
for(var i = 0; i < plugins.length; i++) {
plugins[i].apply(this, args);
}
break;
}
}

Tapable.prototype.applyPlugins0 = function applyPlugins0(name) {
var plugins = this._plugins[name];
if(!plugins) return;
for(var i = 0; i < plugins.length; i++)
plugins[i].call(this);
this.applyPlugins(name)
};

Tapable.prototype.applyPlugins1 = function applyPlugins1(name, param) {
var plugins = this._plugins[name];
if(!plugins) return;
for(var i = 0; i < plugins.length; i++)
plugins[i].call(this, param);
this.applyPlugins(name, param)
};

Tapable.prototype.applyPlugins2 = function applyPlugins2(name, param1, param2) {
var plugins = this._plugins[name];
if(!plugins) return;
for(var i = 0; i < plugins.length; i++)
plugins[i].call(this, param1, param2);
this.applyPlugins(name, param1, param2)
};

Tapable.prototype.applyPluginsWaterfall = function applyPluginsWaterfall(name, init) {
Expand Down