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.

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

Excessing object wrapping. #5600

Closed
jdalton opened this issue Aug 29, 2017 · 15 comments
Closed

Excessing object wrapping. #5600

jdalton opened this issue Aug 29, 2017 · 15 comments
Labels

Comments

@jdalton
Copy link
Contributor

@jdalton jdalton commented Aug 29, 2017

I'm noticing with webpack v3 there's a lot of this kind of generated code:

Object(__WEBPACK_IMPORTED_MODULE_3__module_resolve_filename_js__["a" /* default */])(foo)

What's the reason for wrapping all references with Object() and can this be disabled or optimized away to a single wrap?

@sokra
Copy link
Member

@sokra sokra commented Aug 30, 2017

The reason is to call the function with the correct this context:

import x from "module";

x(foo); // <- called without this (undefined/root)
var __WEBPACK_IMPORTED_MODULE_1__module__ = __webpack_require__(1);

__WEBPACK_IMPORTED_MODULE_1__module__.default(foo);
// ^ called with this = __WEBPACK_IMPORTED_MODULE_1__module__

Object(__WEBPACK_IMPORTED_MODULE_1__module__.default)(foo);
// ^ called without this (undefined/root)

The Object() is generated when calling a imported symbol directly. I omitted this in an older version, assuming nobody care about this in an exported function, but this breaks when exporting native functions like setTimeout with expected no this.

Most of these are optimized away with the ModuleConcatenationPlugin.


A bit of history of this: (This started 1.5 years ago)

  • I started to use (0, a.b)() for this, but this broken because of ASI when the previous line has no semicolon...
  • I tried a.b.bind(undefined)() and also a.b.bind()() but bind is pretty slow.
  • I switched to a.b.call(undefined) as it's a bit faster.
  • I switched to __webpack_require__.i(a.b)() with a identity function
  • I switched to Object(a.b)() because Object behaves like identity for functions and doesn't require a custom function in the runtime.
  • I did some performance measurements:

Here are up-to-date results:

// chrome  60.0.3112.113
call x 1000000: 9ms
call with Object x 1000000: 24ms
call with comma operator x 1000000: 10ms
call with identity function x 1000000: 9ms
call with valueOf x 1000000: 15ms
call with .bind x 1000000: 45ms
call with own getter x 1000000: 12ms
call with .call x 1000000: 10ms
call cached version x 1000000: 8ms

// firefox 55.0.3 (32-Bit)
call x 1000000: 4.65ms
call with Object x 1000000: 33.15ms
call with comma operator x 1000000: 4.26ms
call with identity function x 1000000: 5.67ms
call with valueOf x 1000000: 5.26ms
call with .bind x 1000000: 183.52ms
call with own getter x 1000000: 5.49ms
call with .call x 1000000: 5.62ms
call cached version x 1000000: 4.57ms

// node 8.4.0
call x 1000000: 10.145ms
call with Object x 1000000: 22.028ms
call with comma operator x 1000000: 9.233ms
call with identity function x 1000000: 10.348ms
call with valueOf x 1000000: 16.490ms
call with .bind x 1000000: 43.926ms
call with own getter x 1000000: 10.633ms
call with .call x 1000000: 10.075ms
call cached version x 1000000: 8.391ms

// node 6.11.2
call x 1000000: 17.505ms
call with Object x 1000000: 23.144ms
call with comma operator x 1000000: 9.383ms
call with identity function x 1000000: 9.346ms
call with valueOf x 1000000: 18.307ms
call with .bind x 1000000: 823.117ms
call with own getter x 1000000: 9.449ms
call with .call x 1000000: 17.538ms
call cached version x 1000000: 7.075ms

Note: bind got faster in node 8, bind is slow in firefox, most calls are faster in FF than in Chrome, Object() is also a bit slow.

  • I noticed that Object() is slower than a identity function, but 30ms for 1 million calls... who cares?
  • I maybe switch back to an identity function
  • I could optimize cases where this is not used, or an arrow function is used.
@emilio-martinez
Copy link

@emilio-martinez emilio-martinez commented Aug 30, 2017

@sokra very interesting! Thanks for the explanation.

Maybe it's a stupid question, but I'll ask anyway primarily out of curiosity: why not use (0, a.b)() with a semicolon beforehand?

@bmeurer
Copy link

@bmeurer bmeurer commented Aug 30, 2017

Maybe I'm missing something, but why not just

const foo = o.foo;
foo();

instead of

Object(o.foo)();
@sokra
Copy link
Member

@sokra sokra commented Aug 30, 2017

Maybe it's a stupid question, but I'll ask anyway primarily out of curiosity: why not use (0, a.b)() with a semicolon beforehand?

Our Parser is not so clever to track what's before the identifier.

@bmeurer Caching the export is not always correct as these are live bindings which can change anytime.

@jdalton
Copy link
Contributor Author

@jdalton jdalton commented Aug 30, 2017

Related:

@Andarist
Copy link

@Andarist Andarist commented Aug 30, 2017

If comma version is one of the fastest and is not bloating the generated code so much, why wasnt just ;(0, a.b)(); chosen? Code will get formatted later by the minifirier anyway. Im assuming no other troubles than ASI with comma version

@bmeurer
Copy link

@bmeurer bmeurer commented Aug 31, 2017

@sokra I see, thanks for the clarification. Will land the Object call inlining today, so Chrome M63 and some future version of Node should have Object(o.f)() as fast as const f = o.f; f().

Wrote down some details here.

kisg pushed a commit to paul99/v8mips that referenced this issue Aug 31, 2017
When calling

  Object(value)

where the value is known to be a JSReceiver, we can just replace it with
value, as the Object constructor call is a no-op in that case. Otherwise
when value is known to be not null or undefined then we can replace the
Object constructor call with an invocation of ToObject.

This covers the common pattern found in bundles generated by Webpack,
where the Object constructor is used to call imported functions, i.e.

  Object(module.foo)(1, 2, 3)

There's a lot of detail in webpack/webpack#5600
on this matter and why this pattern was chosen.

Bug: v8:6772
Change-Id: I2b4f0b4542b68b97b337ce571d6d79946c73d8bb
Reviewed-on: https://chromium-review.googlesource.com/643868
Reviewed-by: Yang Guo <yangguo@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#47728}
@sokra
Copy link
Member

@sokra sokra commented Aug 31, 2017

Great to see. If you want to you could improve the error messages for this cases a bit:

> var o = {}
undefined
> o.f()
TypeError: o.f is not a function 👍
> Object(o.f)()
TypeError: Object(...) is not a function 👎
> o.f.call()
TypeError: Cannot read property 'call' of undefined 👎
> (0,o.f)()
TypeError: (0 , o.f) is not a function 👍

This would help people debugging undefined/null functions.

@sokra
Copy link
Member

@sokra sokra commented Aug 31, 2017

i. e.

TypeError: Object(o.f) is not a function
TypeError: Cannot read property 'call' of undefined o.f
@bmeurer
Copy link

@bmeurer bmeurer commented Aug 31, 2017

@staeke
Copy link

@staeke staeke commented Oct 30, 2017

I personally don't care so much about performance. But..it it's a pretty rough hit on readability. I would totally trade clarity for correctness during all development. Would you be able to provide a flag to turn it off?

@fregante
Copy link

@fregante fregante commented Feb 25, 2018

If anyone's wondering, you can disable this in webpack 4 with the config:

{
	optimization: {
		concatenateModules: true
	}
}

Side note: concatenateModules is already enabled in production mode.

@jdalton
Copy link
Contributor Author

@jdalton jdalton commented May 20, 2018

I noticed that with a mode of "production" that if the NODE_ENV was not "production" that the Object(...) wrapping is still applied. In my case NODE_ENV was "production-test". In webpack 4.8.3.

Update:

Narrowed it down that when NODE_ENV had -test in it I was adding more config.entry builds which for some reason is enabling Object(...) wrapping:

config.entry.compiler = "./src/compiler.js"
config.entry.entry = "./src/entry.js"
config.entry.runtime = "./src/runtime.js"
@webpack-bot
Copy link
Contributor

@webpack-bot webpack-bot commented Nov 19, 2018

This issue had no activity for at least half a year.

It's subject to automatic issue closing if there is no activity in the next 15 days.

@webpack-bot
Copy link
Contributor

@webpack-bot webpack-bot commented Dec 4, 2018

Issue was closed because of inactivity.

If you think this is still a valid issue, please file a new issue with additional information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.