Skip to content

util: lazy parse mime parameters - #49889

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
Uzlopak:lazy-parse-mime-parameters
Oct 8, 2023
Merged

util: lazy parse mime parameters#49889
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
Uzlopak:lazy-parse-mime-parameters

Conversation

@Uzlopak

@UzlopakUzlopak commented Sep 27, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

This PR improves the instantiation speed of MimeType by parsing the MimeParams lazily.

MimeParams accepts now a string of the parameter rest.

 confidence improvement accuracy (*) (**) (***)
mime/mimetype-instantiation.js value='application/ecmascript; ' n=100000 *** 43.18 % ±3.42% ±4.58% ±6.02%
mime/mimetype-instantiation.js value='text/html;012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789=x;charset=gbk' n=100000 *** 656.71 % ±21.51% ±28.99% ±38.48%
mime/mimetype-instantiation.js value='text/html;charset=gbk' n=100000 *** 151.06 % ±4.03% ±5.40% ±7.13%
mime/mimetype-instantiation.js value='text/html;test=ÿ;charset=gbk' n=100000 *** 241.04 % ±4.72% ±6.33% ±8.36%
mime/mimetype-instantiation.js value='x/x;\n\r\t x=x\n\r\t ;x=y' n=100000 *** 267.60 % ±8.31% ±11.20% ±14.85%
mime/mimetype-to-string.js value='application/ecmascript; ' n=100000 * -5.74 % ±5.34% ±7.15% ±9.41%
mime/mimetype-to-string.js value='text/html;012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789=x;charset=gbk' n=100000 * -3.68 % ±2.79% ±3.72% ±4.84%
mime/mimetype-to-string.js value='text/html;charset=gbk' n=100000 *** -5.62 % ±2.67% ±3.55% ±4.62%
mime/mimetype-to-string.js value='text/html;test=ÿ;charset=gbk' n=100000 -2.71 % ±3.55% ±4.75% ±6.24%
mime/mimetype-to-string.js value='x/x;\n\r\t x=x\n\r\t ;x=y' n=100000 -1.44 % ±5.78% ±7.74% ±10.18%
Be aware that when doing many comparisons the risk of a false-positive result increases.
In this case, there are 10 comparisons, you can thus expect the following amount of false-positive results:
0.50 false positives, when considering a 5% risk acceptance (*, **, ***),
0.10 false positives, when considering a 1% risk acceptance (**, ***),
0.01 false positives, when considering a 0.1% risk acceptance (***)

@nodejs-github-botnodejs-github-bot added the needs-ci PRs that need a full CI run. label Sep 27, 2023
This was referenced Sep 27, 2023
Closed
Open
Uzlopak
Uzlopak commented Sep 27, 2023
View reviewed changes
Comment threadlib/internal/mime.js Outdated
@Uzlopak

Uzlopak commented Sep 27, 2023

Copy link
Copy Markdown
ContributorAuthor

@metcoder95
PTAL

climba03003
climba03003 reviewed Sep 27, 2023
View reviewed changes
Comment threadlib/internal/mime.js Outdated
metcoder95
metcoder95 approved these changes Sep 27, 2023
View reviewed changes

@metcoder95metcoder95 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM 👍

I'm just curious about the results

Comment threadlib/internal/mime.js Outdated
Comment threadlib/internal/mime.js Outdated
@Uzlopak
Uzlopakforce-pushed the lazy-parse-mime-parameters branch from cae6ad7 to b00e9bcCompareSeptember 27, 2023 09:38
aduh95
aduh95 reviewed Sep 27, 2023
View reviewed changes
Comment threadlib/internal/mime.js Outdated
aduh95
aduh95 reviewed Sep 27, 2023
View reviewed changes
Comment threadlib/internal/mime.js Outdated
aduh95
aduh95 reviewed Sep 27, 2023
View reviewed changes
Comment threadtest/parallel/test-mime-api.js Outdated
Uzlopak
Uzlopak commented Sep 27, 2023
View reviewed changes
Comment threadlib/internal/mime.js Outdated
Uzlopak
Uzlopak commented Sep 27, 2023
View reviewed changes
Comment threadlib/internal/mime.js Outdated
Uzlopak
Uzlopak commented Sep 27, 2023
View reviewed changes
Comment threadlib/internal/mime.js Outdated
Uzlopak
Uzlopak commented Sep 27, 2023
View reviewed changes
Comment threadlib/internal/mime.js Outdated
aduh95
aduh95 reviewed Sep 27, 2023
View reviewed changes
Comment threadlib/internal/mime.js Outdated
aduh95
aduh95 reviewed Sep 27, 2023
View reviewed changes
Comment threadlib/internal/mime.js Outdated
@Uzlopak

Uzlopak commented Sep 27, 2023

Copy link
Copy Markdown
ContributorAuthor

@aduh95

Thanks ;)

aduh95
aduh95 reviewed Sep 27, 2023
View reviewed changes
Comment threadbenchmark/mime/mimetype-instantiation.js Outdated
aduh95
aduh95 reviewed Sep 27, 2023
View reviewed changes
Comment threadbenchmark/mime/mimetype-to-string.js Outdated
aduh95
aduh95 reviewed Sep 27, 2023
View reviewed changes
Comment threadbenchmark/mime/parse-type-and-subtype.js Outdated
aduh95
aduh95 reviewed Sep 27, 2023
View reviewed changes
Comment threadtest/parallel/test-mime-api.js Outdated
@aduh95

aduh95 commented Sep 27, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1422/

Results
confidence
mime/mimetype-instantiation.js value='application/ecmascript; ' n=10000000 ***
mime/mimetype-instantiation.js value='text/html;0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789=x;charset=gbk' n=10000000 ***
mime/mimetype-instantiation.js value='text/html;charset=gbk' n=10000000 ***
mime/mimetype-instantiation.js value='text/html;test=ÿ;charset=gbk' n=10000000 ***
mime/mimetype-instantiation.js value='x/x;\\n\\r\\t x=x\\n\\r\\t ;x=y' n=10000000 ***
mime/mimetype-to-string.js value='application/ecmascript; ' n=10000000 **
mime/mimetype-to-string.js value='text/html;0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789=x;charset=gbk' n=10000000 ***
mime/mimetype-to-string.js value='text/html;charset=gbk' n=10000000 ***
mime/mimetype-to-string.js value='text/html;test=ÿ;charset=gbk' n=10000000 ***
mime/mimetype-to-string.js value='x/x;\\n\\r\\t x=x\\n\\r\\t ;x=y' n=10000000 ***
mime/parse-type-and-subtype.js value='application/ecmascript; ' n=10000000 ***
mime/parse-type-and-subtype.js value='text/html;0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789=x;charset=gbk' n=10000000 ***
mime/parse-type-and-subtype.js value='text/html;charset=gbk' n=10000000 ***
mime/to-ascii-lower.js value='ABCDEFGHIJKLMNOPQRSTUVWXYZ' n=10000000 ***
mime/to-ascii-lower.js value='lowercase' n=10000000 ***
mime/to-ascii-lower.js value='mixedCase' n=10000000 ***
mime/to-ascii-lower.js value='UPPERCASE' n=10000000 ***
improvement
mime/mimetype-instantiation.js value='application/ecmascript; ' n=10000000 57.38 %
mime/mimetype-instantiation.js value='text/html;0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789=x;charset=gbk' n=10000000 1052.84 %
mime/mimetype-instantiation.js value='text/html;charset=gbk' n=10000000 214.77 %
mime/mimetype-instantiation.js value='text/html;test=ÿ;charset=gbk' n=10000000 321.37 %
mime/mimetype-instantiation.js value='x/x;\\n\\r\\t x=x\\n\\r\\t ;x=y' n=10000000 407.48 %
mime/mimetype-to-string.js value='application/ecmascript; ' n=10000000 -6.14 %
mime/mimetype-to-string.js value='text/html;0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789=x;charset=gbk' n=10000000 -5.12 %
mime/mimetype-to-string.js value='text/html;charset=gbk' n=10000000 -4.61 %
mime/mimetype-to-string.js value='text/html;test=ÿ;charset=gbk' n=10000000 -1.47 %
mime/mimetype-to-string.js value='x/x;\\n\\r\\t x=x\\n\\r\\t ;x=y' n=10000000 -4.51 %
mime/parse-type-and-subtype.js value='application/ecmascript; ' n=10000000 4815.70 %
mime/parse-type-and-subtype.js value='text/html;0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789=x;charset=gbk' n=10000000 7722.49 %
mime/parse-type-and-subtype.js value='text/html;charset=gbk' n=10000000 7712.62 %
mime/to-ascii-lower.js value='ABCDEFGHIJKLMNOPQRSTUVWXYZ' n=10000000 5200.23 %
mime/to-ascii-lower.js value='lowercase' n=10000000 23676.12 %
mime/to-ascii-lower.js value='mixedCase' n=10000000 22185.79 %
mime/to-ascii-lower.js value='UPPERCASE' n=10000000 16620.19 %
accuracy (*)
mime/mimetype-instantiation.js value='application/ecmascript; ' n=10000000 ±0.92%
mime/mimetype-instantiation.js value='text/html;0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789=x;charset=gbk' n=10000000 ±4.68%
mime/mimetype-instantiation.js value='text/html;charset=gbk' n=10000000 ±1.44%
mime/mimetype-instantiation.js value='text/html;test=ÿ;charset=gbk' n=10000000 ±1.73%
mime/mimetype-instantiation.js value='x/x;\\n\\r\\t x=x\\n\\r\\t ;x=y' n=10000000 ±2.79%
mime/mimetype-to-string.js value='application/ecmascript; ' n=10000000 ±3.89%
mime/mimetype-to-string.js value='text/html;0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789=x;charset=gbk' n=10000000 ±0.92%
mime/mimetype-to-string.js value='text/html;charset=gbk' n=10000000 ±1.37%
mime/mimetype-to-string.js value='text/html;test=ÿ;charset=gbk' n=10000000 ±0.63%
mime/mimetype-to-string.js value='x/x;\\n\\r\\t x=x\\n\\r\\t ;x=y' n=10000000 ±0.84%
mime/parse-type-and-subtype.js value='application/ecmascript; ' n=10000000 ±23.75%
mime/parse-type-and-subtype.js value='text/html;0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789=x;charset=gbk' n=10000000 ±31.98%
mime/parse-type-and-subtype.js value='text/html;charset=gbk' n=10000000 ±33.80%
mime/to-ascii-lower.js value='ABCDEFGHIJKLMNOPQRSTUVWXYZ' n=10000000 ±30.57%
mime/to-ascii-lower.js value='lowercase' n=10000000 ±145.98%
mime/to-ascii-lower.js value='mixedCase' n=10000000 ±195.91%
mime/to-ascii-lower.js value='UPPERCASE' n=10000000 ±100.02%
(**)
mime/mimetype-instantiation.js value='application/ecmascript; ' n=10000000 ±1.23%
mime/mimetype-instantiation.js value='text/html;0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789=x;charset=gbk' n=10000000 ±6.31%
mime/mimetype-instantiation.js value='text/html;charset=gbk' n=10000000 ±1.92%
mime/mimetype-instantiation.js value='text/html;test=ÿ;charset=gbk' n=10000000 ±2.32%
mime/mimetype-instantiation.js value='x/x;\\n\\r\\t x=x\\n\\r\\t ;x=y' n=10000000 ±3.75%
mime/mimetype-to-string.js value='application/ecmascript; ' n=10000000 ±5.19%
mime/mimetype-to-string.js value='text/html;0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789=x;charset=gbk' n=10000000 ±1.22%
mime/mimetype-to-string.js value='text/html;charset=gbk' n=10000000 ±1.83%
mime/mimetype-to-string.js value='text/html;test=ÿ;charset=gbk' n=10000000 ±0.83%
mime/mimetype-to-string.js value='x/x;\\n\\r\\t x=x\\n\\r\\t ;x=y' n=10000000 ±1.12%
mime/parse-type-and-subtype.js value='application/ecmascript; ' n=10000000 ±32.01%
mime/parse-type-and-subtype.js value='text/html;0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789=x;charset=gbk' n=10000000 ±43.10%
mime/parse-type-and-subtype.js value='text/html;charset=gbk' n=10000000 ±45.56%
mime/to-ascii-lower.js value='ABCDEFGHIJKLMNOPQRSTUVWXYZ' n=10000000 ±41.20%
mime/to-ascii-lower.js value='lowercase' n=10000000 ±196.74%
mime/to-ascii-lower.js value='mixedCase' n=10000000 ±264.03%
mime/to-ascii-lower.js value='UPPERCASE' n=10000000 ±134.79%
(***)
mime/mimetype-instantiation.js value='application/ecmascript; ' n=10000000 ±1.61%
mime/mimetype-instantiation.js value='text/html;0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789=x;charset=gbk' n=10000000 ±8.38%
mime/mimetype-instantiation.js value='text/html;charset=gbk' n=10000000 ±2.51%
mime/mimetype-instantiation.js value='text/html;test=ÿ;charset=gbk' n=10000000 ±3.04%
mime/mimetype-instantiation.js value='x/x;\\n\\r\\t x=x\\n\\r\\t ;x=y' n=10000000 ±4.96%
mime/mimetype-to-string.js value='application/ecmascript; ' n=10000000 ±6.77%
mime/mimetype-to-string.js value='text/html;0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789=x;charset=gbk' n=10000000 ±1.59%
mime/mimetype-to-string.js value='text/html;charset=gbk' n=10000000 ±2.38%
mime/mimetype-to-string.js value='text/html;test=ÿ;charset=gbk' n=10000000 ±1.08%
mime/mimetype-to-string.js value='x/x;\\n\\r\\t x=x\\n\\r\\t ;x=y' n=10000000 ±1.45%
mime/parse-type-and-subtype.js value='application/ecmascript; ' n=10000000 ±42.49%
mime/parse-type-and-subtype.js value='text/html;0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789=x;charset=gbk' n=10000000 ±57.22%
mime/parse-type-and-subtype.js value='text/html;charset=gbk' n=10000000 ±60.48%
mime/to-ascii-lower.js value='ABCDEFGHIJKLMNOPQRSTUVWXYZ' n=10000000 ±54.69%
mime/to-ascii-lower.js value='lowercase' n=10000000 ±261.19%
mime/to-ascii-lower.js value='mixedCase' n=10000000 ±350.52%
mime/to-ascii-lower.js value='UPPERCASE' n=10000000 ±178.95%
Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case, there are 17 comparisons, you can thus
expect the following amount of false-positive results:
0.85 false positives, when considering a 5% risk acceptance (*, **, ***),
0.17 false positives, when considering a 1% risk acceptance (**, ***),
0.02 false positives, when considering a 0.1% risk acceptance (***)

