modules refactored to use export pattern

[MylesBorins]

Looks like a bunch of modules have had some fairly major churn on master / v7 to refactor the way they export.

This is creating quite a bit of churn.

I'd like to use this issue to keep track of all modules that are going through this refactor and make sure we manually backport changes to LTS

prs that do this:

https://github.com/nodejs/node/pull/11696

[MylesBorins]

/cc @nodejs/collaborators in general when you see big project wide churn like this please feel free to open an issue on LTS so we can track it

[jasnell]

Generally, all of the modules should eventually move to this pattern. I'm not convinced that it is something we should backport tho. (Yes, I understand that not backporting it makes it difficult moving forward). The change is due to subtle internal differences in the way that V8 handles these kinds of objects and the changes could actually cause performance regressions on older Node.js versions.

[refack]

nodejs/node#12654

[isaacs]

@jasnell I'm curious about the change to V8 that makes module.exports={x} faster than exports.x=x. (I believe you of course, just wanna see if it's something I can/should leverage in userland code.) Do you have a link or explanation of the change?

[RReverser]

@isaacs I don't know if it's really recent change, but generally one-off object creation should be always faster, as adding properties one-by-one makes lots of transitions between hidden classes, whereas with a single literal engine knows how many properties it needs to allocate up-front and can generate a single final hidden class.

[jasnell]

@isaacs ... it has to do with relatively recent changes to property handling in V8. I don't have all of the specifics and it does not seem to be 100% true in all cases. I've had to run a ton of benchmarks. What I do know for certain is that V8 uses some internal heuristics to selectively compile some bits of code under TurboFan+Ignition now even tho those aren't enabled by default, which has had some definite impact.

Under TurboFan+Ignition, the patterns we use will need to evolve just as we've had to specialize some things for Crankshaft. The one definite downside has to do with monkeypatching -- specifically, this pattern makes it significantly less possible which is the main reason I'm reluctant to backport any of these types of changes to LTS branches.

[vsemozhetbyt]

@isaacs Some links I've saved: http://mrale.ph/blog/2013/08/14/hidden-classes-vs-jsperf.html http://mrale.ph/blog/2015/01/11/whats-up-with-monomorphism.html http://stackoverflow.com/questions/24987896/how-does-bluebirds-util-tofastproperties-function-make-an-objects-properties https://www.html5rocks.com/en/tutorials/speed/v8/ https://blog.ghaiklor.com/optimizations-tricks-in-v8-d284b6c8b183

[lpinca]

This might be also relevant: https://twitter.com/bmeurer/status/849952024498249728.

[sam-github]

@jasnell

this pattern makes it significantly less possible

Why? Isn't the change a refactor of

function fu(){}
module.exports.fu = fu;

to

function fu(){}
module.exports = {
  fu: fu,
};

That doesn't look more or less monkey-patchable, what else is happening that we should worry about?

/cc @nodejs/diagnostics

[isaacs]

Thanks @jasnell @vsemozhetbyt and @rreverser, that makes a lot of sense.

I'm also curious about monkeypatchability, given that I maintain spawn-wrap and graceful-fs, and lots of people rely on both.

[jasnell]

It depends on how the references to those functions are refactored, consider for instance:

function Foo() {
  return Bar();
}

function Bar() {
  return 1;
}

module.exports = {
  Foo, Bar
}

We can monkeypatch Bar by replacing it's definition on the export, but that will not change Foo's internal reference to the original Bar function. In order to preserve the monkeypatchability of Bar, we'd have to ensure that:

function Foo() {
  return exports.Bar();
}

function Bar() {
  return 1;
}

module.exports = exports = {
  Foo, Bar
}

Which is what we are doing in some cases but not others. We don't need to worry about this on internal/* modules, but there is an impact on the public API if we went all in with the modules.exports= {} approach that yields the best performance.

[jasnell]

(this is, btw, why I haven't been aggressive about modifying the public modules that have the most use and the highest likelihood of monkeypatching)

[sam-github]

Thanks @jasnell. Right, that does make things harder, we'd have to patch Foo so that when the underlying function returns a unpatched Bar, we modify it on that return path. Its not impossible, just more work, and definitely an observable/breaking change if back-ported.

[isaacs]

@jasnell As long as we do things like var fs = module.exports = ... and call eg fs.close() instead of just close(), graceful-fs is gonna be fine.

spawn-wrap patches the ChildProcess prototype, so it should be fine no matter what.

[jasnell]

Yeah, we just need to make sure that for the external modules, we are more deliberate and careful about it. Even tho we have more freedom with stuff behind internal/*, I think I'm generally -1 on backporting any of these.

[MylesBorins]

works for me. Biggest concern was delta of churn... but this should be fairly obvious. We are going to notice a bunch of churn and differences anyways between 8 and 6... so it is what it is

[jasnell]

We are definitely going to see the gap start to widen, especially as we continue to make changes that optimize for TF+I.

[MylesBorins]

closing as it doesn't seem like anything actionable