goldbergyoni / nodebestpractices

:white_check_mark: The Node.js best practices list (July 2024)
https://twitter.com/nodepractices/
Creative Commons Attribution Share Alike 4.0 International
99k stars 10.04k forks source link

Some of the performance items #31

Closed gerardmrk closed 5 years ago

gerardmrk commented 6 years ago

I've written these simply (at work atm), so you'd have to reword them if you're going to add it to the README.

Ginden commented 6 years ago

There is significant overhead when accessing environment variables during runtime

Provide source on this statement please. Because as far as I know process.env is just plain objects without any getters.

gerardmrk commented 6 years ago

It was an article I read years back. I'll look for it in a while, don't wanna look like I'm going off task at work haha

gerardmrk commented 6 years ago

I can't find the original article, but here's some sources:

BrunoScheufler commented 6 years ago

Apart from the articles you posted already, there are performance implications when using process.env directly hence it might be useful to look into dependencies that wrap process.env to ensure to overcome these implications

goldbergyoni commented 6 years ago

@gerardmrk sounds like a great start for the performance section. Maybe create a new branch, start writing that section and get your name on the credit wall once we upload it?

Ginden commented 6 years ago

is a very time-consuming operation

I checked right now. Reading process.env is five times slower than reading copy. You can do it only 3600 times per milisecond.

BrunoScheufler commented 6 years ago

that's definitely an important point for the performance section!

mscdex commented 6 years ago

FWIW process.env is currently not a simple object. It has getters, setters, etc. that call out to OS-specific environment retrieval and setting APIs, so that means it has to venture into C++ land and back, which is expensive (or at least the return trip is -- I do not remember offhand).

goldbergyoni commented 6 years ago

@BrunoScheufler @mscdex what other ideas we have for performance bullets? can we fill a section of ~7 items?

mscdex commented 6 years ago

@i0natan Performance (micro optimizations or otherwise) can vary between versions of V8. So for example, some optimizations for versions of node using V8's Crankshaft optimizing compiler will be very different than those optimizations for V8's TurboFan. Even between V8 versions using the same optimizing compiler there can be dramatic changes in performance, especially as the V8 team incrementally adds fast paths that existed with Crankshaft to TurboFan and improves performance on new language features.

So if you were to include such things, you might want to at least include the node/V8 versions that the optimization applies to (or has been tested with).

gerardmrk commented 6 years ago

@i0natan I'll do it after 5pm NZ time, but I'd rather you reword any points I add in the pull request because I am bad with forming coherent statements that is easily understandable and to the point

gerardmrk commented 6 years ago

Another one I'd add is, avoid pure functional programming in very performance-critical services (e.g. chaining multiple map, filter, reduce on heavy data-structures at API entrypoints). This is a no-brainer (I mean this in the least offensive way possible, but I can't think of another way to rephrase that), but I'll try to find non-anecdotal sources later to back it up if needed, and I'll add more explanation if you feel this is not a taboo subject (the Node.js community in general stigmatizes anything non-functional, so I'll understand if you don't want to add this particular point)

mscdex commented 6 years ago

I thought I heard some time back V8 might be able to optimize the functional stuff to essentially be a regular loop, at least in some scenarios. Until then, I would also advocate non-functional (array) methods for hot code.

goldbergyoni commented 6 years ago

what v8 does now when I chain array utils like map, filter? why is it worst than looping over it myself?

@gerardmrk we are now very focused on improving the current sections, let's publish the performance section in few weeks. In the interim, we can collect many ideas (e.g. fast logger, express-cache, node_env=production, detect sync API usages using the flag warn-async, etc), form a solid list and then publish. does this make sense?

gerardmrk commented 6 years ago

@i0natan immutable Array methods like map, filter, reduce returns a new array when they're called. So for instance:

receipts
  .filter(userID)
  .map(parsedValue)
  .filter(reconciled)
  .slice(offsetBy)
  .map(mergeHeaders)
  .reduce(receiptMap)

the example creates a receipt object but in between that would've create 5 arrays total.

This is readable, maintainable, and less prone to bugs. But if your receipts array is huge and gets called very often (i.e. heavy-traffic), it's worth thinking about sacrificing the above mentioned benefits and just mutate the array in-place, but annotating it more to offset complexity if working in a team. what @mscdex said, its possible V8 is doing JIT on chains like this, I have yet to research that though, and he could be right. I've been caught up with work, I'll create the branch today after work, sorry!

gerardmrk commented 6 years ago

