Skip to content

remove redundant isNaN check and add fast path for empty options#264

Merged
mcollina merged 14 commits intofastify:masterfrom
gurgunday:finite
Dec 7, 2023
Merged

remove redundant isNaN check and add fast path for empty options#264
mcollina merged 14 commits intofastify:masterfrom
gurgunday:finite

Conversation

@gurgunday
Copy link
Copy Markdown
Member

@gurgunday gurgunday commented Nov 24, 2023 •
edited
Loading

isFinite already covers NaN

Also added a fast path for the case where options is undefined

Benchmark benchmark/cookie

Before:

Running 10s test @ http://127.0.0.1:5001
10 connections


┌─────────┬──────┬──────┬───────┬──────┬─────────┬─────────┬───────┐
│ Stat    │ 2.5% │ 50%  │ 97.5% │ 99%  │ Avg     │ Stdev   │ Max   │
├─────────┼──────┼──────┼───────┼──────┼─────────┼─────────┼───────┤
│ Latency │ 0 ms │ 0 ms │ 0 ms  │ 0 ms │ 0.01 ms │ 0.12 ms │ 20 ms │
└─────────┴──────┴──────┴───────┴──────┴─────────┴─────────┴───────┘
┌───────────┬─────────┬─────────┬─────────┬─────────┬──────────┬──────────┬─────────┐
│ Stat      │ 1%      │ 2.5%    │ 50%     │ 97.5%   │ Avg      │ Stdev    │ Min     │
├───────────┼─────────┼─────────┼─────────┼─────────┼──────────┼──────────┼─────────┤
│ Req/Sec   │ 35,807  │ 35,807  │ 49,311  │ 52,735  │ 47,676.8 │ 5,391.76 │ 35,785  │
├───────────┼─────────┼─────────┼─────────┼─────────┼──────────┼──────────┼─────────┤
│ Bytes/Sec │ 7.77 MB │ 7.77 MB │ 10.7 MB │ 11.4 MB │ 10.3 MB  │ 1.17 MB  │ 7.77 MB │
└───────────┴─────────┴─────────┴─────────┴─────────┴──────────┴──────────┴─────────┘

Req/Bytes counts sampled once per second.
# of samples: 10

477k requests in 10.01s, 103 MB read

Now:

Running 10s test @ http://127.0.0.1:5001
10 connections


┌─────────┬──────┬──────┬───────┬──────┬─────────┬─────────┬───────┐
│ Stat    │ 2.5% │ 50%  │ 97.5% │ 99%  │ Avg     │ Stdev   │ Max   │
├─────────┼──────┼──────┼───────┼──────┼─────────┼─────────┼───────┤
│ Latency │ 0 ms │ 0 ms │ 0 ms  │ 0 ms │ 0.01 ms │ 0.09 ms │ 20 ms │
└─────────┴──────┴──────┴───────┴──────┴─────────┴─────────┴───────┘
┌───────────┬────────┬────────┬─────────┬─────────┬───────────┬─────────┬────────┐
│ Stat      │ 1%     │ 2.5%   │ 50%     │ 97.5%   │ Avg       │ Stdev   │ Min    │
├───────────┼────────┼────────┼─────────┼─────────┼───────────┼─────────┼────────┤
│ Req/Sec   │ 43,327 │ 43,327 │ 49,823  │ 53,279  │ 49,444.37 │ 2,829.3 │ 43,320 │
├───────────┼────────┼────────┼─────────┼─────────┼───────────┼─────────┼────────┤
│ Bytes/Sec │ 9.4 MB │ 9.4 MB │ 10.8 MB │ 11.6 MB │ 10.7 MB   │ 615 kB  │ 9.4 MB │
└───────────┴────────┴────────┴─────────┴─────────┴───────────┴─────────┴────────┘

Req/Bytes counts sampled once per second.
# of samples: 11

544k requests in 11.01s, 118 MB read

jsumners
jsumners reviewed Nov 24, 2023
View reviewed changes
Comment thread cookie.js Outdated
Comment thread cookie.js
@gurgunday gurgunday requested a review from Uzlopak December 7, 2023 18:29
@gurgunday
Copy link
Copy Markdown
Member Author

gurgunday commented Dec 7, 2023 •
edited
Loading

Can you take a look?

Changes:

  • Don't create empty objects
  • Call regex tests only when string is not empty
  • Don't call isNaN when isFinite is called
  • Fast path for when options is undefined|null
  • Math.trunc is a little faster than .floor
  • +var has better bytecode than var - 0

mcollina
mcollina approved these changes Dec 7, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina merged commit 088871f into fastify:master Dec 7, 2023
@gurgunday gurgunday deleted the finite branch January 20, 2024 09:16
@renovate renovate bot mentioned this pull request Apr 24, 2024
Open
This was referenced Aug 21, 2024
Open
Open
Open
@WesleyR10 WesleyR10 mentioned this pull request Sep 12, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jsumners jsumners jsumners left review comments

@mcollina mcollina mcollina approved these changes

@Uzlopak Uzlopak Awaiting requested review from Uzlopak

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@gurgunday @mcollina @jsumners