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

Some of the performance items #31

Closed
gerardmrk opened this issue Nov 7, 2017 · 36 comments
Closed

Some of the performance items #31

gerardmrk opened this issue Nov 7, 2017 · 36 comments

Comments

@gerardmrk
Copy link

gerardmrk commented Nov 7, 2017

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.

  • There is significant overhead when accessing environment variables during runtime, so avoid referencing process.env per HTTP request in your server code.

  • Don't transpile your code in-memory at runtime (e.g. with transpilers like babel-register or ts-node), as they have to transpile everything and consume a lot of memory. Rather, pre-transpile your code before pushing to production.

@Ginden
Copy link

Ginden commented Nov 7, 2017

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
Copy link
Author

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
Copy link
Author

gerardmrk commented Nov 7, 2017

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

@BrunoScheufler
Copy link
Contributor

BrunoScheufler commented Nov 7, 2017

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
Copy link
Owner

@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
Copy link

Ginden commented Nov 7, 2017

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
Copy link
Contributor

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

@mscdex
Copy link

mscdex commented Nov 7, 2017

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
Copy link
Owner

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

@mscdex
Copy link

mscdex commented Nov 7, 2017

@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
Copy link
Author

@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
Copy link
Author

gerardmrk commented Nov 7, 2017

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
Copy link

mscdex commented Nov 7, 2017

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
Copy link
Owner

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?

@goldbergyoni goldbergyoni self-assigned this Nov 9, 2017
@gerardmrk
Copy link
Author

@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
Copy link
Author

gerardmrk commented Nov 9, 2017

@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
Copy link
Owner

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

@goldbergyoni
Copy link
Owner

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

@snypelife
Copy link

snypelife commented Nov 15, 2017

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
Copy link
Owner

@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
Copy link

sebs commented Apr 7, 2018

some ideas

  • patterns for data with "undefined" length, preferably async/await cases.
  • do not over use events if you can get away with a simple access of error
  • patterns for degradation and detection of "overuse" (queues, runtime checks)
  • emulate the event queue in node ;)
  • there is a stark difference between a efficient request/response mechanism and a file parser. Node is not just http

@goldbergyoni
Copy link
Owner

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

@VinayaSathyanarayana
Copy link
Contributor

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
Copy link

sebs commented Oct 1, 2018

@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
Copy link

sebs commented Oct 1, 2018

@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
Copy link

@i0natan did this get forgotten about?

@goldbergyoni
Copy link
Owner

@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 reminders bot added the reminder label Oct 2, 2018
@reminders
Copy link

reminders bot commented Oct 2, 2018

@i0natan set a reminder for Oct 9th 2018

@VinayaSathyanarayana
Copy link
Contributor

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

@reminders reminders bot removed the reminder label Oct 9, 2018
@reminders
Copy link

reminders bot commented Oct 9, 2018

👋 @i0natan,

@goldbergyoni
Copy link
Owner

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

/remind me next week

@reminders reminders bot added the reminder label Oct 11, 2018
@reminders
Copy link

reminders bot commented Oct 11, 2018

@i0natan set a reminder for Oct 18th 2018

@reminders reminders bot removed the reminder label Oct 18, 2018
@reminders
Copy link

reminders bot commented Oct 18, 2018

👋 @i0natan,

@gerardmrk
Copy link
Author

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
Copy link
Owner

@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
Copy link

stale bot commented Mar 25, 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 Mar 25, 2019
@stale stale bot closed this as completed Apr 4, 2019
goldbergyoni pushed a commit that referenced this issue Mar 27, 2021
MatanYadaev pushed a commit to MatanYadaev/nodebestpractices that referenced this issue Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants