util: use Object.create(null) for dictionary object #3791

JungMinu · 2015-11-12T13:04:38Z

Fixes #3788
arrayToHash() needs to use Object.create(null) for its dictionary object.

Fixes #3788 `arrayToHash()` needs to use `Object.create(null)` for its dictionary object.

evanlucas · 2015-11-12T13:06:26Z

Could use a test

Fishrock123 · 2015-11-12T14:13:00Z

Note: Object.create(null) is pretty slow, relatively speaking. cc @trevnorris?

Fishrock123 · 2015-11-12T14:14:27Z

See also: #728

Fishrock123 · 2015-11-12T14:17:59Z

Probably better would be to special-case __proto__, unless someone has evidence that Object.create(null) is actually faster these days?

Fishrock123 · 2015-11-12T14:32:51Z

lib/util.js

@@ -159,7 +159,7 @@ function stylizeNoColor(str, styleType) {


 function arrayToHash(array) {
-  var hash = {};
+  var hash = Object.create(null);

  array.forEach(function(val, idx) {
    hash[val] = true;


You'd check if val was __proto__ here and discard it if it was, I think. cc @bnoordhuis

add if val is `__proto__`, use `Object.create(null);`

pmq20 · 2015-11-12T14:47:49Z

lib/util.js


  array.forEach(function(val, idx) {
+


Why creating a new hash in each iteration?

@pmq20 Sorry, it was a mistake

Fishrock123 · 2015-11-12T14:49:29Z

@JungMinu please run ./configure && make -j8 test.

pmq20 · 2015-11-12T14:51:03Z

Also rebasing multiple commits into one in a single MR is more preferable.

if array contains __proto__, use Object.create(null)

YurySolovyov · 2015-11-12T15:14:26Z

also make sure you follow coding style (spaces, braces, etc.)

Beautify codes

YurySolovyov · 2015-11-12T15:20:33Z

lib/util.js

+    hash = Object.create(null);
+  } else {
+    hash = {};
+  }

  array.forEach(function(val, idx) {


if idx is not used, why pass it?

@YuriSolovyov Hmm, It was there before I made a PR,
I speculate that it could be used

it is not used anywhere in the scope of the function, IMO it is safe to drop it

trevnorris · 2015-11-12T16:45:59Z

@Fishrock123 Object.create(null) is "slow" in terms of operations we were performing for Buffers (back when it was relevant). Though the implementation has improved and the overhead in this use case is negligible at best. And honestly the O(n) lookup of __proto__ could be slower depending on the array, since the common case would be that __proto__ doesn't exist.

@JungMinu Sorry for the confusion here. Let's simply set var hash = Object.create(null);.

util: use Object.create(null) for dictionary object

Fishrock123 · 2015-11-12T16:50:16Z

Fair enough. My bad.

jasnell · 2015-11-12T16:53:37Z

LGTM

trevnorris · 2015-11-12T16:55:39Z

if CI is happy, LGTM

cjihrig · 2015-11-12T16:58:30Z

LGTM. Good to know that we may be able to use this in other place in the code too.

paulfryzel · 2015-11-12T17:04:06Z

lib/util.js


-  array.forEach(function(val, idx) {
+  array.forEach(function(val) {


Would we want to continue to use arrow functions for inline anons?

array.forEach(val => hash[val] = true);

and/or convert this into a single reduce expression?

function arrayToHash(array) { return array.reduce((acc, val) => { acc[val] = true; return acc; }, Object.create(null)); }

Using reduce() is a bit misleading since nothing is actually being reduced, and I don't think turning it into a single expression is helpful in this case.

As for using arrow functions, personally only think they must be used to prevent var self = this; from happening, but also wouldn't be opposed if it were used. IOW, if it does change then cool, but it's not a blocker.

Ok cool. Yeah I guess I agree, the naming of Array.prototype.reduce is definitely a bit awkward here. Maybe the general arrow function stance is also worth a larger style/convention conversation elsewhere? Thanks for the input!

Ironically @JungMinu has already started some of this :) #3622

Hopefully we'll be able to finish conversion of other applicable cases, but it's not high priority. Don't think the CTC would feel the need to make a stance "official" but if the rest of the code is following some convention then we can point to that as an example going forward.

Fishrock123 · 2015-11-12T17:27:12Z

CI: https://ci.nodejs.org/job/node-test-pull-request/710/

(Continuous Integration test run)

jwueller · 2015-11-13T11:07:17Z

lib/util.js

@@ -159,9 +159,9 @@ function stylizeNoColor(str, styleType) {


 function arrayToHash(array) {
-  var hash = {};
+  var hash = Object.create(null);;


This adds a redundant semicolon.

jasnell · 2015-11-13T18:19:33Z

CI is red but failures look unrelated.
LGTM

jasnell · 2015-11-13T18:20:09Z

@JungMinu .. can I ask you to please squash the commits?

…tch-18

thefourtheye · 2015-11-14T19:22:38Z

I just used this script to compare the performance.

'use strict';

const times = 10e6;

console.time('Object.create');
for (var i = 0; i < times; i += 1) {
  let obj = Object.create(null);
}
console.timeEnd('Object.create');

console.time('Object Literal');
for (var i = 0; i < times; i += 1) {
  let obj = {};
}
console.timeEnd('Object Literal');

And it looks like Object.create is 10 times slower than object literal on my Ubuntu 14.04, 2 core, 4GB RAM machine. But, this will actually be a very small number. Only if this is not a problem, we could have fixed #2350 :(

JungMinu · 2015-11-15T02:56:21Z

@jasnell Sure, I will squash the commits soon :)

jasnell · 2015-11-15T02:59:00Z

Thank you!
On Nov 14, 2015 6:56 PM, "Minwoo Jung" [email protected] wrote:

@jasnell https://github.com/jasnell Sure, I will squash the commits
soon :)

—
Reply to this email directly or view it on GitHub
#3791 (comment).

trevnorris · 2015-11-15T03:31:17Z

@thefourtheye If you look at the IR generated by the optimizing compiler you'll find much of the second call has been optimized out completely. Also, if you look at actual execution time you'll see that the savings is in the low double digit nanoseconds. The call is sufficiently fast for how it's used here.

thefourtheye · 2015-11-15T04:36:17Z

@trevnorris Yup, the difference is not pronounced much. Probably, I'll have a shot at #2350 again.

JungMinu · 2015-11-15T04:37:53Z

@jasnell @trevnorris Thanks a lot 😄

thefourtheye · 2015-11-15T04:37:53Z

lib/util.js

@@ -159,9 +159,9 @@ function stylizeNoColor(str, styleType) {


 function arrayToHash(array) {
-  var hash = {};
+  var hash = Object.create(null);


Why not a Set instead?

JungMinu · 2015-11-15T04:56:38Z

@jasnell @trevnorris Sorry, because of the conflicts in my branch, I submitted a new PR, #3831

Fixes #3788 `arrayToHash()` needs to use `Object.create(null)` for its dictionary object. Refs: #3791 PR-URL: #3831 Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

WebReflection · 2016-03-15T15:11:42Z

has anyone tested var hash = {__proto__: null} performance ?

util: use Object.create(null) for dictionary object

e8091ed

Fixes #3788 `arrayToHash()` needs to use `Object.create(null)` for its dictionary object.

mscdex added the util Issues and PRs related to the built-in util module. label Nov 12, 2015

Fishrock123 reviewed Nov 12, 2015
View reviewed changes

add if val is __proto__

54285ca

add if val is `__proto__`, use `Object.create(null);`

pmq20 reviewed Nov 12, 2015
View reviewed changes

if array contains __proto__

c7de80b

if array contains __proto__, use Object.create(null)

Beautify codes

c61e8c2

Beautify codes

YurySolovyov reviewed Nov 12, 2015
View reviewed changes

JungMinu added 2 commits November 13, 2015 00:35

remove unused index

36ee9fe

remove spaces

7fa52db

util: use Object.create(null) for dictionary object

e412b4f

util: use Object.create(null) for dictionary object

paulfryzel reviewed Nov 12, 2015
View reviewed changes

jwueller reviewed Nov 13, 2015
View reviewed changes

remove unused

56995c7

util: use Object.create(null) for dictionary object

fe4cdc1

Merge branch 'patch-18' of https://github.com/JungMinu/node-1 into pa…

ec9f530

…tch-18

thefourtheye reviewed Nov 15, 2015
View reviewed changes

JungMinu closed this Nov 15, 2015

JungMinu deleted the patch-18 branch November 15, 2015 04:43

JungMinu restored the patch-18 branch November 15, 2015 04:44

JungMinu reopened this Nov 15, 2015

JungMinu closed this Nov 15, 2015


		array.forEach(function(val, idx) {
		array.forEach(function(val) {

util: use Object.create(null) for dictionary object #3791

util: use Object.create(null) for dictionary object #3791

Conversation

JungMinu commented Nov 12, 2015

evanlucas commented Nov 12, 2015

Fishrock123 commented Nov 12, 2015

Fishrock123 commented Nov 12, 2015

Fishrock123 commented Nov 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Nov 12, 2015

pmq20 commented Nov 12, 2015

YurySolovyov commented Nov 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trevnorris commented Nov 12, 2015

Fishrock123 commented Nov 12, 2015

jasnell commented Nov 12, 2015

trevnorris commented Nov 12, 2015

cjihrig commented Nov 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Nov 12, 2015

Choose a reason for hiding this comment

jasnell commented Nov 13, 2015

jasnell commented Nov 13, 2015

thefourtheye commented Nov 14, 2015

JungMinu commented Nov 15, 2015

jasnell commented Nov 15, 2015

trevnorris commented Nov 15, 2015

thefourtheye commented Nov 15, 2015

JungMinu commented Nov 15, 2015

Choose a reason for hiding this comment

JungMinu commented Nov 15, 2015

WebReflection commented Mar 15, 2016