Refactor module.exports proposal

Question

Refactor module.exports proposal

tshemsedinov opened this issue 8 years ago · 16 comments

Here we use object for module.exports to export submodule interface and require to import it in lib/.

I propose to export function(api) with api argument to pass submodule interfaces and node.js api interfaces to each submodule for crosslinking. As we did in metarhia/Common and assemble all submodules in the following way: metarhia/Common/common.js

Answer 1 · 2017-03-12T21:45:49.000Z

Not sure about "optimization" tbh 😅, but I'm gonna test it. As it turned out, even something like

myModule = {};
module.exports = myModule;

myModule.first = () => { ... };
myModule.second = () => { ... };

// or
// exports.first = () => { ... };
// exports.second = () => { ... };
// that's the same, essentially

sometimes has a negative effect on performance (up to 2–5%), especially in large modules, compared to

function first() { ... }
function second() { ... }

module.exports = { first, second };

and that's the reason why Node core is now migrating from the former to the latter style.

Can't say any numbers about Impress-like style, it should be tested and benchmarked (a good candidate for HowProgrammingWorks/Benchmark#7 btw), but adding properties to an object dynamically in different functions and thus messing up with V8's ICs is a slippery slope, IMO. I can see the reasons behind using it in Impress but I'd still avoid it when possible.

OTOH, it might be good to have uniform codebase.

Answer 2 · 2017-03-12T22:00:39.000Z

/cc @belochub

Answer 3 · 2017-03-12T22:34:53.000Z

Exporting interface happens once on loading, so adding methods to empty {} or creating { name1,... nameN } doesn't affect performance. What I propose is about crosslinking between submodules and providing single require if dependency used by multiple submodules:

// Module main
const api = {};
const api.moduleName = {};
module.exports = api.moduleName;
// External dependancy
api.dependencyName = require('dependencyName');
// Internal dependency
['submodule1', 'submodule2']
  .map(path => './lib/' + path)
  .map(require)
  .map(exports => exports(api));

// Submodule
module.exports = (api) => {

  // So namespaces will be the same as in usage place
  api.moduleName.methodName = () => {
    // Here we can address internal and external namespaces using api...
  };

};

Answer 4 · 2017-03-12T22:53:15.000Z

Exporting interface happens once on loading, so adding methods to empty {} or creating { name1,... nameN } doesn't affect performance.

It would be great if it didn't but sometimes it actually does. Hint: slow properties vs fast properties (not a metaphor, that's exactly how they are called in V8) of objects.

Answer 5 · 2017-03-12T22:54:54.000Z

What I propose is about crosslinking...

I got what you propose :) The whole last two paragraphs of my comment are about it.

Answer 6 · 2017-03-12T23:40:59.000Z

Rough test:
https://github.com/HowProgrammingWorks/Benchmark/tree/master/JavaScript/Modularity

exportHash..................8142157419 nanoseconds
exportLink..................6900112765 nanoseconds
exportClosure...............6749524559 nanoseconds

Answer 7 · 2017-06-21T17:02:44.000Z

node v7.10.0

exportClosure.................6711192448.......min
exportLink....................6908150338.......+3%
exportHash....................8275736875......+23%

Answer 8 · 2017-06-21T17:06:42.000Z

@aqrln please test it on latest/master node

Answer 9 · 2017-06-21T17:20:25.000Z

@tshemsedinov well... I guess I did.

Looks like a bug in V8.

➜  modularity git:(master) ✗ ./modularity.sh

name (heat) time opt after: define opt heat loop

{ modulename:
   { submodule1: { first: [function: first], second: [function: second] },
     submodule2: { third: [function: third], fourth: [function: fourth] } } }
{ modulename:
   { submodule1: { first: [function], second: [function] },
     submodule2: { third: [function], fourth: [function] } } }
{ modulename:
   { first: [function],
     second: [function],
     third: [function],
     fourth: [function] } }

<--- last few gcs --->

[66017:0x102801000]     6202 ms: mark-sweep 577.8 (585.3) -> 577.4 (585.3) mb, 210.8 / 0.0 ms  allocation failure gc in old space requested
[66017:0x102801000]     6380 ms: mark-sweep 577.4 (585.3) -> 577.4 (582.3) mb, 178.3 / 0.0 ms  last resort
[66017:0x102801000]     6549 ms: mark-sweep 577.4 (582.3) -> 577.4 (582.3) mb, 168.5 / 0.0 ms  last resort


<--- js stacktrace --->

==== js stack trace =========================================

security context: 0x2f6244d29891 <js object>
    1: push(this=0x37b974f23681 <js array[75209227]>)
    2: /* anonymous */(aka /* anonymous */) [/users/alex/projects/howprogrammingworks/benchmark/javascript/2-benchmark.js:39] [pc=0x378671b62cc6](this=0x902d4982311 <undefined>,fn=0x7dfc7a7a891 <js function exporthash (sharedfunctioninfo 0x3e2b6b6f78e1)>)
    3: arguments adaptor frame: 3->1
    4: map [native array.js:1] [pc=0x378671b5ea89]...

