zlib: make constants keep readonly by JacksonTian · Pull Request #1361 · nodejs/node

JacksonTian · 2015-04-07T06:00:36Z

In zlib module, a dozen constants were exported to user land,
If user change the constant, maybe lead unexcepted error.

Make them readonly and freezon.

In zlib module, a dozen constants were exported to user land, If user change the constant, maybe lead unexcepted error. Make them readonly and freezon.

bnoordhuis · 2015-04-07T09:01:07Z

LGTM. Anyone else from @iojs/collaborators want to weigh in?

jbergstroem · 2015-04-07T09:07:59Z

I like it. Don't think we need a full jenkins run for this, right? LGTM.

shigeki · 2015-04-07T09:10:56Z

How about to add configurable : false ?

JacksonTian · 2015-04-07T09:44:12Z

@shigeki the configurable defaults to false.

shigeki · 2015-04-07T09:58:38Z

@JacksonTian That's fine. LGTM.
@jbergstroem I agree, it's not needed. Could you merge this?

In zlib module, a dozen constants were exported to user land, If user change the constant, maybe lead unexcepted error. Make them readonly and freezon. PR-URL: #1361 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

shigeki · 2015-04-07T15:08:05Z

Thanks, I made tests locally and all is fine. Landed in 372bf83.

indutny · 2015-04-07T15:09:47Z

Wouldn't it slow down the lookup? I remember that I did similar thing for node 0.4, but we reverted it because of slowdown :)

shigeki · 2015-04-07T15:11:20Z

Well, I will make a benchmark.

shigeki · 2015-04-07T15:47:46Z

For quick benchmark as blow, this commit gets about 4.6% slower in the lookup performance.
I think it is not so much serious performance degradation but it really slow down the lookup.
How do everyone think about this?

ohtsu@ubuntu:~/github/io.js$ ./iojs benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 292205.78329
ohtsu@ubuntu:~/github/io.js$ ./iojs benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 311704.56550
ohtsu@ubuntu:~/github/io.js$ ./iojs benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 263579.49684
ohtsu@ubuntu:~/github/io.js$ ./iojs benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 273116.33671
ohtsu@ubuntu:~/github/io.js$ ./iojs benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 317763.51096

ave. 291673.93866/sec

after reverting 372bf83818fd87bae5ad7cedadfa121ddc5d4345
ohtsu@ubuntu:~/github/io.js$ ./iojs benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 329755.18151
ohtsu@ubuntu:~/github/io.js$ ./iojs benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 252276.96915
ohtsu@ubuntu:~/github/io.js$ ./iojs benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 326891.71396
ohtsu@ubuntu:~/github/io.js$ ./iojs benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 290428.49166
ohtsu@ubuntu:~/github/io.js$ ./iojs benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 329554.20748
ave. 305781.312752/sec

291673.93866/305781.312752=0.95386449889617157777803953405404

var common = require('../common.js');
var zlib = require('zlib');
var assert = require('assert');

var bench = common.createBenchmark(main, {n: [2e6]});
var zlib_codes = {
  '0': 'Z_OK',
  '1': 'Z_STREAM_END',
  '2': 'Z_NEED_DICT',
  Z_OK: 0,
  Z_STREAM_END: 1,
  Z_NEED_DICT: 2,
  Z_ERRNO: -1,
  Z_STREAM_ERROR: -2,
  Z_DATA_ERROR: -3,
  Z_MEM_ERROR: -4,
  Z_BUF_ERROR: -5,
  Z_VERSION_ERROR: -6,
  '-1': 'Z_ERRNO',
  '-2': 'Z_STREAM_ERROR',
  '-3': 'Z_DATA_ERROR',
  '-4': 'Z_MEM_ERROR',
  '-5': 'Z_BUF_ERROR',
  '-6': 'Z_VERSION_ERROR'
};

function main(conf) {
  var n = conf.n | 0;

  bench.start();
  for (var i = 0; i < n; i += 1) {
    for(var key in zlib.codes) {
      assert.equal(zlib.codes[key], zlib_codes[key]);
    }
  }
  bench.end(n);
}

indutny · 2015-04-07T15:49:12Z

Thanks @shigeki! I don't think that it would take much time then. LGTM

shigeki · 2015-04-07T15:53:11Z

@indutny Okay, thanks for pointing out to care about performance. I missed it.

jbergstroem · 2015-04-07T22:18:34Z

@shigeki sorry for the silence; blaming timezones.

JacksonTian · 2015-04-08T00:26:26Z

how about use Object.keys instead of for/in in benchmark? For/in is slow case.

发自我的 iPhone

在 2015年4月7日，下午11:47，Shigeki Ohtsu notifications@github.com 写道：

For quick benchmark as blow, this commit gets about 4.6% slower in the lookup performance.
I think it is not so much serious performance degradation but it really slow down the lookup.
How do everyone think about this?

ohtsu@ubuntu:/github/io.js$ ./iojs benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 292205.78329
ohtsu@ubuntu:/github/io.js$ ./iojs benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 311704.56550
ohtsu@ubuntu:/github/io.js$ ./iojs benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 263579.49684
ohtsu@ubuntu:/github/io.js$ ./iojs benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 273116.33671
ohtsu@ubuntu:~/github/io.js$ ./iojs benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 317763.51096

