Repository navigation
lib: refactor PriorityQueue to use private field - #43889
Conversation
commented
Jul 18, 2022
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
|
Bnechmark CI(timers): https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1167/ Results (no significant difference) |
commented
Jul 18, 2022
commented
Jul 18, 2022
commented
Jul 18, 2022
commented
Jul 19, 2022
@RaisinTen I see this pr introduces perf regression, but I don't understand what "How far can we backport this change without introducing perf regressions?" mean. Could you elaborate on this sentence? |
commented
Jul 19, 2022
Having private fields in your source code has been for a long time a performance bottleneck because V8 wasn't able to optimize the class as efficiently as when using |
commented
Jul 19, 2022
I see the benchmark compare result shows improvement is |
commented
Jul 19, 2022
No, because the uncertainty of the value ( |
commented
Jul 19, 2022
|
@targos Could you show me an example that does introduce perf regression? I am not familiar with node benchmark comparison. |
Here's an example: #41745 (comment) TL;DR: you can disregard all the lines in the benchmark result that have no stars ( The gist of it is that the difference of performance in the benchmarks is better explain by random flukes when running the CI than the actual change itself. If we were to re-run the CI, most likely we won't get |
|
I meant that this PR doesn't introduce any perf regressions but there is a chance that it might do so if we backport it to an earlier version of node where V8 doesn't apply the required optimizations. Not backporting to LTS sounds okay too. |
commented
Jul 19, 2022
|
I have learnt something new. Thank you |
commented
Jul 23, 2022
commented
Jul 23, 2022
commented
Jul 23, 2022
|
Landed in df7c49c |
Today I am reading the code of PriorityQueue, I found use symbol to achieve private fields is not easy to read. so I refactor to use private field to improve readability.