-
Notifications
You must be signed in to change notification settings - Fork 30k
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
url: remove unused code from autoEscapeStr #15086
Conversation
Have you run the relevant benchmarks in |
@TimothyGu since this is a first time contributor I ran the benchmarks myself. To @clakech - after building Node you can run The only relevant one I could find in I ran that twice on master and on master + this patch and got:
Above everything, I'm not sure how stable it is - I don't think there should be a regression here and I don't think there is - but I'm not really sure how significant you'd like the statistical analysis to me. |
@benjamingr Yes that is the right benchmark. @clakech Apologies. I'm currently on vacation, or otherwise I would have volunteered to run the benchmark myself. One can get more statistically sound results by using the benchmark/compare.js script. Some documentation around using that script (and benchmarks in general) can be found in https://github.com/nodejs/node/blob/master/doc/guides/writing-and-running-benchmarks.md#comparing-nodejs-versions. Some tips:
The script does require R to be installed on the system though. If that is not possible, simply post the generated CSV file on Gist and I'm sure one of us can run the R script to get the final results. |
Hi, Thanks for your answers. I am here to learn so I am happy to follow your advices. Right now I a build master and my branch to compares url.parse result using R. Results may land here soon ;) |
ouch, bad results for performance:
|
The fact that none of the results have any *'s under "confidence" means we should be in the clear as the performance decrease is not statistically significant. You might want to increase the number of runs, however, to make the results more precise. Also, ignore about the whatwg results: its code was not changed by this PR. You can add Another thing we might have to make sure is that the code path that was changed in this PR is actually tested by the benchmarks… |
I retried with --set method=legacy + a tiny modification for perf (see fixup commit):
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that there is no perf regression and code looks good - approving.
Just another try to execute benchmark to double check perf and it seems good now:
@benjamingr I added a tiny commit for perf, just to be sure you saw it 4b71730 |
CI started at https://ci.nodejs.org/job/node-test-pull-request/9881/ |
Test regression spotted |
@clakech did you mean to close the PR? |
@clakech if you would like assistance with understanding the CI result or how to deal with it - please let us know. In Node.js pull requests are typically left open for 48 hours to discussion so collaborators have a chance to raise objections or concerns. |
Ok, I closed it because of the failing test, I don't want this land 'as is' on master. Will investigate soon. Thanks for helping
|
No problem, we won't land it as is. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Waiting until the tests are fixed. Let us know if you need help with that.
4b71730
to
2333e4c
Compare
Now tests are OK and here is the perf bench result:
|
lib/url.js
Outdated
@@ -539,13 +536,11 @@ function autoEscapeStr(rest) { | |||
break; | |||
} | |||
} | |||
if (lastEscapedPos === 0) // Nothing has been escaped. | |||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to check if it is a good idea to keep the if but return rest here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I tried with this check and here is the perf result:
node benchmark/compare.js --old ../node/out/Release/node --new out/Release/node --filter legacy-vs-whatwg-url-parse.js --runs 10 --set method=legacy url > result.csv
[00:03:31|% 100| 1/1 files | 20/20 runs | 9/9 configs]: Done
cat result.csv | Rscript benchmark/compare.R
improvement confidence p.value
url/legacy-vs-whatwg-url-parse.js n=100000 method="legacy" type="auth" -2.80 % 0.8940666
url/legacy-vs-whatwg-url-parse.js n=100000 method="legacy" type="dot" 10.52 % 0.5992881
url/legacy-vs-whatwg-url-parse.js n=100000 method="legacy" type="file" -5.34 % 0.7988524
url/legacy-vs-whatwg-url-parse.js n=100000 method="legacy" type="idn" -3.03 % 0.8843592
url/legacy-vs-whatwg-url-parse.js n=100000 method="legacy" type="javascript" 7.21 % 0.7159490
url/legacy-vs-whatwg-url-parse.js n=100000 method="legacy" type="long" -3.54 % 0.8643568
url/legacy-vs-whatwg-url-parse.js n=100000 method="legacy" type="percent" 11.36 % 0.5700568
url/legacy-vs-whatwg-url-parse.js n=100000 method="legacy" type="short" -2.29 % 0.9104903
url/legacy-vs-whatwg-url-parse.js n=100000 method="legacy" type="ws" 11.91 % 0.5628974
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I retry with --runs 100 and now this is better:
cat result.csv | Rscript benchmark/compare.R
improvement confidence p.value
url/legacy-vs-whatwg-url-parse.js n=100000 method="legacy" type="auth" -0.99 % 0.2587248
url/legacy-vs-whatwg-url-parse.js n=100000 method="legacy" type="dot" -0.50 % 0.6006786
url/legacy-vs-whatwg-url-parse.js n=100000 method="legacy" type="file" 0.87 % 0.3582855
url/legacy-vs-whatwg-url-parse.js n=100000 method="legacy" type="idn" 0.03 % 0.9783704
url/legacy-vs-whatwg-url-parse.js n=100000 method="legacy" type="javascript" -0.58 % 0.5305400
url/legacy-vs-whatwg-url-parse.js n=100000 method="legacy" type="long" -0.20 % 0.7804390
url/legacy-vs-whatwg-url-parse.js n=100000 method="legacy" type="percent" 1.31 % 0.1699119
url/legacy-vs-whatwg-url-parse.js n=100000 method="legacy" type="short" 0.47 % 0.5801138
url/legacy-vs-whatwg-url-parse.js n=100000 method="legacy" type="ws" -0.57 % 0.5023730
Hey, can you push the recent changes? |
@benjamingr "Done", I just remove the perf tuning commit that introduce a regression... rollbacked to the first state of this PR now ;) |
I tried to bench with --runs 100 as suggested by @TimothyGu #15086 (comment) and with the modification proposed by @BridgeAR #15086 (comment) to be sure:
but Comparing this perf result with the code without this @BridgeAR suggestion it is worse:
|
CI looks good to me (failure is unrelated https://ci.nodejs.org/job/node-test-commit-arm/11827//console ). Going to approve. |
@benjamingr I think @BridgeAR suggestion was good because it seems like it improves the performance compared to the actuel code (see #15086 (comment)) So I added this "if (lastEscapedPos === 0)" again in this fixup 5eeedc8 We may need to relaunch CI again #sorry |
@lpinca the build result is aborted because of https://ci.nodejs.org/job/node-test-binary-arm/RUN_SUBSET=2,label=pi1-raspbian-wheezy/9930/console
Could someone reschedule another CI build please ? #thx |
@clakech we can ignore infrastructure related failures. Anyway here is a new CI: https://ci.nodejs.org/job/node-test-pull-request/9903/ |
OK thanks @lpinca, we still got the same build error with ClosedChannelException |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks much better. LGTM!
Landed in ed084a0 |
PR-URL: nodejs#15086 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: nodejs/node#15086 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #15086 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #15086 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #15086 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
autoEscapeStr returned undefined when no character are escaped but this
behaviour seems useless when used into the parse function
Tests are OK on Mac, untested on Windows yet