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,src: make constants not inherit from Object #10458

Closed
wants to merge 1 commit into
base: master
from

Conversation

@thefourtheye
Contributor

thefourtheye commented Dec 26, 2016

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)

lib src test

Description of change

Make sure constants object and all the nested objects don't inherit
from Object.prototype but from null.

Show outdated Hide outdated lib/fs.js
@@ -1004,7 +1004,7 @@ fs.fchmodSync = function(fd, mode) {
return binding.fchmod(fd, modeNum(mode));
};
if (constants.hasOwnProperty('O_SYMLINK')) {
if ('O_SYMLINK' in constants) {

This comment has been minimized.

@addaleax

addaleax Dec 26, 2016

Member

I think these have a small performance impact, maybe Object.prototype.hasOwnProperty.call is better for all of these?

@addaleax

addaleax Dec 26, 2016

Member

I think these have a small performance impact, maybe Object.prototype.hasOwnProperty.call is better for all of these?

This comment has been minimized.

@thefourtheye

thefourtheye Dec 26, 2016

Contributor

As constants inherit from null, will the performance implication still be applicable?

@thefourtheye

thefourtheye Dec 26, 2016

Contributor

As constants inherit from null, will the performance implication still be applicable?

This comment has been minimized.

@targos

targos Dec 26, 2016

Member

Performance of the in operator has recently been optimized in V8 (in 5.2).

@targos

targos Dec 26, 2016

Member

Performance of the in operator has recently been optimized in V8 (in 5.2).

This comment has been minimized.

@mscdex

mscdex Dec 26, 2016

Contributor

@targos It's still quite slow though in master (V8 5.4). I recently swapped usage of in with a simpler undefined check and saw a ~50% improvement.

@thefourtheye I would just do an undefined check (constants.O_SYMLINK !== undefined) instead of in. It should still work fine.

@mscdex

mscdex Dec 26, 2016

Contributor

@targos It's still quite slow though in master (V8 5.4). I recently swapped usage of in with a simpler undefined check and saw a ~50% improvement.

@thefourtheye I would just do an undefined check (constants.O_SYMLINK !== undefined) instead of in. It should still work fine.

This comment has been minimized.

@thefourtheye

thefourtheye Dec 26, 2016

Contributor

Done! I changed them to !== undefined.

@thefourtheye

thefourtheye Dec 26, 2016

Contributor

Done! I changed them to !== undefined.

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Dec 26, 2016

Member

What do we gain from this change?

Member

targos commented Dec 26, 2016

What do we gain from this change?

@thefourtheye

This comment has been minimized.

Show comment
Hide comment
@thefourtheye

thefourtheye Dec 26, 2016

Contributor

@targos Lookup should be comparatively faster and problems because of accidental accessing of inherited properties will be avoided, like in #10423

Contributor

thefourtheye commented Dec 26, 2016

@targos Lookup should be comparatively faster and problems because of accidental accessing of inherited properties will be avoided, like in #10423

Show outdated Hide outdated test/parallel/test-binding-constants.js
Object.keys(constants.os).sort(), ['UV_UDP_REUSEADDR', 'errno', 'signals']
);
// make sure all the constants objects don't inherit from Object.prototype

This comment has been minimized.

@cjihrig

cjihrig Dec 26, 2016

Contributor

Can you capitalize "make" please.

@cjihrig

cjihrig Dec 26, 2016

Contributor

Can you capitalize "make" please.

This comment has been minimized.

@thefourtheye

thefourtheye Dec 26, 2016

Contributor

Ack.

@thefourtheye

thefourtheye Dec 26, 2016

Contributor

Ack.

Show outdated Hide outdated test/parallel/test-binding-constants.js
);
// make sure all the constants objects don't inherit from Object.prototype
const INHERITED_PROPERTIES = Object.getOwnPropertyNames(Object.prototype);

This comment has been minimized.

@cjihrig

cjihrig Dec 26, 2016

Contributor

Can you name this with camelCase please.

@cjihrig

cjihrig Dec 26, 2016

Contributor

Can you name this with camelCase please.

This comment has been minimized.

@thefourtheye

thefourtheye Dec 26, 2016

Contributor

Ack.

@thefourtheye

thefourtheye Dec 26, 2016

Contributor

Ack.

@mscdex

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

LGTM for a major (if it works, ci)

@cjihrig

cjihrig approved these changes Jan 3, 2017

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Jan 3, 2017

Contributor

LGTM

Contributor

mscdex commented Jan 3, 2017

LGTM

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Jan 3, 2017

Member

I'm wondering if a similar semver-major change should not be made to the docs-only deprecated require('constants')

Member

jasnell commented Jan 3, 2017

I'm wondering if a similar semver-major change should not be made to the docs-only deprecated require('constants')

@thefourtheye

This comment has been minimized.

Show comment
Hide comment
@thefourtheye

thefourtheye Jan 3, 2017

Contributor

@jasnell Do you mean the object returned by require('constants') should also inherit from null?

Contributor

thefourtheye commented Jan 3, 2017

@jasnell Do you mean the object returned by require('constants') should also inherit from null?

lib,src: make constants not inherit from Object
Make sure `constants` object and all the nested objects don't inherit
from `Object.prototype` but from `null`.
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Jan 3, 2017

Member

@thefourtheye ... yes, that's what I'm thinking. We could choose to leave it untouched but for consistency it may make sense

Member

jasnell commented Jan 3, 2017

@thefourtheye ... yes, that's what I'm thinking. We could choose to leave it untouched but for consistency it may make sense

@thefourtheye

This comment has been minimized.

Show comment
Hide comment
@thefourtheye
Contributor

thefourtheye commented Jan 4, 2017

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Jan 4, 2017

Member

Boo. Ok, I'd leave it as is then.

Member

jasnell commented Jan 4, 2017

Boo. Ok, I'd leave it as is then.

@thefourtheye

This comment has been minimized.

Show comment
Hide comment
@thefourtheye

thefourtheye Jan 15, 2017

Contributor

cc @nodejs/ctc as this is a major change.

Contributor

thefourtheye commented Jan 15, 2017

cc @nodejs/ctc as this is a major change.

@thefourtheye

This comment has been minimized.

Show comment
Hide comment
@thefourtheye
Contributor

thefourtheye commented Feb 13, 2017

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 13, 2017

Member

@thefourtheye ... this should be good to go!

Member

jasnell commented Feb 13, 2017

@thefourtheye ... this should be good to go!

@thefourtheye

This comment has been minimized.

Show comment
Hide comment
@thefourtheye

thefourtheye Feb 13, 2017

Contributor

@jasnell Thanks :-) I would be comfortable if I get a green CITGM as well, as this is a major change. I'll try to spend sometime this week to fix it.

Contributor

thefourtheye commented Feb 13, 2017

@jasnell Thanks :-) I would be comfortable if I get a green CITGM as well, as this is a major change. I'll try to spend sometime this week to fix it.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 22, 2017

Member

CITGM and CI both were good. Landing.

Member

jasnell commented Mar 22, 2017

CITGM and CI both were good. Landing.

jasnell added a commit that referenced this pull request Mar 22, 2017

lib,src: make constants not inherit from Object
Make sure `constants` object and all the nested objects don't inherit
from `Object.prototype` but from `null`.

PR-URL: #10458
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 22, 2017

Member

Landed in caf9ae7

Member

jasnell commented Mar 22, 2017

Landed in caf9ae7

@jasnell jasnell closed this Mar 22, 2017

@thefourtheye thefourtheye deleted the thefourtheye:make-constants-inherit-from-null branch Apr 2, 2017

@jasnell jasnell referenced this pull request Apr 4, 2017

Closed

8.0.0 Release Proposal #12220

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment