refactor: use async/await instead then() in promises #379

multivoltage · 2023-07-09T15:40:29Z

Checklist

run npm run test and npm run benchmark
tests and/or benchmarks are included
documentation is changed or added
commit message and code follows the Developer's Certification of Origin
and the Code of conduct

Why this PR?

I added support for html-minifier-terser in a separate PR. To to that I wrote some "await" and "async". Matteo Collina advised that it's better to have all "await" or all callback for promises.
So I tried to remove .then() and .catch() in favour of await asynk

mcollina · 2023-07-10T06:24:21Z

index.js

-          this.send(err)
-        })
+      try {
+        const res = engine.renderAsync(page, data, config)


this should be awaited

mcollina · 2023-07-10T06:24:56Z

index.js

@@ -386,7 +394,7 @@ function fastifyView (fastify, opts, next) {
    // append view extension
    page = getPage(page, type)
    const requestedPath = getRequestedPath(this)
-    getTemplate(page, (err, template) => {
+    getTemplate(page, async (err, template) => {


getTemplate should be ported to be an async function

To change "getTemplate" I have two options:

return a Promise and convert callback(err,data) with resolve and reject. (readFile remain readFile)

return an async function and use await with await readFileAsync. For me is better, but reading on web, even if this seems faster than readFile it should make the code blocking in case of many users.

Based on this what do you suggest?

You should be using https://nodejs.org/api/fs.html#fspromisesreadfilepath-options.

(Available in all supported Node.js versions as require('fs').promises.readFile

Happy to use it <3

mcollina · 2023-07-10T06:25:51Z

index.js

@@ -260,7 +265,7 @@ function fastifyView (fastify, opts, next) {
  }

  function readCallback (that, page, data, localOptions) {


This whole logic would have to change because we are not using callbacks anymore, we are using async/await.

Eomm · 2023-07-10T14:36:12Z

index.js

@@ -12,7 +12,7 @@ const { basename, dirname, extname } = require('path')
 const HLRU = require('hashlru')
 const supportedEngines = ['ejs', 'nunjucks', 'pug', 'handlebars', 'mustache', 'art-template', 'twig', 'liquid', 'dot', 'eta']

-function fastifyView (fastify, opts, next) {
+async function fastifyView (fastify, opts, next) {


Suggested change

async function fastifyView (fastify, opts, next) {

async function fastifyView (fastify, opts) {

multivoltage · 2023-07-10T14:40:16Z

I tried to edit all calback

After that I tried npm run exaple && autocannon -c 100 -d 5 -p 10 localhost:3000

In master

┌─────────┬───────┬────────┬────────┬────────┬───────────┬─────────┬────────┐
│ Stat    │ 2.5%  │ 50%    │ 97.5%  │ 99%    │ Avg       │ Stdev   │ Max    │
├─────────┼───────┼────────┼────────┼────────┼───────────┼─────────┼────────┤
│ Latency │ 98 ms │ 101 ms │ 161 ms │ 174 ms │ 107.03 ms │ 16.1 ms │ 198 ms │
└─────────┴───────┴────────┴────────┴────────┴───────────┴─────────┴────────┘
┌───────────┬─────────┬─────────┬─────────┬────────┬─────────┬────────┬─────────┐
│ Stat      │ 1%      │ 2.5%    │ 50%     │ 97.5%  │ Avg     │ Stdev  │ Min     │
├───────────┼─────────┼─────────┼─────────┼────────┼─────────┼────────┼─────────┤
│ Req/Sec   │ 7703    │ 7703    │ 9543    │ 9687   │ 9190    │ 747.26 │ 7703    │
├───────────┼─────────┼─────────┼─────────┼────────┼─────────┼────────┼─────────┤
│ Bytes/Sec │ 2.07 MB │ 2.07 MB │ 2.56 MB │ 2.6 MB │ 2.46 MB │ 200 kB │ 2.06 MB │
└───────────┴─────────┴─────────┴─────────┴────────┴─────────┴────────┴─────────┘

Req/Bytes counts sampled once per second.
# of samples: 5

47k requests in 5.03s, 12.3 MB read

In my commit:

┌─────────┬────────┬────────┬────────┬────────┬──────────┬─────────┬────────┐
│ Stat    │ 2.5%   │ 50%    │ 97.5%  │ 99%    │ Avg      │ Stdev   │ Max    │
├─────────┼────────┼────────┼────────┼────────┼──────────┼─────────┼────────┤
│ Latency │ 140 ms │ 146 ms │ 165 ms │ 176 ms │ 147.6 ms │ 7.62 ms │ 210 ms │
└─────────┴────────┴────────┴────────┴────────┴──────────┴─────────┴────────┘
┌───────────┬─────────┬─────────┬─────────┬─────────┬─────────┬─────────┬─────────┐
│ Stat      │ 1%      │ 2.5%    │ 50%     │ 97.5%   │ Avg     │ Stdev   │ Min     │
├───────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
│ Req/Sec   │ 6003    │ 6003    │ 6783    │ 6995    │ 6632.4  │ 363.27  │ 6000    │
├───────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
│ Bytes/Sec │ 1.61 MB │ 1.61 MB │ 1.82 MB │ 1.87 MB │ 1.78 MB │ 97.5 kB │ 1.61 MB │
└───────────┴─────────┴─────────┴─────────┴─────────┴─────────┴─────────┴─────────┘

Req/Bytes counts sampled once per second.
# of samples: 5

34k requests in 5.03s, 8.89 MB read

i think the performance decreased, maybe using async await is not the best way or maybe i made some mistakes

mcollina · 2023-07-10T15:42:45Z

index.js

-      promise.then(done.bind(null, null), done)
+      try {
+        const result = await promise
+        done(null, result)


Suggested change

done(null, result)

process.nextTick(done, null, result)

mcollina · 2023-07-10T15:43:43Z

index.js

+        const result = await promise
+        done(null, result)
+      } catch (err) {
+        done(err)


Suggested change

done(err)

process.nextTick(done, err)

You need to nextTick every callback to avoid errors in that chain to be matched by the try-catch

mcollina · 2023-07-10T15:44:29Z

index.js

-          }
-        })
-      })
+      for (const key of Object.keys(partials)) {


You should use a Promise.all here

Sorry. I' m feel like junior in all this. I never used node js with perfomance in mind. Probably is not my level.

I trying to understand why use promise all. Do you think performance decrease come from this piece of code? Putting nextTick in all points did not increase perfomance in my local project

Adding nextTick is about error handling, not performance. If you don't, the callback with be called twice in case of an error.

The original code did the readFile in parallel, so you should do the same here. with Promise.all.

I tried to follow all suggestions. Replaced for of with Promise.all().
Maybe in a wrong way. Perfomances with autocannon did not increse.

mcollina · 2023-07-10T15:45:27Z

index.js

      Object.keys(partialsObject).forEach((name) => {
        engine.registerPartial(name, engine.compile(partialsObject[name]))
      })
      next()
-    })
+    } catch (err) {
+      next(err)


this should really not be a callback.

mcollina · 2023-07-12T07:55:06Z

Hopefully performance will not be degraded when we are finished :/.

mcollina · 2023-07-12T15:38:14Z

how are the benchmarks?

multivoltage · 2023-07-12T15:51:08Z

at my commit:

┌─────────┬────────┬────────┬────────┬────────┬───────────┬─────────┬────────┐
│ Stat    │ 2.5%   │ 50%    │ 97.5%  │ 99%    │ Avg       │ Stdev   │ Max    │
├─────────┼────────┼────────┼────────┼────────┼───────────┼─────────┼────────┤
│ Latency │ 141 ms │ 153 ms │ 286 ms │ 324 ms │ 161.49 ms │ 32.4 ms │ 344 ms │
└─────────┴────────┴────────┴────────┴────────┴───────────┴─────────┴────────┘
┌───────────┬─────────┬─────────┬─────────┬─────────┬─────────┬────────┬─────────┐
│ Stat      │ 1%      │ 2.5%    │ 50%     │ 97.5%   │ Avg     │ Stdev  │ Min     │
├───────────┼─────────┼─────────┼─────────┼─────────┼─────────┼────────┼─────────┤
│ Req/Sec   │ 5003    │ 5003    │ 6463    │ 6995    │ 6090    │ 890.18 │ 5000    │
├───────────┼─────────┼─────────┼─────────┼─────────┼─────────┼────────┼─────────┤
│ Bytes/Sec │ 1.34 MB │ 1.34 MB │ 1.73 MB │ 1.87 MB │ 1.63 MB │ 239 kB │ 1.34 MB │
└───────────┴─────────┴─────────┴─────────┴─────────┴─────────┴────────┴─────────┘

Req/Bytes counts sampled once per second.
# of samples: 5

31k requests in 5.03s, 8.16 MB read

I start reading this issue

Using

const readFile = require("util").promisify(require("fs").readFile)

is really impressive and performance come back to the same than master commit.
All my test are made with node 18.
@mcollina what do you think about ?

Uzlopak · 2023-10-25T07:43:22Z

@mcollina
@multivoltage

What about this PR?

multivoltage · 2023-10-25T08:30:32Z

@Uzlopak
Hi. Since there was a lot of code new after some release I decided to create new branch from new release and did same commit from 0. So I have a ready branch but not pushed.
That because after my last reply I did not find a method different from node utils promisify which keep same perfomance as original method readFile

mweberxyz · 2024-02-09T23:55:39Z

@multivoltage are you still in continuing with this work? I am interested in picking it up where you left off.

multivoltage · 2024-02-10T20:51:52Z

@mweberxyz nope in this moment. But in 2/3 days I will update my PR and maybe we can help each other if you want

multivoltage · 2024-02-12T15:39:08Z

@mweberxyz
I updated my branch if you want to start from that.
Two main points that are important for me:

const { readFile } = require("node:fs").promises is VERY slow
I ended with const readFile = require("node:util").promisify(require("node:fs").readFile); witch is similar do sync version, but a little bit slower
in node js 21 test fails

multivoltage · 2024-02-14T07:10:42Z

thank @mweberxyz, I will close the issue :)

Uzlopak · 2024-02-14T07:12:49Z

Whats the point of using promisify on a sync function? You will anyway block the Event loop.

multivoltage · 2024-02-14T10:31:27Z

Only because async version of readFile seem to be slower than sync version.
But nevermind since I closed PR :) in favor of new one by @mweberxyz

mcollina requested changes Jul 10, 2023

View reviewed changes

Eomm reviewed Jul 10, 2023

View reviewed changes

mcollina reviewed Jul 10, 2023

View reviewed changes

multivoltage force-pushed the chore/refactor-promises branch from 6b84619 to 8056027 Compare February 12, 2024 12:20

refactor: use await instead promise

8869daa

multivoltage force-pushed the chore/refactor-promises branch from 8056027 to 8869daa Compare February 12, 2024 12:22

multivoltage closed this Feb 14, 2024

multivoltage deleted the chore/refactor-promises branch February 14, 2024 07:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: use async/await instead then() in promises #379

refactor: use async/await instead then() in promises #379

multivoltage commented Jul 9, 2023

mcollina Jul 10, 2023

mcollina Jul 10, 2023

multivoltage Jul 10, 2023

mcollina Jul 10, 2023

multivoltage Jul 10, 2023

mcollina Jul 10, 2023

Eomm Jul 10, 2023

multivoltage commented Jul 10, 2023

mcollina Jul 10, 2023

mcollina Jul 10, 2023

mcollina Jul 10, 2023

multivoltage Jul 11, 2023

mcollina Jul 12, 2023

multivoltage Jul 12, 2023

mcollina Jul 10, 2023

mcollina commented Jul 12, 2023

mcollina commented Jul 12, 2023

multivoltage commented Jul 12, 2023 •

edited

Loading

Uzlopak commented Oct 25, 2023

multivoltage commented Oct 25, 2023

mweberxyz commented Feb 9, 2024

multivoltage commented Feb 10, 2024

multivoltage commented Feb 12, 2024

multivoltage commented Feb 14, 2024

Uzlopak commented Feb 14, 2024

multivoltage commented Feb 14, 2024

		@@ -260,7 +265,7 @@ function fastifyView (fastify, opts, next) {
		}

		function readCallback (that, page, data, localOptions) {

	async function fastifyView (fastify, opts, next) {
	async function fastifyView (fastify, opts) {

refactor: use async/await instead then() in promises #379

refactor: use async/await instead then() in promises #379

Conversation

multivoltage commented Jul 9, 2023

Checklist

Why this PR?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

multivoltage commented Jul 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina commented Jul 12, 2023

mcollina commented Jul 12, 2023

multivoltage commented Jul 12, 2023 • edited Loading

Uzlopak commented Oct 25, 2023

multivoltage commented Oct 25, 2023

mweberxyz commented Feb 9, 2024

multivoltage commented Feb 10, 2024

multivoltage commented Feb 12, 2024

multivoltage commented Feb 14, 2024

Uzlopak commented Feb 14, 2024

multivoltage commented Feb 14, 2024

multivoltage commented Jul 12, 2023 •

edited

Loading