chore(util): attempt of performance improve on MIMEType class by metcoder95 · Pull Request #46607 · nodejs/node

metcoder95 · 2023-02-10T13:07:54Z

The PR makes an attempt to improve the performance of the MIMEType class at the moment of parsing an incoming MIME string.
The vast majority of changes were made in an attempt of preserving current implementation meanwhile pushing the limits as far as possible.

As shown in the performance#38 comment, the improvements were about 12 - 20% on average.

Compared against Node v18

                                                                                  confidence improvement accuracy (*)   (**)  (***)
util/mime-parser.js n=100000 strings='application/json; charset="utf-8"'                 ***     12.07 %       ±2.78% ±3.72% ±4.87%
util/mime-parser.js n=100000 strings='text/html ;charset=gbk'                            ***      8.13 %       ±2.18% ±2.91% ±3.79%
util/mime-parser.js n=100000 strings='text/html; charset=gbk'                            ***      4.20 %       ±2.35% ±3.13% ±4.08%
util/mime-parser.js n=100000 strings='text/html;charset= "gbk"'                          ***     10.10 %       ±2.02% ±2.69% ±3.50%
util/mime-parser.js n=100000 strings='text/html;charset=GBK'                             ***     11.53 %       ±2.61% ±3.48% ±4.57%
util/mime-parser.js n=100000 strings='text/html;charset=gbk'                             ***      8.37 %       ±2.23% ±2.96% ±3.86%
util/mime-parser.js n=100000 strings='text/html;charset=gbk;charset=windows-1255'        ***     14.77 %       ±1.66% ±2.21% ±2.88%
util/mime-parser.js n=100000 strings='text/html;x=(;charset=gbk'                         ***     12.14 %       ±3.32% ±4.41% ±5.75%

The improvements were between ~8-12% on average, which is good but still not sure if enough to call it successful. On ops/sec, the results were unprecise as with the previous baseline of:

 * util#MIMEType x 1,349,103 ops/sec ±0.16% (92 runs sampled)
 * undici#parseMIMEType x 2,349,808 ops/sec ±0.14% (99 runs sampled)
 * undici#parseMIMEType(original) x 1,520,777 ops/sec ±0.26% (100 runs sampled)
 * fast-content-type-parse#parse x 5,663,227 ops/sec ±0.58% (98 runs sampled)
 * fast-content-type-parse#safeParse x 5,719,013 ops/sec ±0.44% (95 runs sampled)

the new baseline varied to:

util#MIMEType x 2,075,831 ops/sec ±0.14% (197 runs sampled)
undici#parseMIMEType x 2,729,673 ops/sec ±0.15% (194 runs sampled)
fast-content-type-parse#parse x 5,751,725 ops/sec ±0.13% (195 runs sampled)
fast-content-type-parse#safeParse x 5,766,105 ops/sec ±0.10% (195 runs sampled)
Fastest is fast-content-type-parse#safeParse

Almost ~50%, which seems to be quite out compared to the results of Node benchmarks. I wanted to put them on the table but wouldn't ensure they are precise as they were varying by ~20% on several iterations.

Happy to see any feedback on possible points I missed or further improvements that can be made 🙂

cc: @anonrig

anonrig · 2023-02-22T13:46:56Z

Nit: Ideally, if we don't have quote or backslash character, we don't need to create a result string and append it for each character. I don't know the probability of having this happy path, but if it's common, we might just check on the beginning and return the str

I had in mind using the Regex#matchAll but it requires having the Regex marked as global (making them stateful), which can make usages of Regex tricky. Matter of tradeoffs maybe?

Though, this only affects while using the toString method of the class (template literals, concatenation, etc.). At parsing is not involved at all 🙂

RexExp.prototype.matchAll creates a RegExpStringIterator object, and it has all kinds of prototype pollution vulnerabilities. I don't think it's the right tool for lib/ (and doubt it would be more performant than the alternatives).

It is only performant at the extent of the regex iterating over the string at once and having all the indices where this applies, so the escaping does not need to iterate once again over the string.

But I agree that the security side it is more important here 👍

metcoder95 · 2023-03-03T09:49:09Z

Hi @aduh95, I noticed that RegExpPrototypeTest is marked as dangerous as it makes a lookup to the RegExp.prototype.exec of the chain.

Sadly this comes with regressions in the overall performance (~10%). Is there anything we can do to also speed up the Primordial?
Asking as I noticed that on net#isIPv4 is marked as allowed but subject to improvement once performance gets better

aduh95 · 2023-03-03T09:57:06Z

@metcoder95 what are you comparing it with?

metcoder95 · 2023-03-03T10:07:27Z

RegExpPrototypeExec, which is the closest one.

This is the small benchmark I ran:

const regex = /foo*/;
const regexTest = /foo*/;
const regexExec = /foo*/;
const str = 'table football, foosball';

suite
  .add('RegExp#exec', function () {
    regexExec.exec(str)[0];
  })
  .add('RegExp#test', function () {
    regexTest.test(str);
  })
  .add('String#search', function () {
    str.search(regex);
  })
  .add('String#match', function () {
    str.match(regex);
  })
  .on('cycle', function (event) {
    console.log(String(event.target))
  })
  .on('complete', function () {
    console.log('Fastest is ' + this.filter('fastest').map('name'))
  })
  .run({ async: true })

Results:

RegExp#exec x 37,341,962 ops/sec ±0.24% (96 runs sampled)
RegExp#test x 66,946,944 ops/sec ±0.09% (98 runs sampled)
String#search x 53,180,141 ops/sec ±0.06% (93 runs sampled)
String#match x 36,475,617 ops/sec ±0.18% (99 runs sampled)

The benchmark is pretty rudimentary but it highlights a significative difference.

aduh95 · 2023-03-03T10:17:15Z

You should also compare the primordials to have a more complete benchmark. You can access them if you use --expose-internals flag and import them like this:

const {
  RegExpPrototypeExec,
  RegExpPrototypeTest,
  StringPrototypeSearch,
  StringPrototypeMatch,
} = require('internal/test/binding').primordials;

You should also test adding === null after the exec calls, in case V8 is able to recognize the pattern.

aduh95 · 2023-03-03T10:21:08Z

You should also compare the primordials to have a more complete benchmark. You can access them if you use --expose-internals flag and import them like this:

const {
  RegExpPrototypeExec,
  RegExpPrototypeTest,
  StringPrototypeSearch,
  StringPrototypeMatch,
} = require('internal/test/binding').primordials;

You should also test adding === null after the exec calls, in case V8 is able to recognize the pattern.

aduh95 · 2023-03-03T10:28:02Z

You can also add the case where we use hardenRegExp for completeness sake, although it's unlikely to have better perf results.

metcoder95 · 2023-03-03T12:20:31Z

Sure! I made the changes suggested, and the results didn't vary that much:

RegExp#exec x 37,376,162 ops/sec ±0.25% (92 runs sampled)
RegExp#test x 66,952,599 ops/sec ±0.07% (98 runs sampled)
String#search x 53,146,898 ops/sec ±0.09% (98 runs sampled)
String#match x 35,598,485 ops/sec ±0.26% (98 runs sampled)

const {
    RegExpPrototypeExec,
    RegExpPrototypeTest,
    StringPrototypeSearch,
    StringPrototypeMatch,
  } = require('internal/test/binding').primordials;
const Benchmark = require('benchmark')

const suite = new Benchmark.Suite()

const regex = /foo*/;
const regexTest = /foo*/;
const regexExec = /foo*/;
const str = 'table football, foosball';

suite
  .add('RegExp#exec', function () {
    RegExpPrototypeExec(regexExec, str) === null;
  })
  .add('RegExp#test', function () {
    RegExpPrototypeTest(regexTest, str) === false;
  })
  .add('String#search', function () {
    StringPrototypeSearch(str, regex) === -1;
  })
  .add('String#match', function () {
    StringPrototypeMatch(str, regex) === null;
  })
  .on('cycle', function (event) {
    console.log(String(event.target))
  })
  .on('complete', function () {
    console.log('Fastest is ' + this.filter('fastest').map('name'))
  })
  .run({ async: true })

This is actually nice, it seems that Primordials doesn't add any overhead

aduh95 · 2023-03-03T12:38:10Z

So according to your results, RegExpPrototypeTest looks to be twice as fast as RegExpPrototypeExec and SafeStringPrototypeSearch. It'd be nice if V8 exposed a way to reimplement an alternative to %RegExp.prototype.test% that would be as fast but without doing any prototype lookup, but I don't know how to make that. Unfortunately, until then it seems we have to choose between robustness and speed.

metcoder95 · 2023-03-05T22:31:53Z

Understood, I'll fallback to SafeStringPrototypeSearch instead, I'm expecting a regression of ~10%, but will update the numbers afterward.

Thanks for the instructions @aduh95 🙇
cc: @anonrig

metcoder95 · 2023-03-08T12:39:21Z

I made the changes accordingly and it had a reduction of ~5% which stills marks an improvement.

1,952,986 ops/sec

Though, I'm having a little bit of struggles understanding what's wrong with the testing and why are they timing out, any guidance would be super helpful 😅

anonrig · 2023-03-08T13:04:31Z

any guidance would be super helpful 😅

I reached out from OpenJSF Slack

metcoder95 · 2023-03-08T14:45:01Z

Thanks @anonrig, I actually thing found the culprit. It seems a bad parsing of a given MIME is causing the issue, I'll look through the day 👍

aduh95 · 2023-03-10T10:13:49Z

It's probably worth trying out SafeRegExp like Deno: denoland/deno#17592
Could you try to add that to your benchmark suite?

metcoder95 · 2023-03-10T22:20:39Z

Thanks, let me give it a try and I'll share the results as well 👍

metcoder95 · 2023-03-10T22:24:04Z

I found this line that hardens regex patterns on primordials.
Is it possible to use it instead?

https://github.com/nodejs/node/blob/359172868fa186bf8647fd58fa091d932481cbd4/lib/internal/per_context/primordials.js#L636C17-L722

aduh95 · 2023-03-10T22:27:17Z

Is it possible to use it instead?

Of course you can, but it has terrible performance.

metcoder95 · 2023-03-10T22:31:25Z

Of course you can, but it has terrible performance.

Well, was worth it to ask 🤷‍♂️

aduh95 · 2023-03-10T22:44:30Z

FYI here are the results I've found:

RegExpPrototypeExec x 13,128,237 ops/sec ±6.29% (87 runs sampled)
RegExpPrototypeTest x 7,634,710 ops/sec ±1.71% (94 runs sampled)
RegExp#test x 24,493,324 ops/sec ±0.17% (90 runs sampled)
RegExpPrototypeTest+hardenedRegExp x 5,678,249 ops/sec ±0.29% (94 runs sampled)
SafeRegExp#test x 6,288,065 ops/sec ±0.15% (92 runs sampled)
SafeStringPrototypeSearch x 13,386,956 ops/sec ±1.67% (93 runs sampled)
RegExpPrototypeSymbolSearch x 6,099,046 ops/sec ±0.39% (91 runs sampled)

Code

'use strict';
const {
    RegExpPrototypeExec,
    RegExpPrototypeTest,
    RegExpPrototypeSymbolSearch,
    SafeStringPrototypeSearch,
    hardenRegExp,
    makeSafe,
  } = require('internal/test/binding').primordials;
const Benchmark = require('benchmark')

const SafeRegExp = makeSafe(RegExp,
    class SafeRegExp extends RegExp {}
)

const suite = new Benchmark.Suite();

const regex = /foo*/;
const regexTest = /foo*/;
const regexHardenedTest = hardenRegExp(/foo*/);
const safeRegexTest = new SafeRegExp(regexHardenedTest);
const regexExec = /foo*/;
const str = 'table football, foosball';

suite
  .add('RegExpPrototypeExec', function () {
    RegExpPrototypeExec(regexExec, str) === null;
  })
  .add('RegExpPrototypeTest', function () {
    RegExpPrototypeTest(regexTest, str) === false;
  })
  .add('RegExp#test', function () {
    regexTest.test(str) === false;
  })
  .add('RegExpPrototypeTest+hardenedRegExp', function () {
    RegExpPrototypeTest(regexHardenedTest, str) === false;
  })
  .add('SafeRegExp#test', function () {
    safeRegexTest.test(str) === false;
  })
  .add('SafeStringPrototypeSearch', function () {
    SafeStringPrototypeSearch(str, regex) === -1;
  })
  .add('RegExpPrototypeSymbolSearch', function () {
    RegExpPrototypeSymbolSearch(regex, str) === -1;
  })
  .on('cycle', function (event) {
    console.log(String(event.target))
  })
  .on('complete', function () {
    console.log('Fastest is ' + this.filter('fastest').map('name'))
  })
  .run({ async: true });

AFAICT, SafeStringPrototypeSearch is still the best available option if we don't want prototype pollution.

metcoder95 · 2023-03-13T19:40:07Z

AFAICT, SafeStringPrototypeSearch is still the best option if we don't want prototype pollution.

Agree, the changes I made reflected ~5% regression, but they were worth it compared to the progression made. I just have struggled with some scenarios that need adjustments.

Hope can have something by eow 👍

metcoder95 · 2023-03-22T12:03:00Z

Update on this PR

Sadly haven't had the time to properly focus on this, might have some next week but has to be seen 🙁
On the other hand, I made a step back and tried another approach and that was to be more explicit about the standard steps on how to parse the mime string.
Made a few changes, and adjusted. Performance-wise, it is on the same line as the previous benchmark but by trying to trick bit the standard for performance, I facing some smaller blockers with certain cases.

Solved the timeout issue, but the edge cases are the ones to cover now. Feel free to have a look and I'll incorporate feedback if any. Thanks! 🙂

Uzlopak · 2023-07-30T00:32:50Z

is marker an object, because we want to keep it as byRef parameter?, needs comment

Yeah, that's the main reason. To use as a checkpoint record for the iterations

Uzlopak · 2023-07-30T00:37:08Z

maybe first check if the last character is a whitespace and then call if so trimEnd.

Uzlopak · 2023-07-30T00:38:46Z

switch positions as if trimmedValue.length === 0 means that marker.position += trimmedValue.length is doing nothing.

Suggested change

marker.position += trimmedValue.length;

// #11-9-3 If parameterValue is the empty string, then continue.

if (trimmedValue.length === 0) continue;

// #11-9-3 If parameterValue is the empty string, then continue.

if (trimmedValue.length === 0) continue;

marker.position += trimmedValue.length;

Uzlopak · 2023-07-30T00:41:44Z

isnt .has on Map O(1)?

Suggested change

SafeStringPrototypeSearch(

parameterValue,

NOT_HTTP_QUOTED_STRING_CODE_POINT,

) === -1 &&

params.has(parameterName) === false

params.has(parameterName) === false &&

SafeStringPrototypeSearch(

parameterValue,

NOT_HTTP_QUOTED_STRING_CODE_POINT,

) === -1

Yeah, but I'm afraid I'm not following 🤔

Uzlopak · 2023-07-30T00:43:05Z

Suggested change

string = StringPrototypeTrim(`${string}`);

typeof string !== 'string' && (string = `${string}`)

string = StringPrototypeTrim(string);

%String.prototype.trim% always call ToString per spec, it shouldn't be necessary to to convert it first.

Suggested change

string = StringPrototypeTrim(`${string}`);

string = StringPrototypeTrim(string);

Uzlopak · 2023-07-30T00:43:52Z

Suggested change

this.#essence = `${this.#type}/${this.#subtype}`;

this.#essence = `${type}/${subtype}`;

Uzlopak · 2023-07-30T00:45:55Z

is this maybe better?

if (v.length === 0) { throw new ERR_INVALID_MIME_SYNTAX('type', v, -1); } const invalidTypeIndex = SafeStringPrototypeSearch(v, NOT_HTTP_TOKEN_CODE_POINT); if (invalidTypeIndex !== -1) { throw new ERR_INVALID_MIME_SYNTAX('type', v, invalidTypeIndex); }

We can even merge the SafeStringPrototypeSearch within the if statement

If you want to pass the invalidTypeIndex to the error, then you have to do it outside the if

Uzlopak

added some thoughts. maybe you can press some op/s out of my suggestions. looking for feedback

Uzlopak · 2023-07-30T02:35:35Z

Btw. If you use benchmarkjs for benching and use async true (for what reason ever) then you could set delay to 0.

metcoder95 · 2023-07-30T11:46:07Z

added some thoughts. maybe you can press some op/s out of my suggestions. looking for feedback

Thanks for the feedback @Uzlopak, they are a fresh look over the PR about several optimisations a missed 👌
Has been a while since I visited the PR, so let me take the baseline performance and implement some optimizations based on your feedback to compare results

Uzlopak · 2023-07-30T11:54:20Z

This PR got me actually the motivation to clone the node repo and try to get it built and run some benchmarks

metcoder95 · 2023-12-13T10:24:33Z

I'm re-taking this PR. I'm giving it a try with the recommendations from @Uzlopak and doing some experiments with direct buffers to see if we can speed this ahead since we start parsing the MIMEType string.

I'd hope to have something by end of week 🙂

metcoder95 · 2025-05-15T18:56:04Z

Will attempt to re-open another PR soon.

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Feb 10, 2023

metcoder95 mentioned this pull request Feb 10, 2023

MIMEType Performance nodejs/performance#38

Open

aduh95 reviewed Feb 10, 2023

View reviewed changes

Comment thread lib/internal/mime.js Outdated

metcoder95 changed the title ~~chore: attempt of performance improve on MIMEType class~~ chore(util): attempt of performance improve on MIMEType class Feb 15, 2023

metcoder95 force-pushed the perf/mime_type branch 2 times, most recently from fe82488 to c6e4132 Compare February 20, 2023 16:23

metcoder95 marked this pull request as ready for review February 20, 2023 16:30

anonrig reviewed Feb 20, 2023

View reviewed changes

Comment thread benchmark/util/mime-parser.js Outdated

Comment thread lib/internal/mime.js Outdated

Comment thread lib/internal/mime.js Outdated

Comment thread lib/internal/mime.js Outdated

Comment thread lib/internal/mime.js Outdated

Comment thread lib/internal/mime.js Outdated

metcoder95 mentioned this pull request Feb 21, 2023

doc: add meeting notes 2023-02-20 (#58) nodejs/performance#61

Merged

anonrig reviewed Feb 22, 2023

View reviewed changes

Uzlopak reviewed Jul 30, 2023

View reviewed changes

metcoder95 added 13 commits August 7, 2023 09:54

chore(util): first attempt of performance improve on MIMEType class

01161bc

refactor: small set of adjustments

26564da

refactor: cleanup implementation

c34812c

fix: smaller adjustments

a7c03ad

refactor: leftovers and linting

1b30086

refactor: use charcode instead of string check

c7eb5e3

refactor: remove else branching

c6896c3

fix: bad encoding

e94628e

fix: linting issues

8d8a023

refactor: revert to string#search

3951cf5

refactor: take explicit steps from standard

e2eca5b

fix: bad parsing of several inputs

aca6158

fix: linter issues

3394c74

metcoder95 force-pushed the perf/mime_type branch from 583249d to 3394c74 Compare August 7, 2023 07:54

metcoder95 added 2 commits November 24, 2023 12:21

Merge remote-tracking branch 'upstream/main' into perf/mime_type

dbc8c4d

refactor: preserve commit

46d27ae

metcoder95 closed this May 15, 2025

-        marker.position += trimmedValue.length;
-        // #11-9-3 If parameterValue is the empty string, then continue.
-        if (trimmedValue.length === 0) continue;
+        // #11-9-3 If parameterValue is the empty string, then continue.
+        if (trimmedValue.length === 0) continue;
+        marker.position += trimmedValue.length;

-        SafeStringPrototypeSearch(
-          parameterValue,
-          NOT_HTTP_QUOTED_STRING_CODE_POINT,
-        ) === -1 &&
-        params.has(parameterName) === false
+        params.has(parameterName) === false &&
+        SafeStringPrototypeSearch(
+          parameterValue,
+          NOT_HTTP_QUOTED_STRING_CODE_POINT,
+        ) === -1

	string = StringPrototypeTrim(`${string}`);
	typeof string !== 'string' && (string = `${string}`)
	string = StringPrototypeTrim(string);

	this.#essence = `${this.#type}/${this.#subtype}`;
	this.#essence = `${type}/${subtype}`;

Conversation

metcoder95 commented Feb 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

metcoder95 commented Mar 3, 2023

Uh oh!

aduh95 commented Mar 3, 2023

Uh oh!

metcoder95 commented Mar 3, 2023

Uh oh!

aduh95 commented Mar 3, 2023

Uh oh!

aduh95 commented Mar 3, 2023

Uh oh!

aduh95 commented Mar 3, 2023

Uh oh!

metcoder95 commented Mar 3, 2023

Uh oh!

aduh95 commented Mar 3, 2023

Uh oh!

metcoder95 commented Mar 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

metcoder95 commented Mar 8, 2023

Uh oh!

anonrig commented Mar 8, 2023

Uh oh!

metcoder95 commented Mar 8, 2023

Uh oh!

aduh95 commented Mar 10, 2023

Uh oh!

metcoder95 commented Mar 10, 2023

Uh oh!

metcoder95 commented Mar 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduh95 commented Mar 10, 2023

Uh oh!

metcoder95 commented Mar 10, 2023

Uh oh!

aduh95 commented Mar 10, 2023

Uh oh!

metcoder95 commented Mar 13, 2023

Uh oh!

metcoder95 commented Mar 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uzlopak Jul 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

metcoder95 commented Feb 10, 2023 •

edited

Loading

metcoder95 commented Mar 5, 2023 •

edited

Loading

metcoder95 commented Mar 10, 2023 •

edited

Loading

Uzlopak Jul 30, 2023 •

edited

Loading