Alias node with flags for dyno-specific memory limits

[hunterloftis]

Eg, something like this for a 2x dyno:

alias node='node --gc_global --optimize_for_size --max_old_space_size=960 --always_compact --max_executable_size=64 --gc_interval=100 --expose_gc'

The alias should be opt-in. Maybe with an env var like NODE_ALIAS=true?

To illustrate the problem, this is from a customer - pre and post GC flags:

[ojacobson]

Wow, that graph is striking. Defaulting to sane settings for the dyno type is clearly a smart choice.

Two thoughts occur:

1. Is it possible to compute the node options from the values provided by ulimit/memory info/etc, instead of doing the giant lookup table on memory size that we've historically had trouble with in other buildpacks? I know that better dyno metadata would generally improve the situation, but we can probably survive for now using what the OS reports.
2. I feel like the node argv should be unsurprising from the point of view of a Node developer. If a user puts node --max_old_space_size=1024 … in their Procfile, for example, we should probably pass node --gc_global --optimize_for_size --always_compact … --max_old_space_size=1024 … to the actual Node binary, dropping the buildpack default argument out of argv entirely and appending the user-supplied arguments at the end. I don't want to have to support users whose primary complaint is "node behaves weirdly when we pass this argument" when the root cause is "we're actually passing two copies of the same option."

I think that rules out an alias, but we could make something more sophisticated (a shell function, if we want to avoid polluting PATH, or a real node wrapper script, if we want subprocess node invocations to get the same default options).

@jkutner I know you've looked at writing a java wrapper to do things like apply $JAVA_OPTS and set up memory limits automatically, and opted not to do that. What kind of issues have you run into?

[hone]

Are there ways you could use env vars? If so, we do this in Ruby land for setting sane defaults. This is where bin/release comes in handy. This way it does not affect existing apps, but all new pushed apps get it. In addition, when you run heroku config you can see env vars that are set and can change them as a user and it won't get clobbered by Heroku.

[ojacobson]

bin/release doesn't know the dyno size the app will ultimately run on, which makes things like computed memory limits a bit tricky.

[hunterloftis]

Defaulting to sane settings

Perhaps it makes sense to even default to this and to allow opt-out. My concern with that is primarily the potential disruption to existing apps from hard crashes. Maybe what @hone was saying about bin/release could help me avoid disturbing existing apps.

Is it possible to compute the node options from the values provided by ulimit/memory info/etc, instead of doing the giant lookup table on memory size that we've historically had trouble with in other buildpacks?

I suspect that I'd inject this into the profile script much like the existing WEB_CONCURRENCY stuff, since it depends on the same runtime container info.

I don't want to have to support users whose primary complaint is "node behaves weirdly when we pass this argument" when the root cause is "we're actually passing two copies of the same option."

Agreed. I'd like to test what node does when you just append duplicate flags though because an alias is so simple, and it may do exactly what I would hope (use the last provided value).

@hone could you link me to the similar stuff you're doing for ruby?

[jkutner]

For the JVM I set JAVA_TOOL_OPTIONS, which is picked up by any Java process. Any command-line args override it.

The wrapper @ojacobson mentioned was for dealing with some stderr messages I was trying to suppress, so kinda unrelated. I decided against it because it make debugging hard for me -- but never had complaints from users (I had it in prod for months).

I think the wrapper is a good idea. So basically you would have a node executable that is really just a shell script. then it would do something like:

# do some opt processing...
default_opts="sane opts based on processing"
.heroku/nodejs/bin/node $default_opts $@

[hone]

@ojacobson that's a fair point. ultimately, we ended up leveraging .profile.d.

@hunterloftis we've used it for stuff like RAILS_ENV, I can't think of a good way to do it per dyno type without using .profile.d, but that would affect existing apps.

[GertSallaerts]

Is this still under consideration? We were running into the memory issues described above and finally found this. But it took some time to discover the root of our issue. We want to apply this to all our node apps and were thinking about rolling out a custom buildpack until I came across this.

When I tested this, node always took the last argument if you pass multiple of the same, so I think an alias wouldn't be a problem.

There is one more potential issue I can think of though. For performance dyno's etc, you'd have WEB_CONCURRENCY=5 for example, so you should divide the sane defaults by 5 as well to prevent the app from exceeding the memory quota. However, for users that do not cluster their app, this would mean they get only a fraction of the available memory by default. Might be confusing for them?

[danielleadams]

Going to close this since the memory tuning should not be needed any more. If there are other memory issues, please open another issue.

[JakeChampion]

Hi @danielleadams,

Is there some more information you could share with us about why using the flags: --max_semi_space_size and --max_old_space_size is now not needed?

[mpvosseller]

@JakeChampion Not sure if this is what @danielleadams is referring to but there is the following note in the Heroku docs:

Versions of Node that are >= 12 may not need to use the --optimize_for_size and --max_old_space_size flags because JavaScript heap limit will be based on available memory.