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(remix-server-runtime): use faster alternative to jsesc #3889

Merged
merged 6 commits into from
Aug 12, 2022
Merged

perf(remix-server-runtime): use faster alternative to jsesc #3889

merged 6 commits into from
Aug 12, 2022

Conversation

redabacha
Copy link
Contributor

@redabacha redabacha commented Jul 31, 2022

Closes: #3854

  • Docs
  • Tests

Testing Strategy:

used a faster version of jsesc to serialize the server handoff very much inspired by what next.js has done here. created a benchmark here (which i also included some other libraries in the comparison for additional points of reference).

results from my machine:

cpu: AMD Ryzen 9 5950X 16-Core Processor
runtime: node v16.16.0 (x64-linux)

benchmark                                       time (avg)             (min … max)       p75       p99      p995
---------------------------------------------------------------------------------- -----------------------------
escapeHtml                                   18.69 ms/iter   (17.17 ms … 32.03 ms)  17.95 ms  32.03 ms  32.03 ms
escapeHtml w/ fast-json-stable-stringify      99.6 ms/iter  (86.78 ms … 146.71 ms)  97.41 ms 146.71 ms 146.71 ms
escapeHtml w/ json-stable-stringify         103.18 ms/iter  (75.77 ms … 134.89 ms) 116.19 ms 134.89 ms 134.89 ms
escapeHtml w/ json-stringify-deterministic  105.39 ms/iter  (89.47 ms … 131.08 ms) 115.49 ms 131.08 ms 131.08 ms
escapeHtml w/ superjson.stringify           351.23 ms/iter    (322 ms … 432.56 ms) 362.27 ms 432.56 ms 432.56 ms
jsesc                                       258.64 ms/iter (237.88 ms … 289.99 ms) 263.97 ms 289.99 ms 289.99 ms
js-string-escape                            106.71 ms/iter  (92.13 ms … 132.33 ms) 117.55 ms 132.33 ms 132.33 ms
serialize-javascript                         43.52 ms/iter   (40.56 ms … 73.23 ms)  42.22 ms  73.23 ms  73.23 ms
remix-typedjson serialize                    42.35 ms/iter   (40.97 ms … 50.97 ms)  42.53 ms  50.97 ms  50.97 ms

summary
  escapeHtml
   2.27x faster than remix-typedjson serialize
   2.33x faster than serialize-javascript
   5.33x faster than escapeHtml w/ fast-json-stable-stringify
   5.52x faster than escapeHtml w/ json-stable-stringify
   5.64x faster than escapeHtml w/ json-stringify-deterministic
   5.71x faster than js-string-escape
   13.84x faster than jsesc
   18.79x faster than escapeHtml w/ superjson.stringify

@changeset-bot
Copy link

changeset-bot bot commented Jul 31, 2022

🦋 Changeset detected

Latest commit: bd3add7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages
Name Type
@remix-run/server-runtime Patch
@remix-run/cloudflare Patch
@remix-run/deno Patch
@remix-run/dev Patch
@remix-run/node Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
create-remix Patch
@remix-run/architect Patch
@remix-run/express Patch
@remix-run/netlify Patch
@remix-run/vercel Patch
@remix-run/serve Patch
remix Patch
@remix-run/eslint-config Patch
@remix-run/react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Jul 31, 2022

Hi @redabacha,

Welcome, and thank you for contributing to Remix!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run.

Thanks!

- The Remix team

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Jul 31, 2022

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@MichaelDeBoey MichaelDeBoey changed the title perf(remix-server-runtime): use faster alternative to jsesc perf(remix-server-runtime): use faster alternative to jsesc Aug 4, 2022
@redabacha
Copy link
Contributor Author

the single test failure seems unrelated. i'm also seeing it happen in other prs e.g. https://github.com/remix-run/remix/runs/7671223447?check_suite_focus=true

@jacob-ebey
Copy link
Member

jacob-ebey commented Aug 5, 2022

Going to have to double check, but I believe the serialization method needs to be consistent across machines / runtimes. I.e, serialize(obj) should return the same thing on the server and in the browser. JSON.stringify alone will not suffice here as it does not take into account key order during serialization leading to possible mismatchs on hydration. Going to need something like json-stable-stringify

@redabacha
Copy link
Contributor Author

Going to have to double check, but I believe the serialization method needs to be consistent across machines / runtimes. I.e, serialize(obj) should return the same thing on the server and in the browser. JSON.stringify alone will not suffice here as it does not take into account key order during serialization leading to possible mismatchs on hydration. Going to need something like json-stable-stringify

according to mdn, https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify#description:

Enumerable properties are visited using the same algorithm as Object.keys, which has a well-defined order and is stable across implementations. For example, JSON.stringify on the same object will always produce the same string, and JSON.parse(JSON.stringify(obj)) would produce an object with the same key ordering as the original (assuming the object is completely JSON-serializable).

so my guess is that it would be fine. next.js seem to be getting away with just using JSON.stringify (and JSON.parse on the client).

@redabacha
Copy link
Contributor Author

redabacha commented Aug 6, 2022

@jacob-ebey i've added additional benchmarks with json-stable-stringify and some other stable JSON.stringify packages i've found. according to my understanding though (see prior comment), they shouldn't be necessary in this day and age.

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Thanks! Just one request.

packages/remix-server-runtime/markup.ts Show resolved Hide resolved
Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@jacob-ebey jacob-ebey merged commit c551c33 into remix-run:dev Aug 12, 2022
@MichaelDeBoey MichaelDeBoey added the awaiting release This issue has been fixed and will be released soon label Aug 15, 2022
@MichaelDeBoey MichaelDeBoey removed the awaiting release This issue has been fixed and will be released soon label Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants