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: improve priority queue performance #44506

Closed
wants to merge 1 commit into from
Closed

lib: improve priority queue performance #44506

wants to merge 1 commit into from

Conversation

falsandtru
Copy link
Contributor

                                  confidence improvement accuracy (*)   (**)  (***)
util/priority-queue.js n=1000000        ***      6.66 %       ±2.93% ±3.90% ±5.08%

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Sep 3, 2022
@falsandtru
Copy link
Contributor Author

Path: parallel/test-vm-timeout-escape-promise-module
Error: --- stderr ---
(node:130552) ExperimentalWarning: VM Modules is an experimental feature. This feature could change at any time

This test often makes false positive #41839.

@falsandtru
Copy link
Contributor Author

Can you approve?

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM while the longer version suggested by @devsnek is still nicer to read for me and I would rather keep the benchmark runs, if possible.

@@ -3,10 +3,10 @@
const common = require('../common');

const bench = common.createBenchmark(main, {
n: [1e5]
n: [1e6]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have to increase the value? Our benchmarks have the tendency of taking very long overall.

Copy link
Contributor Author

@falsandtru falsandtru Sep 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value is needed to get *** confidence stably. And the benchmark only takes one minute even after the change.

                                 confidence improvement accuracy (*)   (**)  (***)
util/priority-queue.js n=100000                 0.46 %       ±2.98% ±3.98% ±5.20%

                                 confidence improvement accuracy (*)   (**)  (***)
util/priority-queue.js n=100000        ***      4.36 %       ±1.99% ±2.65% ±3.46%
                                  confidence improvement accuracy (*)   (**)  (***)
util/priority-queue.js n=1000000        ***      9.43 %       ±3.13% ±4.17% ±5.43%

@@ -11,7 +11,7 @@ const {
// just a single criteria.

module.exports = class PriorityQueue {
#compare = (a, b) => a - b;
#compare = (a, b) => (a > b ? 1 : a < b ? -1 : 0);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know such a common static code can commonly be compact as an idiom. How about, other members?

@mscdex
Copy link
Contributor

mscdex commented Sep 8, 2022

https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1185/ and https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1186/ don't seem to be able to reproduce the improvement.

@falsandtru
Copy link
Contributor Author

falsandtru commented Sep 8, 2022

Since those results have no confidence, the sample size is probably insufficient. Let's set runs to 100 as my previous pr did.

https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1180/

Anyway the default sample size(runs value) 30 is too small. It have to be 60 or more.

@mscdex
Copy link
Contributor

mscdex commented Sep 8, 2022

I re-ran it with 100 iterations and now it looks the same or worse:

https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1188/console

util/priority-queue.js n=1000000         **     -2.08 %       ±1.29% ±1.70% ±2.19%

https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1189/console

util/priority-queue.js n=1000000                -1.36 %       ±1.50% ±1.99% ±2.55%

@falsandtru
Copy link
Contributor Author

I found that CI uses Ubuntu 16 and gcc/g++8, but I'm using Ubuntu 20.04 and gcc/g++ 9.4.0. CI may be inaccurate because of its legacy environment. Can anyone run benchmark on Ubuntu 20 or later and gcc/g++ 9.4.0?

@falsandtru
Copy link
Contributor Author

How to

  1. git checkout main
  2. ./configure && make -j4 && mv out/Release/node ./node-main
  3. Apply the changes.
  4. ./configure && make -j4 && mv out/Release/node ./node-pr
  5. node benchmark/compare.js --old ./node-main --new ./node-pr --filter "prio" util > compare-pr.csv && node-benchmark-compare compare-pr.csv
$ node benchmark/compare.js --old ./node-main --new ./node-pr --filter "prio" util > compare-pr.csv && node-benchmark-compare compare-pr.csv
[00:01:05|% 100| 1/1 files | 60/60 runs | 1/1 configs]: Done
                                  confidence improvement accuracy (*)   (**)  (***)
util/priority-queue.js n=1000000        ***      9.66 %       ±2.80% ±3.72% ±4.84%

Be aware that when doing many comparisons the risk of a false-positive result increases.
In this case, there are 1 comparisons, you can thus expect the following amount of false-positive results:
  0.05 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.01 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)

$ g++ --version
g++ (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0
Copyright (C) 2019 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

$ gcc --version
gcc (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0
Copyright (C) 2019 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

@mscdex
Copy link
Contributor

mscdex commented Sep 9, 2022

I ran the benchmark several times locally for 100 iterations each and on my machine (with gcc 9.4.0) I consistently get results like:

util/priority-queue.js n=1000000        ***     -4.60 %       ±0.15% ±0.20% ±0.26%

At any rate, it seems the code change here isn't a clear performance win.

@falsandtru
Copy link
Contributor Author

I couldn't reproduce the improvement on GitHub Actions. I've no idea what made the difference.

@falsandtru falsandtru closed this Sep 9, 2022
@falsandtru falsandtru deleted the heap branch September 9, 2022 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants