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

Performance section pool of ideas #302

Closed
js-kyle opened this issue Dec 17, 2018 · 25 comments
Closed

Performance section pool of ideas #302

js-kyle opened this issue Dec 17, 2018 · 25 comments
Labels

Comments

@js-kyle
Copy link
Contributor

js-kyle commented Dec 17, 2018

This is our list of performance best practice ideas. Here we filter and finalize the list of items to write about.

Based upon the discussion in #256, this is an issue to track what bullets are approved, and/or who is working on them.

Title Approved Assigned to Gist
Don't block the event loop Yes @TheHollidayInn Use dedicated libs, minimize CPU-bound
Use text compression No (Generic) Compress static assets
Monitoring: prioritize customer-facing metrics @i0natan Focus on API metrics like latency, the golden signals
Avoid synchronous functions ?
Replicate the Node process, for example using a cluster Yes Multi-core can run even IO-only code faster! Use any replication techniques like K8S, PM2
Use a version of node that ships with new TurboFan JIT compiler Yes
Deal with database and external APIs in batches No (Generic)
Benchmark slow db calls No (Generic)
Use factories or constructors to ensure objects are created using the same schema so the v8 won't have to generate hidden classes on runtime Yes
Analyze repeated API Calls/Database Queries to cache them No (Generic)
Debounce and Throttle ?
Benchmarking your code with professional testing tools Yes Constantly load test your app, Apache AB, Auto-canon
Analyzing and monitoring cpu bottlenecks or memory leaks with Node Clinic/Prometheus Yes
Serve static assets from a CDN No (Generic)
Use HTTP/2 ? why
Tree shaking dependencies ? How this affects performance?
Generic performance advice Yes Various performance advice that are not Node specific
Minimize the bootstrap phase The process load time should be minimized, no code outside functions but declarations, measure using a tracer
When using DB ORMs/helpers, Query DB in lean mode Yes Most ORMs support returning the raw DB result without spending precious time without decorating each row with custom JS prototype (huge save)
Prefer native JS methods over user-land utils like Lodash Yes methods like concat & join for example, show perf diff graphs, Great article here, eslint-plugin-here, thank you @Berkmann18
@goldbergyoni
Copy link
Owner

@js-kyle You're the man

I adjusted the list a bit and added my suggestion within the status column. Seems to me like we need to refine & enrich this list a bit with @BrunoScheufler and @sagirk , I personally plan to read some perf stuff in the next few days to be able to suggest few more

p.s. Shall we add below the table a bibliography with great articles?

@goldbergyoni goldbergyoni changed the title Performance section bullets Performance section pool of ideas Dec 17, 2018
@BrunoScheufler
Copy link
Contributor

@js-kyle @i0natan sounds good, I'll assign some issues to me soon and work on some content 👍

@goldbergyoni
Copy link
Owner

goldbergyoni commented Dec 21, 2018

@js-kyle I added @Berkmann18 idea from #256 about avoiding user land util methods

@Berkmann18 Thanks for the great advice, you wanna take the lead and write this bullet?

@Berkmann18
Copy link
Contributor

@i0natan Sure, I'll get on it when I can.

@js-kyle
Copy link
Contributor Author

js-kyle commented Dec 21, 2018

I will remove Choose the classical for loop over forEach/ES6 of when dealing with huge amounts of data from the list, as the new practice covers it

@goldbergyoni
Copy link
Owner

Hey @mcollina, so we're pushing soon the performance section. The ideas we collected thus far are listed above, will be honored if you feedback or suggest some other ideas.

Concurrent question - during the writing we're probably gonna benchmark few things and would like to ensure we pick the right benchmarking tools. As of API/network benchmarking I'm pretty confident with auto-canon (:)), what would you recommend for micro-benchmarks (e.g. compare the run-time of two different functions in-process)?

@mcollina
Copy link

Some notes:

  1. typo auto-canon -> autocannon
  2. writing good microbenchmarks is hard, documenting it would be awesome
  3. Avoid synchronous functions is a bad advice. You should in fact do the opposite: do not introduce "fake" asyncronicity. In other terms, no Promise.resolve() or process.nextTick() - I've got quite a few example where adding these can bite you.
  4. Replicate the Node process, for example using a cluster - this should take into account Kubernetes and the like. In most deployments the infra will take care of this.
  5. A special section should be added to serverless/aws lambda deployments. It's quite a special case and it should be taken into consideration. As an example, I recommend using webpack to minimize startup time.
  6. Do not transpile, bundle or mingle your source code. Plain old JS is easier to optimize and it would be harder to map your performance issue with your code. (Note that this conflicts with 5, but there are different goals).
  7. Use a version of node that ships with new TurboFan JIT compiler - Node 6 is being retired in a month or so. You should not be talking about this, all Node LTS versions will be Turbofan-enabled in a month.
  8. Set NODE_ENV to production is going to be valuable only for express-based application. This is a convention, and not something used by Node internally.

Feel free to ping me for reviews.

@TheHollidayInn
Copy link
Contributor

Thanks @mcollina!!

Could you elaborate on 3? I think Avoid synchronous functions usually refers to things like reading file syncs on an api request. But I'm interested in hearing more :D

@mcollina
Copy link

mcollina commented Mar 23, 2019

All the following examples are wrong:

async function a () {
  const content = fs.readFileSync(__filename)
  return await b(content)
}
async function a () {
  return await b()
}

// Do not use async!!!
async function b () {
  return 42
}
async function a () {
  return await b()
}

// Do not use async!!!
function b () {
  return Promise.resolve(42)
}

Avoid synchronous function can be intended as "always put async in front of functions" - which is a very bad advice.

@goldbergyoni
Copy link
Owner

@mcollina Thanks a bunch for these words of wisdom. 99% resonates with me.

Few minor following questions/remarks:

  1. "writing good microbenchmarks is hard, documenting it would be awesome

Could you recommend one or this is a tautology to saying there's not even one?

  1. "As an example, I recommend using webpack to minimize startup time"

Just curious, any solid benchmarks on the cost of building the dependencies tree (e.g. evaluating 200 require calls and loading files)?

  1. "Avoid synchronous functions is a bad advice."

I believe this is related to blocking the flow with sync IO (e.g. fs.readFileSync, --trace-sync-io) - does this make sense?

@TheHollidayInn
Copy link
Contributor

Ah okay I see. So we just need to clarify what we mean by avoid sync and possibly reword the tip.

I have a small start on an issue that I've been meaning to turn into a PR. And I'll do that then ask for your help on clarifying the above if you don't mind.

@mcollina
Copy link

I believe this is related to blocking the flow with sync IO (e.g. fs.readFileSync, --trace-sync-io) - does this make sense?

Yes exactly.

@goldbergyoni
Copy link
Owner

@VinayaSathyanarayana @migramcastillo @Berkmann18 @js-kyle @TheHollidayInn @BrunoScheufler @js-kyle

Performance writers, let's make this happen and share this vital knowledge? we would like release ~5 more performance items by 15th May. This means that if we take it together - each has to write a single bullet in a month. Would you like to participate?

If yes, please pick from the approved items in the list above which is your favorite topics 👋

before writing the entire bullet, consider opening an issue for a short brainstorming - this can greatly help to ensure all the important piece of information are included

@TheHollidayInn This release will include your generic list and don't block the loop items 😄

I'm taking the item "Focus on customer-facing metrics" and have more than a few ideas to share with the writers of the other bullets

@Berkmann18
Copy link
Contributor

@i0natan I'm up for that.

I'll happily have a go at either of the following:

  • Tree shaking dependencies
  • Generic performance advice

@goldbergyoni
Copy link
Owner

@Berkmann18 I'm happy.

@TheHollidayInn Is already taking care for this with a comprehensive generic list.

Let's do treeshaking dependencies, would you mind opening an issue to discuss the TOC of this?

See also @mcollina on this above: treeshaking is only one technique to boost the app start time, maybe we should focus on the goal (faster bootstrap) and name various techniques like tree shaking, minifying, etc. Could be also great to show some benchmark how faster a code/serverless boost when remove the need to resolve 200 files during bootstrap

@BrunoScheufler @js-kyle @VinayaSathyanarayana @migramcastillo What's your preferred topics?

@Berkmann18
Copy link
Contributor

@i0natan Wait, so @TheHollidayInn is working on both topics?

@migramcastillo
Copy link

@i0natan sorry I've been very busy lately, so I'm retaking this and reading some stuff above and in other issues, few doubts:

  • The original topic I was working on is about performance enhancement of using Promise.all instead of sequential await sentences for making a better use of the multi-thread, is this still on or it'll be cover on the Don't block the event loop topic? Cause I think is some kind related.
  • If the topic is canceled or covered in any other topic, I can work on Replicate the Node process

@mcollina
Copy link

Note that Promise.all does not guarantee the use of multi-threading.

@goldbergyoni
Copy link
Owner

@i0natan Wait, so @TheHollidayInn is working on both topics?

Oh no, only on the 'generic items'. Can you handle the 'optimize dependencies for faster bootstrap' (or how you want to call it)?

@Berkmann18

@goldbergyoni
Copy link
Owner

  • If the topic is canceled or covered in any other topic, I can work on Replicate the Node process

Both items are valid. I would prioritize the process replication since parallelizing things (Promise.all) is more of a generic advice that applies to many programming language where the need to replicate processes is more unique to the webserver-less approach of node. Pick your preferred idea. Shall we start with an issue that lists the TOC?

@migramcastillo

@Berkmann18
Copy link
Contributor

@i0natan

Oh no, only on the 'generic items'. Can you handle the 'optimize dependencies for faster bootstrap' (or how you want to call it)?

Ah okay, sure! On that note, have you had time to explore the idea you mentioned on my test repo?

@goldbergyoni
Copy link
Owner

Ah okay, sure! On that note, have you had time to explore the idea you mentioned on my test repo?

Yes, can we hop on a call sometime next week? would you kindly approach via me at goldbergyoni.com?

@Berkmann18
Copy link
Contributor

Berkmann18 commented Apr 14, 2019

@i0natan

Yes, can we hop on a call sometime next week? would you kindly approach via me at goldbergyoni.com?

Sure, email sent.

@goldbergyoni
Copy link
Owner

@Berkmann18 Cool, anyway feel free to push the faster bootsrap item

@stale
Copy link

stale bot commented Jul 18, 2019

Hello there! 👋
This issue has gone silent. Eerily silent. ⏳
We currently close issues after 100 days of inactivity. It has been 90 days since the last update here.
If needed, you can keep it open by replying here.
Thanks for being a part of the Node.js Best Practices community! 💚

@stale stale bot added the stale label Jul 18, 2019
@stale stale bot closed this as completed Jul 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants
@mcollina @TheHollidayInn @BrunoScheufler @Berkmann18 @goldbergyoni @js-kyle @migramcastillo and others