ave. 291673.93866/sec

after reverting 372bf83
ohtsu@ubuntu:/github/io.js$ ./iojs benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 329755.18151
ohtsu@ubuntu:/github/io.js$ ./iojs benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 252276.96915
ohtsu@ubuntu:/github/io.js$ ./iojs benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 326891.71396
ohtsu@ubuntu:/github/io.js$ ./iojs benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 290428.49166
ohtsu@ubuntu:~/github/io.js$ ./iojs benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 329554.20748
ave. 305781.312752/sec

291673.93866/305781.312752=0.95386449889617157777803953405404
var common = require('../common.js');
var zlib = require('zlib');
var assert = require('assert');

var bench = common.createBenchmark(main, {n: [2e6]});
var zlib_codes = {
'0': 'Z_OK',
'1': 'Z_STREAM_END',
'2': 'Z_NEED_DICT',
Z_OK: 0,
Z_STREAM_END: 1,
Z_NEED_DICT: 2,
Z_ERRNO: -1,
Z_STREAM_ERROR: -2,
Z_DATA_ERROR: -3,
Z_MEM_ERROR: -4,
Z_BUF_ERROR: -5,
Z_VERSION_ERROR: -6,
'-1': 'Z_ERRNO',
'-2': 'Z_STREAM_ERROR',
'-3': 'Z_DATA_ERROR',
'-4': 'Z_MEM_ERROR',
'-5': 'Z_BUF_ERROR',
'-6': 'Z_VERSION_ERROR'
};

function main(conf) {
var n = conf.n | 0;

bench.start();
for (var i = 0; i < n; i += 1) {
for(var key in zlib.codes) {
assert.equal(zlib.codes[key], zlib_codes[key]);
}
}
bench.end(n);
}
—
Reply to this email directly or view it on GitHub.

shigeki · 2015-04-08T01:12:39Z

@JacksonTian It makes both performance get better owing to non-enumeration of prototype chain. A new benchmark shows that difference is about 7.5% slower in lookups. It seems that this number varies between 13% and 5%.

--- a/benchmark/zlib/zlib_lookup.js
+++ b/benchmark/zlib/zlib_lookup.js
@@ -29,9 +29,9 @@ function main(conf) {

   bench.start();
   for (var i = 0; i < n; i += 1) {
-    for(var key in zlib.codes) {
+    Object.keys(zlib.codes).forEach(function(key) {
       assert.equal(zlib.codes[key], zlib_codes[key]);
-    }
+    });
   }
   bench.end(n);
 }

ohtsu@ubuntu:~/github/io.js$ ./iojs ./benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 343327.09061
ohtsu@ubuntu:~/github/io.js$ ./iojs ./benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 363036.73678
ohtsu@ubuntu:~/github/io.js$ ./iojs ./benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 355959.65101
ohtsu@ubuntu:~/github/io.js$ ./iojs ./benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 349914.96226
ohtsu@ubuntu:~/github/io.js$ ./iojs ./benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 353198.02292

ave. 353087.292716

Reverted 
ohtsu@ubuntu:~/github/io.js$ ./iojs ./benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 381779.95576
ohtsu@ubuntu:~/github/io.js$ ./iojs ./benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 388148.84485
ohtsu@ubuntu:~/github/io.js$ ./iojs ./benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 386636.61563
ohtsu@ubuntu:~/github/io.js$ ./iojs ./benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 379509.47249
ohtsu@ubuntu:~/github/io.js$ ./iojs ./benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 370719.73390

ave. 381358.924526

353087.292716/381358.924526=0.92586608050371581616651897385889

@jbergstroem Never mind. I thought you had been in online at that time.

trevnorris · 2015-04-08T10:08:21Z

@shigeki The .forEach() is going to kill your performance for a couple reasons. Try enumerating over the Object.keys() array with a for() loop and I'll guarantee that performance will improve.

shigeki · 2015-04-08T13:15:09Z

@trevnorris Wow, I did not realize that the forEach has so much performance degradation. Thanks.

A new benchmark shows about 11% performance loss in lookup with this commit. I can't decide if it is serious.

ohtsu@ubuntu:~/github/io.js$ ./iojs ./benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 625939.69392
ohtsu@ubuntu:~/github/io.js$ ./iojs ./benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 669428.59785
ohtsu@ubuntu:~/github/io.js$ ./iojs ./benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 661328.00027
ohtsu@ubuntu:~/github/io.js$ ./iojs ./benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 656103.20345
ohtsu@ubuntu:~/github/io.js$ ./iojs ./benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 632819.48158

ave. 649123.795414

Reverted
ohtsu@ubuntu:~/github/io.js$ ./iojs ./benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 702057.08514
ohtsu@ubuntu:~/github/io.js$ ./iojs ./benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 755169.90155
ohtsu@ubuntu:~/github/io.js$ ./iojs ./benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 742403.59253
ohtsu@ubuntu:~/github/io.js$ ./iojs ./benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 745905.08977
ohtsu@ubuntu:~/github/io.js$ ./iojs ./benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 693043.52458

