[debadree25]

An attempt at increasing the default semi-space as per the discussion here nodejs/performance#67

(not sure if its testable)

Refs: nodejs/performance#67

[nodejs-github-bot]

Review requested:

• @nodejs/startup

[legendecas]

Should this apply to embedders like electron as well?

[debadree25]

I think it should apply? electron also probably wont be running on severely constrained environments where this might affect i think

[bnoordhuis]

How do you validate a change like this? We have benchmarks but we don't have a machine matrix that tells us "hey, this is really beneficial on big iron but it destroys raspberry pis."

[bnoordhuis]

Is it still possible to override it from the command line? Initialization is so spaghetti-like these days I have a hard time telling if this comes before or after V8::SetFlagsFromCommandLine().

[debadree25]

I think yes V8::SetFlagsFromCommandLine() is being called at ProcessGlobalArgsInternal which is called after this line

[debadree25]

@joyeecheung or @addaleax I think can validate if am correct

[debadree25]

Regards to testing even I have no idea honestly 😅, improvement seems to be there on local tests and running the web-tooling-benchmark nodejs/performance#67 (comment) but other than that not sure
The readme on @nodejs/build suggests we have some raspberry pis, so are those usable for testing such a case if someone could enlighten

[joyeecheung]

You can just set this in the resource constraints in the isolate’s Isolate::CreateParams (https://v8.github.io/api/head/structv8_1_1Isolate_1_1CreateParams.html), which can be done in

node/src/api/environment.cc

Line 229 in fe449a2

void SetIsolateCreateParamsForNode(Isolate::CreateParams* params) {

, this allows us to no go over the system memory available and makes it possible for users to override it with a flag

[debadree25]

Hey so using the ResourceConstraints class we should set set_max_young_generation_size_in_bytes value then? to update the value on semi space? the doc says it includes two semi-spaces and a large object space. is the same as setting --max_semi_space_size=64 or will it be different?

[debadree25]

I tried something like this

diff --git a/src/api/environment.cc b/src/api/environment.cc
index fd187f7342..409795b7a4 100644
--- a/src/api/environment.cc
+++ b/src/api/environment.cc
@@ -231,12 +231,14 @@ void SetIsolateCreateParamsForNode(Isolate::CreateParams* params) {
   const uint64_t total_memory = constrained_memory > 0 ?
       std::min(uv_get_total_memory(), constrained_memory) :
       uv_get_total_memory();
+  const uint64_t max_allowed_young_gen_size_in_bytes = 64 * 1024 * 1024;
   if (total_memory > 0 &&
       params->constraints.max_old_generation_size_in_bytes() == 0) {
     // V8 defaults to 700MB or 1.4GB on 32 and 64 bit platforms respectively.
     // This default is based on browser use-cases. Tell V8 to configure the
     // heap based on the actual physical memory.
     params->constraints.ConfigureDefaults(total_memory, 0);
+    params->constraints.set_max_young_generation_size_in_bytes(params->constraints.max_old_generation_size_in_bytes() / 64);
   }
   params->embedder_wrapper_object_index = BaseObject::InternalFields::kSlot;
   params->embedder_wrapper_type_index = std::numeric_limits<int>::max();
diff --git a/src/node.cc b/src/node.cc
index 7fcc60cf46..2aba7333d9 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -779,8 +779,6 @@ static ExitCode InitializeNodeWithArgsInternal(
   // is security relevant, for Node it's less important.
   V8::SetFlagsFromString("--no-freeze-flags-after-init");

The perf improvement seems to be the same, but cant understand whether its setting the semi size or size of the entire scavenger heap (i think?)

[ronag]

This should provide some insight regarding semi space vs young generation size

https://chromium.googlesource.com/v8/v8/+/8024204828361d4bc9d5be9f63f368dcc943fa9a/src/heap/heap.cc#300
https://chromium.googlesource.com/v8/v8/+/8024204828361d4bc9d5be9f63f368dcc943fa9a/src/heap/heap.cc#4512

static constexpr size_t kNewLargeObjectSpaceToSemiSpaceRatio = 1;

size_t Heap::SemiSpaceSizeFromYoungGenerationSize(size_t young_generation_size) {
  return young_generation_size / (2 + kNewLargeObjectSpaceToSemiSpaceRatio);
}

So basically semi space is a third of young generation space

[ronag]

Also useful v8/v8@e423f00

[debadree25]

Ok looking at the code that initialises these heaps here: https://source.chromium.org/chromium/chromium/src/+/refs/heads/main:v8/src/heap/heap.cc;drc=7a672572345786ff318aa95e32f13bc0354328bf;bpv=1;bpt=1;l=344?q=ConfigureDefaults&ss=chromium%2Fchromium%2Fsrc&gsn=GenerationSizesFromHeapSize&gs=KYTHE%3A%2F%2Fkythe%3A%2F%2Fchromium.googlesource.com%2Fcodesearch%2Fchromium%2Fsrc%2F%2Fmain%3Flang%3Dc%252B%252B%3Fpath%3Dv8%2Fsrc%2Fheap%2Fheap.cc%23uRZS21wfKKCXKZIAbkrtU8ywzF0LRx_CGW9hSHNKb7I it seems like based on the young generation size it calculates the required old gen size and ensures the sum total of two is <= heap size so if we increase the semi space size we have to proportionally decrease the old generation size, so the question becomes what should be our proportions for this, also i think this is causing the test failures all of it relate to oom tests

[richardlau]

The readme on @nodejs/build suggests we have some raspberry pis, so are those usable for testing such a case if someone could enlighten

The Pi's have sadly now been retired and are no longer available.

[debadree25]

Maybe we could then test locally with memory-constrained vms then? although I think the issue right now becomes changing the proportions of partitions between old generation size and young generation size the total memory available would be the same

[debadree25]

Heres the function that generates the young heap size given the old gen size and this is the ratio being used is here maybe we could also pursue this on the v8 side to update these defaults atleast for the non-low memory situation also looks like there was intent to increase heap size here @ronag also cc @nodejs/v8

[ronag]

pursue this on the v8 side to update these defaults atleast for the non-low memory situation

I think that's unlikely to go anywhere.

[debadree25]

Understood, I will then update to include the partitioning logic on our side like in here then, tunring this into a draft for now

[github-actions]

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.