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

Perf parse params decode text #109

Merged
merged 1 commit into from
Oct 10, 2023
Merged

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Jan 8, 2023

Based on the work of mscdex regarding the decodeText. Pretty smart how he did it.

Checklist

@Uzlopak Uzlopak requested review from Eomm and kibertoad January 8, 2023 02:10
Copy link
Member

@Eomm Eomm left a comment

Choose a reason for hiding this comment

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

Based on the work of mscdex regarding the decodeText. Pretty smart how he did it.

Now sure to understand: did we charry-pick some work from the original busboy repo? - In case I would add credits

@Eomm Eomm mentioned this pull request Jan 8, 2023
4 tasks
@kibertoad
Copy link
Member

@Uzlopak Can you resolve the conflicts?

@kibertoad
Copy link
Member

@Uzlopak what are the benchmarks agains the master?

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Sep 28, 2023

@kibertoad
PTAL

@kibertoad
Copy link
Member

@Uzlopak can you run benchmark to see the perf difference?

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Sep 28, 2023

before:
aras@aras-Lenovo-Legion-5-17ARH05H:~/workspace/busboy$ node bench/parse-params.js
video/ogg x 2,298,591 ops/sec ±0.80% (92 runs sampled)
'text/plain; filename*=utf-8''%c2%a3%20and%20%e2%82%ac%20rates' x 257,143 ops/sec ±0.29% (96 runs sample

after:
aras@aras-Lenovo-Legion-5-17ARH05H:~/workspace/busboy$ node bench/parse-params.js
video/ogg x 2,579,688 ops/sec ±0.31% (90 runs sampled)
'text/plain; filename*=utf-8''%c2%a3%20and%20%e2%82%ac%20rates' x 330,596 ops/sec ±0.26% (97 runs sampled)

package.json Outdated Show resolved Hide resolved
@Uzlopak
Copy link
Contributor Author

Uzlopak commented Sep 28, 2023

@kibertoad
I dont know... tbh photofinish is a little bit complicated. I maybe rewrite the new benchmark in tinybench. Or can you rewrite that benchmark from benchmark to photofinish?

@kibertoad
Copy link
Member

@Uzlopak We can rewrite to tinybench, photofinish main strength is that it allows running easily across Node versions, which is an overkill here.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Oct 6, 2023

@kibertoad
can we just merge? I have no motivation to rewrite the benchmarks. Maybe create an issue to rewrite the benchmarks and slab a 'good-first-issue' label to it.

@Uzlopak Uzlopak merged commit a616201 into master Oct 10, 2023
18 checks passed
@Uzlopak Uzlopak deleted the perf-parseParams-decodeText branch October 10, 2023 09:19
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