ave. 727715.838714

649123.795414/727715.838714=0.89200174145050111799867436958126

--- a/benchmark/zlib/zlib_lookup.js
+++ b/benchmark/zlib/zlib_lookup.js
@@ -29,9 +29,10 @@ function main(conf) {

   bench.start();
   for (var i = 0; i < n; i += 1) {
-    Object.keys(zlib.codes).forEach(function(key) {
-      assert.equal(zlib.codes[key], zlib_codes[key]);
-    });
+    var keys = Object.keys(zlib.codes);
+    for(var j = 0; j < keys.length; ++j) {
+      assert.equal(zlib.codes[keys[j]], zlib_codes[keys[j]]);
+    };
   }
   bench.end(n);
 }

JacksonTian · 2015-04-08T14:06:14Z

The case shows for/in, forEach ’s performance effect more than lookup a lot.

在 2015年4月8日，下午9:15，Shigeki Ohtsu <notifications@github.com mailto:notifications@github.com> 写道：

@trevnorris https://github.com/trevnorris Wow, I did not realize that the forEach has so much performance degradation. Thanks.

A new benchmark shows about 11% performance loss in lookup with this commit. I can't decide if it is serious.

ohtsu@ubuntu:/github/io.js$ ./iojs ./benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 625939.69392
ohtsu@ubuntu:/github/io.js$ ./iojs ./benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 669428.59785
ohtsu@ubuntu:/github/io.js$ ./iojs ./benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 661328.00027
ohtsu@ubuntu:/github/io.js$ ./iojs ./benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 656103.20345
ohtsu@ubuntu:~/github/io.js$ ./iojs ./benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 632819.48158

ave. 649123.795414

Reverted
ohtsu@ubuntu:/github/io.js$ ./iojs ./benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 702057.08514
ohtsu@ubuntu:/github/io.js$ ./iojs ./benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 755169.90155
ohtsu@ubuntu:/github/io.js$ ./iojs ./benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 742403.59253
ohtsu@ubuntu:/github/io.js$ ./iojs ./benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 745905.08977
ohtsu@ubuntu:~/github/io.js$ ./iojs ./benchmark/zlib/zlib_lookup.js
zlib/zlib_lookup.js n=2000000: 693043.52458

ave. 727715.838714

649123.795414/727715.838714=0.89200174145050111799867436958126
--- a/benchmark/zlib/zlib_lookup.js
+++ b/benchmark/zlib/zlib_lookup.js
@@ -29,9 +29,10 @@ function main(conf) {

bench.start();
for (var i = 0; i < n; i += 1) {
Object.keys(zlib.codes).forEach(function(key) {
 assert.equal(zlib.codes[key], zlib_codes[key]);
});

var keys = Object.keys(zlib.codes);

for(var j = 0; j < keys.length; ++j) {
 assert.equal(zlib.codes[keys[j]], zlib_codes[keys[j]]);
};
}
bench.end(n);
}
—
Reply to this email directly or view it on GitHub zlib: make constants keep readonly #1361 (comment).

zlib: make constants keep readonly

b3c2bcb

In zlib module, a dozen constants were exported to user land, If user change the constant, maybe lead unexcepted error. Make them readonly and freezon.

brendanashworth added the zlib Issues and PRs related to the zlib subsystem. label Apr 7, 2015

shigeki closed this Apr 7, 2015

chrisdickinson mentioned this pull request Apr 7, 2015

doc: first pass at stability policy nodejs/dev-policy#21

Merged

JacksonTian deleted the zlib branch April 8, 2015 07:29

rvagg mentioned this pull request Apr 11, 2015

Release proposal: v1.7.0 #1396

Closed

snyk-bot mentioned this pull request Apr 26, 2023

[Snyk] Fix for 6 vulnerabilities aliscco/alisco-node#239

Open

Uh oh!

Conversation

JacksonTian commented Apr 7, 2015

Uh oh!

bnoordhuis commented Apr 7, 2015

Uh oh!

jbergstroem commented Apr 7, 2015

Uh oh!

shigeki commented Apr 7, 2015

Uh oh!

JacksonTian commented Apr 7, 2015

Uh oh!

shigeki commented Apr 7, 2015

Uh oh!

shigeki commented Apr 7, 2015

Uh oh!

indutny commented Apr 7, 2015

Uh oh!

shigeki commented Apr 7, 2015

Uh oh!

shigeki commented Apr 7, 2015

Uh oh!

indutny commented Apr 7, 2015

Uh oh!

shigeki commented Apr 7, 2015

Uh oh!

jbergstroem commented Apr 7, 2015

Uh oh!

JacksonTian commented Apr 8, 2015

Uh oh!

shigeki commented Apr 8, 2015

Uh oh!

trevnorris commented Apr 8, 2015

Uh oh!

shigeki commented Apr 8, 2015

Uh oh!

JacksonTian commented Apr 8, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants