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

http: simplify isInvalidPath #17525

Closed
wants to merge 1 commit into from

Conversation

starkwang
Copy link
Contributor

@starkwang starkwang commented Dec 7, 2017

The loop unrolling here shows no performance benefits in V8 6.3 any longer, so we can get rid of it. This PR is to simplify isInvalidPath without obvious performance regression.

                                               improvement confidence    p.value
 http/create-clientrequest.js n=1000000 len=1       -3.06 %          * 0.03418092
 http/create-clientrequest.js n=1000000 len=128      0.20 %            0.88729859
 http/create-clientrequest.js n=1000000 len=16      -0.50 %            0.66812482
 http/create-clientrequest.js n=1000000 len=32       2.21 %            0.06679157
 http/create-clientrequest.js n=1000000 len=64      -0.06 %            0.95083016
 http/create-clientrequest.js n=1000000 len=8       -0.61 %            0.49496794
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

http

@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Dec 7, 2017
@apapirovski
Copy link
Member

Could you confirm whether this applies to V8 6.2 or 6.3? The latter just landed on master and I'm pretty sure the unrolled version was faster on 6.2.

@starkwang
Copy link
Contributor Author

@apapirovski Sorry for my careless mistake, the benchmark above is based on V8 6.3.292

The loop unrolling shows no performance benefits in V8 6.3.
This change is to simplify `isInvalidPath` without performance
regression.
@starkwang starkwang force-pushed the http-client-simplify branch from b5659a6 to cdafdf3 Compare December 7, 2017 14:16
@starkwang
Copy link
Contributor Author

Another benchmark result after rebasing from the latest master:

                                               improvement confidence   p.value
 http/create-clientrequest.js n=1000000 len=1        0.11 %            0.8811138
 http/create-clientrequest.js n=1000000 len=128      0.09 %            0.8854386
 http/create-clientrequest.js n=1000000 len=16      -0.19 %            0.7372795
 http/create-clientrequest.js n=1000000 len=32       0.58 %            0.1907908
 http/create-clientrequest.js n=1000000 len=64       1.15 %            0.1873734
 http/create-clientrequest.js n=1000000 len=8       -0.45 %            0.5272168

@apapirovski
Copy link
Member

apapirovski commented Dec 7, 2017

Ok, thanks. I think this might not work well on V8 6.2 but also there's this PR that might get rid of that function all together: #16237 — I'm not sure what the situation around it is (that change is potentially breaking, although it fixes a bug) so that's something to keep in mind.

@BridgeAR
Copy link
Member

BridgeAR commented Dec 7, 2017

I guess #16237 is going to land soon. In general it is still good to know that loop unrolling is from now on not needed anymore!

I just added the dont-land-on-v8.x label to make sure it will not be backported in case it will land.

@starkwang
Copy link
Contributor Author

Closed due to #16237

@starkwang starkwang closed this Dec 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants