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

feat: improve performance of response data parsing #1580

Merged
merged 30 commits into from
Oct 21, 2023

Conversation

arthurschreiber
Copy link
Collaborator

@arthurschreiber arthurschreiber commented Oct 5, 2023

This replaces the current token parsing code (including value and metadata parsers) to use the "try parsing a single token, fail if not enough data is buffered, retry once more data comes in" approach described over in #1499 (comment).

TODOs

  • Fix CI failures 😬 Seems like there's some tokens that are currently not parsed correctly?
  • Remove remaining references to parser.streamBuffer.waitForData
  • Implement NBCROW token parsing.
  • Change buf: Buffer | BufferList type to only buf: Buffer, I don't think we ever pass a BufferList object here.
  • Change Result to be a proper class? So we can do return Result(value, offset). Probably should be benchmarked to see if it makes any difference.

Results

On benchmarks that measure the full request-response cycle (like the select-many-rows.js benchmark), we can see an improvement ranging between a few percent up to 50% increase in iterations per second. Actual numbers will vary depending on various variables.

Here are example benchmark results from my machine:

Node.js v16.20.0 master branch
query/select-many-rows.js size=10 n=10: 732.5808750971586 (minor: 0 - 0ms, major: 0 - 0ms, incremental: 0 - 0ms)
query/select-many-rows.js size=100 n=10: 236.7257682532375 (minor: 1 - 2.320625999942422ms, major: 0 - 0ms, incremental: 0 - 0ms)
query/select-many-rows.js size=1000 n=10: 108.46899089030772 (minor: 3 - 6.047306000255048ms, major: 0 - 0ms, incremental: 0 - 0ms)
query/select-many-rows.js size=10000 n=10: 23.63362124251892 (minor: 28 - 10.39379300083965ms, major: 0 - 0ms, incremental: 0 - 0ms)
query/select-many-rows.js size=10 n=100: 943.124882268542 (minor: 1 - 0.5346679999493062ms, major: 0 - 0ms, incremental: 0 - 0ms)
query/select-many-rows.js size=100 n=100: 674.9636528698111 (minor: 7 - 3.505343999247998ms, major: 0 - 0ms, incremental: 0 - 0ms)
query/select-many-rows.js size=1000 n=100: 196.5643086662155 (minor: 30 - 17.99759300146252ms, major: 0 - 0ms, incremental: 0 - 0ms)
query/select-many-rows.js size=10000 n=100: 27.498503692295312 (minor: 271 - 105.15239100111648ms, major: 0 - 0ms, incremental: 0 - 0ms)
query/select-many-rows.js size=10 n=1000: 1,498.2127961884485 (minor: 10 - 11.73080400051549ms, major: 0 - 0ms, incremental: 0 - 0ms)
query/select-many-rows.js size=100 n=1000: 970.4751161722522 (minor: 42 - 28.6563880010508ms, major: 0 - 0ms, incremental: 0 - 0ms)
query/select-many-rows.js size=1000 n=1000: 240.3745889329216 (minor: 297 - 111.55920999590307ms, major: 0 - 0ms, incremental: 0 - 0ms)
query/select-many-rows.js size=10000 n=1000: 28.68140546790126 (minor: 2713 - 854.066908005625ms, major: 3 - 19.69343699933961ms, incremental: 6 - 6.423661998938769ms)
Node.js v16.20.0 arthur/rewrite-parser branch
query/select-many-rows.js size=10 n=10: 851.4735516136872 (minor: 0 - 0ms, major: 0 - 0ms, incremental: 0 - 0ms)
query/select-many-rows.js size=100 n=10: 350.286941050631 (minor: 1 - 2.049265999812633ms, major: 0 - 0ms, incremental: 0 - 0ms)
query/select-many-rows.js size=1000 n=10: 151.6876111358244 (minor: 2 - 4.578842999879271ms, major: 0 - 0ms, incremental: 0 - 0ms)
query/select-many-rows.js size=10000 n=10: 27.913029431453573 (minor: 16 - 8.864512000698596ms, major: 0 - 0ms, incremental: 0 - 0ms)
query/select-many-rows.js size=10 n=100: 1,015.8547807959602 (minor: 1 - 1.1216849996708333ms, major: 0 - 0ms, incremental: 0 - 0ms)
query/select-many-rows.js size=100 n=100: 830.9837475616963 (minor: 5 - 3.184196999296546ms, major: 0 - 0ms, incremental: 0 - 0ms)
query/select-many-rows.js size=1000 n=100: 279.6579015364296 (minor: 18 - 12.758315999992192ms, major: 0 - 0ms, incremental: 0 - 0ms)
query/select-many-rows.js size=10000 n=100: 39.79706595757135 (minor: 156 - 54.824087999761105ms, major: 0 - 0ms, incremental: 0 - 0ms)
query/select-many-rows.js size=10 n=1000: 1,543.7394101309656 (minor: 8 - 12.882265999447554ms, major: 0 - 0ms, incremental: 0 - 0ms)
query/select-many-rows.js size=100 n=1000: 1,115.126761878539 (minor: 27 - 22.832545001525432ms, major: 0 - 0ms, incremental: 0 - 0ms)
query/select-many-rows.js size=1000 n=1000: 329.5490264037709 (minor: 172 - 69.64875099854544ms, major: 0 - 0ms, incremental: 0 - 0ms)
query/select-many-rows.js size=10000 n=1000: 42.634000067428566 (minor: 1555 - 534.4554190100171ms, major: 2 - 10.8917609998025ms, incremental: 4 - 0.6896780002862215ms)

@arthurschreiber
Copy link
Collaborator Author

@mShan0 @MichaelSun90 As discussed, here's the changes I have so far. Would you mind taking this over?

src/value-parser.ts Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 13, 2023

Codecov Report

Merging #1580 (fd4d03a) into master (443701f) will decrease coverage by 2.44%.
The diff coverage is 73.03%.

@@            Coverage Diff             @@
##           master    #1580      +/-   ##
==========================================
- Coverage   80.67%   78.24%   -2.44%     
==========================================
  Files          92       93       +1     
  Lines        4709     4821     +112     
  Branches      871      921      +50     
==========================================
- Hits         3799     3772      -27     
- Misses        633      750     +117     
- Partials      277      299      +22     
Files Coverage Δ
src/token/done-token-parser.ts 100.00% <100.00%> (ø)
src/token/returnstatus-token-parser.ts 100.00% <100.00%> (ø)
src/token/token.ts 92.92% <ø> (ø)
src/token/feature-ext-ack-parser.ts 87.50% <85.71%> (-12.50%) ⬇️
src/token/infoerror-token-parser.ts 86.66% <86.66%> (-13.34%) ⬇️
src/token/loginack-token-parser.ts 86.66% <84.61%> (-13.34%) ⬇️
src/token/order-token-parser.ts 77.77% <75.00%> (-22.23%) ⬇️
src/token/sspi-token-parser.ts 91.30% <66.66%> (-8.70%) ⬇️
src/token/row-token-parser.ts 90.32% <85.71%> (-9.68%) ⬇️
src/token/fedauth-info-parser.ts 84.61% <83.33%> (-5.87%) ⬇️
... and 8 more

... and 4 files with indirect coverage changes

src/value-parser.ts Outdated Show resolved Hide resolved
src/value-parser.ts Outdated Show resolved Hide resolved
src/value-parser.ts Outdated Show resolved Hide resolved
@arthurschreiber arthurschreiber changed the title [WIP] synchronous chunk based token parser feat: improve performance of response data parsing Oct 21, 2023
@arthurschreiber arthurschreiber marked this pull request as ready for review October 21, 2023 11:35
@arthurschreiber arthurschreiber merged commit 78a4530 into master Oct 21, 2023
20 of 26 checks passed
@github-actions
Copy link

🎉 This PR is included in version 16.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants