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

fix: make asString monomorphic and trigger V8 optimization #689

Merged
merged 1 commit into from
Mar 10, 2024
Merged

fix: make asString monomorphic and trigger V8 optimization #689

merged 1 commit into from
Mar 10, 2024

Conversation

nigrosimone
Copy link
Contributor

@nigrosimone nigrosimone commented Mar 10, 2024

asString is Polymorphic/Megamorphic because accept more type (string, date, regex, null)

So let’s spend some more time defining this.

  • Monomorphic: If function arguments are always the same type.
  • Polymorphic: If function arguments are few (four or fewer) different type.
  • Megamorphic: If function arguments are more than four type.

In short: functions that are invoked with the same types over and over again are optimized in the JIT compiler, hence faster.

Megamorphism asString (actual code)

short string............................................. x 14,825,973 ops/sec ±4.07% (156 runs sampled)
short string with double quote........................... x 6,743,956 ops/sec ±4.15% (156 runs sampled)
long string without double quotes........................ x 3,662 ops/sec ±5.18% (161 runs sampled)
long string.............................................. x 5,154 ops/sec ±3.17% (183 runs sampled)

Monomorphic asString (my PR)

short string............................................. x 15,750,724 ops/sec ±2.55% (172 runs sampled)
short string with double quote........................... x 6,067,413 ops/sec ±3.97% (158 runs sampled)
long string without double quotes........................ x 19,700 ops/sec ±3.74% (169 runs sampled)
long string.............................................. x 13,170 ops/sec ±1.65% (180 runs sampled)

Checklist

Copy link
Contributor

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

Definetly needs great consensus
@ivan-tymoshenko
@mcollina
@climba03003
@gurgunday
@kibertoad

I propose the necessity of atleast 3 approvals.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina
Copy link
Member

makes 100% sense

Copy link
Contributor

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

LGTM

@kibertoad
Copy link
Member

Should types be adjusted accordingly?

@Uzlopak
Copy link
Contributor

Uzlopak commented Mar 10, 2024

Which types?

@kibertoad
Copy link
Member

@Uzlopak checked https://github.com/fastify/fast-json-stringify/blob/master/types/index.d.ts, it doesn't seem to be there. so it's an internal only method?

@Uzlopak
Copy link
Contributor

Uzlopak commented Mar 10, 2024

yup, only internal

@Uzlopak Uzlopak merged commit dfb627e into fastify:master Mar 10, 2024
19 checks passed
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