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

optimizing mimetype serialization #1054

Merged
merged 5 commits into from
Aug 30, 2023
Merged

Conversation

mikea
Copy link
Collaborator

@mikea mikea commented Aug 23, 2023

kj::strTree results in plenty of heap allocation. Using capacity-bound string buffer to do only one allocation per serialize

before:

----------------------------------------------------------------------
Benchmark                            Time             CPU   Iterations
----------------------------------------------------------------------
Mimetype::ParseAndSerialize       2.18 us         2.18 us       324082
Mimetype::Serialize              0.351 us        0.351 us      1982953

after:

----------------------------------------------------------------------
Benchmark                            Time             CPU   Iterations
----------------------------------------------------------------------
Mimetype::ParseAndSerialize       1.48 us         1.48 us       456517
Mimetype::Serialize              0.114 us        0.114 us      6175365

@mikea mikea requested review from jasnell and fhanau August 23, 2023 20:50
src/workerd/util/mimetype.c++ Outdated Show resolved Hide resolved
@mikea mikea force-pushed the maizatskyi/2023-08-23-opt-mimetype branch 3 times, most recently from 64252d3 to b8d5218 Compare August 23, 2023 21:01
@mikea mikea force-pushed the maizatskyi/2023-08-23-opt-mimetype branch from b8d5218 to 572f2fd Compare August 23, 2023 21:26
@mikea mikea force-pushed the maizatskyi/2023-08-23-opt-mimetype branch from 572f2fd to 51f1c9c Compare August 24, 2023 19:56
@mikea
Copy link
Collaborator Author

mikea commented Aug 24, 2023

updated string buffer to be unbounded and to grow on the heap. ptal @kentonv @jasnell

@mikea mikea requested a review from ohodson August 24, 2023 19:58
@irvinebroque
Copy link
Collaborator

Does new benchmarking tooling we added gives us a way to fail CI if a given benchmark regresses more than a given percentage? (like if something shot up 400%)

@mikea
Copy link
Collaborator Author

mikea commented Aug 24, 2023

Does new benchmarking tooling we added gives us a way to fail CI if a given benchmark regresses more than a given percentage? (like if something shot up 400%)

eventually this will be possible. Right now it is just an instrument to measure a single change impact. I plan to start collecting these on TC for a longer term trend

Copy link
Contributor

@ohodson ohodson left a comment

Choose a reason for hiding this comment

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

nice, lgtm with James suggestions.

Comment on lines +44 to +47
char arr[StackSize];

// on the heap chunks
kj::Vector<kj::Array<char>> chunks;
Copy link
Member

Choose a reason for hiding this comment

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

Non-blocking... and just thinking out loud.... In Node.js we have a utility type called MaybeStackBuffer<T> that encapsulates this Maybe-Stack-Maybe-Heap pattern. It's a lot like KJ_STACK_ARRAY but in a form that is easy to define and use as class members like this. It might be worthwhile to adopt a version of that for our use.

@mikea mikea merged commit e5597b6 into main Aug 30, 2023
6 checks passed
@mikea mikea deleted the maizatskyi/2023-08-23-opt-mimetype branch August 30, 2023 22:28
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.

5 participants