@anonriganonrig added the performance Issues and PRs related to the performance of Node.js. label Sep 27, 2023
@anonriganonrig added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Sep 27, 2023
@H4ad

H4ad commented Sep 28, 2023

Copy link
Copy Markdown
Member

@Uzlopak Can you reduce the number of iterations on the benchmarks? The current benchmark ci is taking many hours.

@anonriganonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 2, 2023
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 2, 2023
@aduh95aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 5, 2023
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 5, 2023
@nodejs-github-bot

nodejs-github-bot commented Oct 5, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/54553/

@aduh95

aduh95 commented Oct 6, 2023

Copy link
Copy Markdown
Contributor

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1438/

@Uzlopak

Uzlopak commented Oct 6, 2023

Copy link
Copy Markdown
ContributorAuthor

@aduh95
Added a remark to your comment.

@Uzlopak@aduh95
Update test/parallel/test-mime-api.js
2da4719
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@Uzlopak

Uzlopak commented Oct 7, 2023

Copy link
Copy Markdown
ContributorAuthor

lets apply your remark. I go to sleep now

@aduh95

aduh95 commented Oct 7, 2023

Copy link
Copy Markdown
Contributor

The benchmark has taken 16 hours to complete, which is an improvement over the 17 hours it took the first time, but a relatively small one if you ask me. I wonder what's taking so long, it would be beneficial to lower that time further.

