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

lib: refactor some internal/* code #12644

Closed
wants to merge 5 commits into from

Conversation

@jasnell
Copy link
Member

commented Apr 25, 2017

This one is a bit of a grab bag containing a number of small edits to a number of internal/* modules that I have been accumulating. They generally are around code cleanup or modernization

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

internal: process, module, streams, lib, fs, freelist

@@ -4,16 +4,17 @@ const Buffer = require('buffer').Buffer;
const Writable = require('stream').Writable;
const fs = require('fs');
const util = require('util');
const constants = process.binding('constants').fs;

const O_APPEND = constants.O_APPEND | 0;

This comment has been minimized.

Copy link
@mscdex

mscdex Apr 25, 2017

Contributor

Might want to check if the implicit coercion to a 32-bit value was intentional (e.g. performance related or the values were not originally numbers for some reason)?

This comment has been minimized.

Copy link
@jasnell

jasnell Apr 25, 2017

Author Member

I explored that a bit a couldn't find anything specific. I could be wrong, but I think the switch was more defensive just in case.

This comment has been minimized.

Copy link
@Fishrock123

Fishrock123 Apr 26, 2017

Member

This also converts to an integer if it was a float, or some other type of number-ish.

@@ -2,71 +2,71 @@

const Buffer = require('buffer').Buffer;

module.exports = BufferList;
module.exports = class BufferList {

This comment has been minimized.

Copy link
@mscdex

mscdex Apr 25, 2017

Contributor

Did you benchmark this change?

This comment has been minimized.

Copy link
@jasnell

jasnell Apr 25, 2017

Author Member

Yes, negligible difference in master with 5.7. Notable improvement under TF-I ... tho I'd have to pull the numbers back up. It was a week or so ago when I ran it.

lib/internal/process/next_tick.js Outdated
function setupNextTick() {
const promises = require('internal/process/promises');
const errors = require('internal/errors');
const emitPendingUnhandledRejections = promises.setup(scheduleMicrotasks);
const emitPendingUnhandledRejections = promises(scheduleMicrotasks);

This comment has been minimized.

Copy link
@mscdex

mscdex Apr 25, 2017

Contributor

I know the other 'setup' instances had the benefit of not having to be assigned to a variable first, but perhaps we should at least change the variable name here to something more straight-forward, like setupPromises instead of just promises?

This comment has been minimized.

Copy link
@jasnell

jasnell Apr 25, 2017

Author Member

Works for me..

@jasnell jasnell force-pushed the jasnell:refactor-some-internals branch Apr 25, 2017

lib/internal/bootstrap_node.js Outdated
_process.setupKillAndExit();
_process.setupSignalHandlers();
if (global.__coverage__)
NativeModule.require('internal/process/write-coverage').setup();
NativeModule.require('internal/process/write-coverage')();

This comment has been minimized.

Copy link
@Fishrock123

Fishrock123 Apr 26, 2017

Member

Note: I used this naming convention for consistency with the other setup functions.

IMO, it would be better to preserve it for clarity.

This comment has been minimized.

Copy link
@jasnell

jasnell Apr 26, 2017

Author Member

these are already different from the other ones tho and we eliminate an extraneous property lookup. I'm not going to fight for it, but I don't see much value in keeping the separate setup() functions.

@@ -130,3 +121,12 @@ function addBuiltinLibsToObject(object) {
});
});
}

module.exports = exports = {

This comment has been minimized.

Copy link
@Fishrock123

Fishrock123 Apr 26, 2017

Member

Remove exports?

This comment has been minimized.

Copy link
@jasnell

jasnell Apr 26, 2017

Author Member

That's my preference but in a separate issue @sam-github expressed a preference for these to retain the = exports

This comment has been minimized.

Copy link
@Fishrock123

Fishrock123 Apr 27, 2017

Member

Why? There is no point. Since you are already exposing the variable name to grab it this way exports is defunct. (Edit: because You can just access the variable anyways)

If necessary, we can remove this in another PR entirely but idk why we'd add more here...

This comment has been minimized.

Copy link
@mscdex

mscdex Apr 27, 2017

Contributor

I also agree that re-assigning exports is unnecessary. *shrug*

This comment has been minimized.

Copy link
@jasnell

jasnell Apr 27, 2017

Author Member

I'm perfectly happy with removing it.

lib/internal/process.js Outdated
@@ -267,3 +257,14 @@ function setupRawDebug() {
rawDebug(format.apply(null, arguments));
};
}

module.exports = exports = {

This comment has been minimized.

Copy link
@Fishrock123

@jasnell jasnell force-pushed the jasnell:refactor-some-internals branch Apr 27, 2017

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Apr 27, 2017

@Fishrock123 ... ok, I dropped the commit that altered the naming conventions on the process setup items. PTAL

@Fishrock123

This comment has been minimized.

Copy link
Member

commented Apr 27, 2017

Replied to the comment about module.exports = exports.

@jasnell jasnell force-pushed the jasnell:refactor-some-internals branch Apr 27, 2017

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Apr 27, 2017

Updated to remove the module.exports = exports =

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Apr 27, 2017

lib/internal/module.js Outdated
@@ -130,3 +121,12 @@ function addBuiltinLibsToObject(object) {
});
});
}

module.exports = {

This comment has been minimized.

Copy link
@mscdex

mscdex Apr 27, 2017

Contributor

Actually, I just realized we do need exports re-assigned here in this file because exports.requireDepth is modified in this module and lib/module.js checks this value.

This comment has been minimized.

Copy link
@jasnell

jasnell Apr 27, 2017

Author Member

heh... doh!

This comment has been minimized.

Copy link
@jasnell

jasnell Apr 27, 2017

Author Member

that particular bit could likely be refactored but I'll just replace the = exports = here for now

@jasnell jasnell force-pushed the jasnell:refactor-some-internals branch to 0c9bd21 Apr 27, 2017

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Apr 27, 2017

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Apr 27, 2017

CI is green

@mscdex

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2017

LGTM

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Apr 27, 2017

@Fishrock123 ... does this LGTY?

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Apr 28, 2017

Will land this by monday if there are no further objections

jasnell added a commit that referenced this pull request May 1, 2017
process: refactor internal/process.js export style
PR-URL: #12644
Reviewed-By: Brian White <mscdex@mscdex.net>
jasnell added a commit that referenced this pull request May 1, 2017
module: refactor internal/module export style
PR-URL: #12644
Reviewed-By: Brian White <mscdex@mscdex.net>
jasnell added a commit that referenced this pull request May 1, 2017
streams: refactor BufferList into ES6 class
PR-URL: #12644
Reviewed-By: Brian White <mscdex@mscdex.net>
jasnell added a commit that referenced this pull request May 1, 2017
fs: simplify constant decls
PR-URL: #12644
Reviewed-By: Brian White <mscdex@mscdex.net>
jasnell added a commit that referenced this pull request May 1, 2017
freelist: simplify export
PR-URL: #12644
Reviewed-By: Brian White <mscdex@mscdex.net>
@jasnell

This comment has been minimized.

Copy link
Member Author

commented May 1, 2017

Landed in 579ff2a, ed0716f, e2199e0, 08809f2 and ea9eed5

@jasnell jasnell closed this May 1, 2017

anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
process: refactor internal/process.js export style
PR-URL: nodejs#12644
Reviewed-By: Brian White <mscdex@mscdex.net>
anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
module: refactor internal/module export style
PR-URL: nodejs#12644
Reviewed-By: Brian White <mscdex@mscdex.net>
anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
streams: refactor BufferList into ES6 class
PR-URL: nodejs#12644
Reviewed-By: Brian White <mscdex@mscdex.net>
anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
fs: simplify constant decls
PR-URL: nodejs#12644
Reviewed-By: Brian White <mscdex@mscdex.net>
anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
freelist: simplify export
PR-URL: nodejs#12644
Reviewed-By: Brian White <mscdex@mscdex.net>
@jasnell jasnell referenced this pull request May 11, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
@gibfahn

This comment has been minimized.

Copy link
Member

commented Jun 18, 2017

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

gareth-ellis added a commit to gareth-ellis/node that referenced this pull request Aug 4, 2017
Update constructor for freelist inline with nodejs#12644
Updates to use current constructor for freelist, which was changed 
under pr nodejs#12644
gareth-ellis added a commit to gareth-ellis/node that referenced this pull request Aug 4, 2017
benchmark: Correct constructor for freelist
Updates to use current constructor for freelist, which was changed 
under pr nodejs#12644
@gareth-ellis gareth-ellis referenced this pull request Aug 4, 2017
2 of 2 tasks complete
addaleax added a commit that referenced this pull request Aug 10, 2017
benchmark: Correct constructor for freelist
Updates to use current constructor for freelist, which was changed
under pr #12644

Ref: #12644
PR-URL: #14627
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
icarter09 added a commit to icarter09/node that referenced this pull request Aug 12, 2017
benchmark: Correct constructor for freelist
Updates to use current constructor for freelist, which was changed
under pr nodejs#12644

Ref: nodejs#12644
PR-URL: nodejs#14627
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax added a commit that referenced this pull request Aug 12, 2017
benchmark: Correct constructor for freelist
Updates to use current constructor for freelist, which was changed
under pr #12644

Ref: #12644
PR-URL: #14627
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.