@i0natan also feel free to create the branch yourself and add the points if I'm taking too slow to do it, I am not fussed about not taking credits for all these, I'm just doing this cos I want to highlight important performance tips for the Node community because performance is usually a second class citizen

goldbergyoni commented 6 years ago

@gerardmrk your explanation was great, I think u belong here :)

goldbergyoni commented 6 years ago

Performance section will be added to our milestones in the next couple of days, there will group all the performance advice

snypelife commented 6 years ago

Not sure if hijacking this issue is the right approach, but I'll start here and move if that makes more sense.


This may sound silly and pretty obvious, but upgrading your apps to the latest LTS as soon as possible to take advantage of V8 improvements. Teams often get so bogged down by bugs/features, that doing something like this is always gets back burnered.


With respect to the functional/immutable comments above:

I've found that working with JS data structures in an immutable way has significantly improved application performance.

While it may seem counter-intuitive in terms of working with completely new objects/values, when operating inside of complex loops/algorithms it has made both understanding what's happening to the data far easier to comprehend in context, as well you avoid scenarios where mutations can leak, be far reaching and difficult to trace.

My team has successfully adopted this approach with one of our production APIs, and found it significantly improved performance, but I suppose YMMV.

We then took it a step further and use @substack's deep-freeze module as a simple safe guard against mutations.

This is one example I have, I'll see about digging up some more and/or finding decent articles that help make this more than just anecdotal :) .

goldbergyoni commented 6 years ago

@snypelife I can see why working with immutable can make the code more simple, but isn't [1,2,3].map(...).reduce(...) slower than imperative code?

sebs commented 6 years ago

some ideas

goldbergyoni commented 6 years ago

@sebs can u kindly clarify the first two? they both sound interesting but I'm not 100% sure I understood

VinayaSathyanarayana commented 5 years ago

Agree that we should avoid "repeatedly processing identical steps that give the same result" process.env is one of them. When a complex application is analyzed, one is likely to find a number of issues

sebs commented 5 years ago

@i0natan Processing

  1. Arrays of data potentially so big, that iterating over the array has to be done probably in a async way. Surely giving away performance per item, but all in all will be way better to handle. All you are doing here is "burst" calculations and you are fine by "Iterate faster", wehen you go out of burst, but "batch mode" the handling from one element to the next plays a bit role and even leaving optimization Potential might be ok.

  2. Doing things "evented" will be costly. I am a proponent of event oriented programing and all its design merits, but I was teached a lesson or two by node, when I did stuff like generate 50K responses in minimal time. So here is a optimization potential: Go from evented to a static, if no immutable data structure.

sebs commented 5 years ago

@snypelife I can see why working with immutable can make the code more simple, but isn't [1,2,3].map(...).reduce(...) slower than imperative code?

I'd be not so sure about that over all engines for all time. And I would really like to check out, if chaining vs. assigning each result to a new var is maybe a difference.

whithajess commented 5 years ago

@i0natan did this get forgotten about?

goldbergyoni commented 5 years ago

@whithajess This indeed got forgotten and it's about the perfect timing to bring back up! as we're now collecting performance best practices #256

Appreciate if you anyone who shared an idea here (@Ginden @gerardmrk @mscdex @snypelife @sebs) can also copy to #256 , I'll copy items that weren't copied by next week

/remind me in a week

reminders[bot] commented 5 years ago

@i0natan set a reminder for Oct 9th 2018

VinayaSathyanarayana commented 5 years ago

We should add a note on not blocking the Event loop (https://nodejs.org/en/docs/guides/dont-block-the-event-loop/)

reminders[bot] commented 5 years ago

:wave: @i0natan,

goldbergyoni commented 5 years ago

@VinayaSathyanarayana Welcome! Can you add that to #256 ?

/remind me next week

reminders[bot] commented 5 years ago

@i0natan set a reminder for Oct 18th 2018

reminders[bot] commented 5 years ago

:wave: @i0natan,

gerardmrk commented 5 years ago

I just got the reminder via email. Was that intended for me? I'm not sure if there's anything more I can contribute to this thread that hasn't been mentioned elsewhere or written better than I can articulate. If you'd like to post the stuff I've written, feel free to reword them as appropriate!

goldbergyoni commented 5 years ago

@gerardmrk No, the reminder is for me to copy the items to the main performance ideas thread.

Would you like to participate in writing as well? in any case, goes without saying that will reward you... :]

stale[bot] commented 5 years ago

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! 💚