@Uzlopak

Uzlopak commented Oct 7, 2023

Copy link
Copy Markdown
ContributorAuthor

@aduh95
I guess it is because I also benchmark for toASCIILower. Because in main toAsciiLower is not exported, it throws errors, which are collected. So that is super slow.

@aduh95

aduh95 commented Oct 7, 2023

Copy link
Copy Markdown
Contributor

Ah yeah, the to-ascii-lower benchmark ran for 9 hours, that's probably it indeed.

@aduh95aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 8, 2023
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 8, 2023
@nodejs-github-bot

nodejs-github-bot commented Oct 8, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/54611/

@nodejs-github-bot

nodejs-github-bot commented Oct 8, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/54617/

@aduh95aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 8, 2023
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 8, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 54bb691 into nodejs:mainOct 8, 2023
@nodejs-github-bot

nodejs-github-bot commented Oct 8, 2023

Copy link
Copy Markdown
Collaborator

Landed in 54bb691

@Uzlopak
Uzlopak deleted the lazy-parse-mime-parameters branch October 8, 2023 15:43
@github-actionsgithub-actionsBot mentioned this pull request Oct 9, 2023
Open
23 tasks
@RafaelGSSRafaelGSS mentioned this pull request Oct 9, 2023
Merged
This was referenced Oct 10, 2023
Open
Open
targos pushed a commit that referenced this pull request Nov 11, 2023
@Uzlopak@targos
util: lazy parse mime parameters
ac02cdb
PR-URL: #49889
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@targostargos mentioned this pull request Nov 12, 2023
Merged
@juanarboljuanarbol mentioned this pull request Dec 6, 2023
Merged
debadree25 pushed a commit to debadree25/node that referenced this pull request Apr 15, 2024
@Uzlopak@debadree25
util: lazy parse mime parameters
063f689
PR-URL: nodejs#49889
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anonriganonriganonrig approved these changes
@aduh95aduh95aduh95 approved these changes
+2 more reviewers
@climba03003climba03003climba03003 left review comments
@metcoder95metcoder95metcoder95 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.needs-ciPRs that need a full CI run.performanceIssues and PRs related to the performance of Node.js.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@Uzlopak@aduh95@H4ad@nodejs-github-bot@anonrig@metcoder95@climba03003