fatal error: call_and_retry_last allocation failed - javascript heap out of memory
 1: node::abort() [/users/alex/.nvm/versions/node/v8.1.2/bin/node]
 2: node::fatalexception(v8::isolate*, v8::local<v8::value>, v8::local<v8::message>) [/users/alex/.nvm/versions/node/v8.1.2/bin/node]
 3: v8::internal::v8::fatalprocessoutofmemory(char const*, bool) [/users/alex/.nvm/versions/node/v8.1.2/bin/node]
 4: v8::internal::factory::newuninitializedfixedarray(int) [/users/alex/.nvm/versions/node/v8.1.2/bin/node]
 5: v8::internal::(anonymous namespace)::elementsaccessorbase<v8::internal::(anonymous namespace)::fastpackedobjectelementsaccessor, v8::internal::(anonymous namespace)::elementskindtraits<(v8::internal::elementskind)2> >::convertelementswithcapacity(v8::internal::handle<v8::internal::jsobject>, v8::internal::handle<v8::internal::fixedarraybase>, v8::internal::elementskind, unsigned int, unsigned int, unsigned int, int) [/users/alex/.nvm/versions/node/v8.1.2/bin/node]
 6: v8::internal::(anonymous namespace)::elementsaccessorbase<v8::internal::(anonymous namespace)::fastpackedobjectelementsaccessor, v8::internal::(anonymous namespace)::elementskindtraits<(v8::internal::elementskind)2> >::growcapacityandconvertimpl(v8::internal::handle<v8::internal::jsobject>, unsigned int) [/users/alex/.nvm/versions/node/v8.1.2/bin/node]
 7: v8::internal::(anonymous namespace)::elementsaccessorbase<v8::internal::(anonymous namespace)::fastpackedobjectelementsaccessor, v8::internal::(anonymous namespace)::elementskindtraits<(v8::internal::elementskind)2> >::add(v8::internal::handle<v8::internal::jsobject>, unsigned int, v8::internal::handle<v8::internal::object>, v8::internal::propertyattributes, unsigned int) [/users/alex/.nvm/versions/node/v8.1.2/bin/node]
 8: v8::internal::jsobject::adddataelement(v8::internal::handle<v8::internal::jsobject>, unsigned int, v8::internal::handle<v8::internal::object>, v8::internal::propertyattributes, v8::internal::object::shouldthrow) [/users/alex/.nvm/versions/node/v8.1.2/bin/node]
 9: v8::internal::runtime::setobjectproperty(v8::internal::isolate*, v8::internal::handle<v8::internal::object>, v8::internal::handle<v8::internal::object>, v8::internal::handle<v8::internal::object>, v8::internal::languagemode) [/users/alex/.nvm/versions/node/v8.1.2/bin/node]
10: v8::internal::runtime_setproperty(int, v8::internal::object**, v8::internal::isolate*) [/users/alex/.nvm/versions/node/v8.1.2/bin/node]
11: 0x37867198437d
12: 0x378671a33985
./modularity.sh: line 3: 66017 abort trap: 6           node --allow-natives-syntax modularity.js

Answer 10 · 2017-06-21T17:37:42.000Z

@tshemsedinov it fails somewhere in the benchmarking code. After reducing the number of iterations a bit, I am able to run it.

Node.js 8.1.2, V8 5.8.283.41 (Crankshaft):

exportClosure..................644901477.......min
exportHash.....................789679444......+22%
exportLink.....................820563794......+27%

Node.js master, V8 5.9.211.37 (Ignition + TurboFan):

exportHash.....................293962381.......min
exportClosure..................327534960......+11%
exportLink.....................518704967......+76%

Answer 11 · 2017-07-01T11:29:38.000Z

@aqrln thanks, that's great that they fixed this bug but I thin +11% is ok for those benefits it gives us:

Submodules crosslinking
Ability to instantiate modules without require
Full namespace in sources

Answer 12 · 2017-07-01T12:45:44.000Z

/cc @metarhia/jstp-core

Answer 13 · 2017-07-07T18:44:29.000Z

We will use use hash export for both external export and submodules

Answer 14 · 2017-07-09T23:03:08.000Z

@aqrln I tried to use hash in common and metasync but experiencing difficulties with that.
Here are some problems actual for all ours libs and we need a decision:

It is good to use namespaces in even in submodules to show that this certain function will be exported or was imported. Local function will not have namespaces and it is obvious very convenient to avoid naming conflicts and to browse the code if we need to find common.cache over multiple files, for example. I agree that using long namespaces like api.common.cache is very unusual practice in node.js and js at all. But short (2 step) namespace like common.cache is not something strange.
I agree that explicit import/export is good for external dependencies but I don't like to duplicate it multiple times like adding require('common') to each file. I'd prefer to do it once on module load and pass a link to submodules to be accessed by namespace.
In common and metasync we do not export closure to dependent modules we export hash but use closure to pass this hash over submodules.
I think there is no reason to search fastest solution in this case because import/export isn't a frequent operation, so we should find most conceptually suitable.
We have multiple export types even in one repository. In jstp we have two types export

Export hash at the end, example: /lib/applications.js
Mixin to hash exported at the beginning, example: /lib/socket.js

Case with module internal constants is an edge case because in JS it is better to define internal constant in that file where we use it. External constants have no such problem. I also considered pass module internal namespace as a second parameter to closure:

module.exports = (api, metasync) => {

  const FILE_SCOPE_CONSTANT = 100;
  metasync.MODULE_SCOPE_CONSTANT = 200;
  api.metasync.EXPORTED_CONSTANT = 300;

  const fileScopeFunction = () => {};
  metasync.moduleScopeFunction = () => {};
  api.metasync.exportedFunction = () => {};

};

And it seems to me that it is lesser evil. So I invite @belochub @nechaido @lundibundi to discussion.

Answer 15 · 2017-07-09T23:16:46.000Z

Module main for submodule template posted above will look like this:

'use strict';

const api = {};
const metasync = {};
api.metasync = {};
api.common = require('metarhia-common');
api.util = require('util');

module.exports = api.metasync;

const submodules = [
  'utils', // Basic utilities
  'flow', // Flow control
  ...
];

submodules
  .map(path => './lib/' + path)
  .map(require)
  .map(submodule => submodule(api, metasync));

Answer 16 · 2017-07-30T21:47:08.000Z

PR #244 includes export unification where needed