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.

Sign up for GitHub

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

Jump to bottom

Optimize parsing for bytes &size=N regressed QUIC performance #1908

Open
awelzel opened this issue Oct 23, 2024 · 2 comments · May be fixed by #1909
Open

Optimize parsing for bytes &size=N regressed QUIC performance #1908

awelzel opened this issue Oct 23, 2024 · 2 comments · May be fixed by #1909
Assignees
@rsmmr
Labels
Regression

Comments

@awelzel
Copy link
Contributor

awelzel commented Oct 23, 2024 •
edited
Loading

As reported in the Zeek repo, a recent bump of Spicy regressed performance of pcap-quic-16-50mb.

zeek/zeek#3953 (comment)

I bisected the range of the bump and it is: ca5de30

$ git bisect log
git bisect start
# status: waiting for both good and bad commits
# good: [64c0000c2a9385020e7f357711c0da3de4b03517] Merge branch 'topic/bbannier/update-update-changes' [skip CI]
git bisect good 64c0000c2a9385020e7f357711c0da3de4b03517
# status: waiting for bad commit, 1 good commit known
# bad: [7347a2be4188a7ec2b3b71cf620fd3e3267c78af] Merge remote-tracking branch 'origin/topic/robin/simplify-ast-tests'
git bisect bad 7347a2be4188a7ec2b3b71cf620fd3e3267c78af
# good: [0af29c57e43f1c1c17f888469d67849ff9200b5b] Add a new unit item `Block` that stores a sequence of subitems.
git bisect good 0af29c57e43f1c1c17f888469d67849ff9200b5b
# bad: [8bb969e1c4c7ca416c27d26fb1caa0c6e4d92dec] Simplify parsing of literals.
git bisect bad 8bb969e1c4c7ca416c27d26fb1caa0c6e4d92dec
# good: [a1f8334e85a9b7c843e2820542b8f82e94b34548] Merge remote-tracking branch 'origin/topic/robin/gh-1839-aggregate-if'
git bisect good a1f8334e85a9b7c843e2820542b8f82e94b34548
# skip: [a0b18dca4a426f972e0a0060a6a1f072b6fd00de] Refactor type parsing modes.
git bisect skip a0b18dca4a426f972e0a0060a6a1f072b6fd00de
# good: [96d6cb47a3c06cae81f23f32f986a9808f042841] Add mode for optimize types parsing.
git bisect good 96d6cb47a3c06cae81f23f32f986a9808f042841
# bad: [ca5de306a7c397c1d37aa7eb662dabafaa8ca26b] Optimize parsing for `bytes &size=N`.
git bisect bad ca5de306a7c397c1d37aa7eb662dabafaa8ca26b
# first bad commit: [ca5de306a7c397c1d37aa7eb662dabafaa8ca26b] Optimize parsing for `bytes &size=N`.

The timings for a hyperfine -r 3 'taskset -c 1 zeek -C -r ./quic-16-50mb-transfers.pcap Log::default_writer=Log::WRITER_NONE' run are as follows locally:

000c2a9385020e7f357711c0da3de4b03517 4.035 4.075

0af29c57e43f1c1c17f888469d67849ff9200b5b 4.078 4.054

8bb969e1c4c7ca416c27d26fb1caa0c6e4d92dec 4.318 4.344

a1f8334e85a9b7c843e2820542b8f82e94b34548 4.045 4.038

a0b18dca4a426f972e0a0060a6a1f072b6fd00de failed to build

96d6cb47a3c06cae81f23f32f986a9808f042841 4.033 4.021

ca5de306a7c397c1d37aa7eb662dabafaa8ca26b 4.269 4.221

7347a2be4188a7ec2b3b71cf620fd3e3267c78af 4.239

PCAP is available on the os-perf-1 host /mnt/data/test_data/quic-16-50mb-transfers.pcap.
@awelzel
Copy link
Contributor Author

awelzel commented Oct 23, 2024 •
edited
Loading

Commit 8bb969e also stands out as slow and wonder a bit about
detail::expectBytesLiteral taking a hilti::rt::Bytes for literal instead of a string_view. Doesn't hilti::rt::Bytes result in a string allocation including potential memory allocation if the literal is large?

@rsmmr
Copy link
Member

rsmmr commented Oct 24, 2024

I'll take a look.

@rsmmr rsmmr self-assigned this Oct 24, 2024
rsmmr added a commit that referenced this issue Oct 25, 2024
@rsmmr
Fix performance regression when parsing bytes.
e7bc029 
Turns out our improved error messages were adding additional overhead
because we were now constructing them through `fmt()` each time we
needed more data, independent of whether there was actually going to
be an error reported.

This adds a second version of `waitForInput()` that doesn't receive an
already prepared error message, but just returns false on error so
that the caller can throw itself.

Closes #1908.
@rsmmr rsmmr linked a pull request Oct 25, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rsmmr rsmmr

Labels
Regression
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix performance regression when parsing bytes. zeek/spicy
2 participants
@rsmmr @awelzel