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

break: do not decode req.path by default #172

Merged
merged 4 commits into from
Aug 13, 2021
Merged

Conversation

benmccann
Copy link

Fixes #142

packages/polka/index.js Outdated Show resolved Hide resolved
Copy link
Owner

@lukeed lukeed left a comment

Choose a reason for hiding this comment

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

I have this change locally, but with some performance optimizations too.

On its own, decodeURIComponent is expensive & needs to be done sparingly. But this (the suggestion) can land in the meantime before those optimizations are ready to land.

@codecov-commenter
Copy link

Codecov Report

Merging #172 (83751fb) into next (4885177) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              next      #172   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines          105       107    +2     
=========================================
+ Hits           105       107    +2     
Impacted Files Coverage Δ
packages/polka/index.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4885177...83751fb. Read the comment docs.

@lukeed lukeed changed the title [fix] properly parse path break: do not decode req.path by default Aug 13, 2021
@lukeed lukeed merged commit 6ef32a6 into lukeed:next Aug 13, 2021
@benmccann benmccann deleted the path-parsing branch August 13, 2021 21:26
@lukeed
Copy link
Owner

lukeed commented Aug 13, 2021

Just for the record, the effect of this PR:

# before

Running 10s test @ http://localhost:3000/
  4 threads and 4 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   115.46us  161.68us   8.41ms   97.94%
    Req/Sec     9.28k     1.71k   12.81k    73.27%
  372959 requests in 10.10s, 45.17MB read
Requests/sec:  36927.84
Transfer/sec:      4.47MB

# after

Running 10s test @ http://localhost:3000/
  4 threads and 4 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   147.72us  204.77us  11.45ms   97.27%
    Req/Sec     7.46k     1.43k   11.18k    73.51%
  299801 requests in 10.10s, 36.31MB read
Requests/sec:  29683.11
Transfer/sec:      3.60MB

lukeed added a commit that referenced this pull request Aug 13, 2021
@lukeed
Copy link
Owner

lukeed commented Aug 13, 2021

After the linked commit:

Running 10s test @ http://localhost:3000/
  4 threads and 4 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   109.20us   53.27us   1.68ms   90.50%
    Req/Sec     9.22k     1.49k   12.64k    67.57%
  370649 requests in 10.10s, 44.89MB read
Requests/sec:  36800.70
Transfer/sec:      4.46MB

lukeed added a commit to lukeed/sirv that referenced this pull request Aug 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants