BAAS-32231: Track memory of temporary value created in Array.prototype.map calls #123

Calvinnix · 2024-06-18T18:45:30Z

This change allows us to prevent the following function from OOMing pods.

exports = async function () {
  let variants = [];
  for (let i = 0; i < 10; i++) { // array of size 1000 would easily OOM a pod
      variants.push({})
  }
  variants = variants.map((variant) => {
      return {
          a: 'a'.repeat(100 * 1024 * 1024), // 100mb,
      }
  })
  // this function creates 1GB of memory, and succeeds
};

vm.go

Calvinnix · 2024-06-18T18:57:18Z

builtin_array.go

@@ -786,6 +786,7 @@ func (r *Runtime) arrayproto_map(call FunctionCall) Value {
 	if _, stdSrc := o.self.(*arrayObject); stdSrc {
 		if arr, ok := a.self.(*arrayObject); ok {
 			values := make([]Value, length)


A concern I had about this approach was the potential for double counting memory, I'm including a breakdown about why this is not an issue.

values variable memory utilization goes untracked.

Calvinnix · 2024-06-18T18:58:23Z

builtin_array.go

@@ -786,6 +786,7 @@ func (r *Runtime) arrayproto_map(call FunctionCall) Value {
 	if _, stdSrc := o.self.(*arrayObject); stdSrc {
 		if arr, ok := a.self.(*arrayObject); ok {
 			values := make([]Value, length)
+			r.vm.tmpValues = values


We assign a reference to the values array so that we are now tracking memory utilization of this temporary values array.

Is there any chance this might be used concurrently (I don't think so but you might know better at this point)? I haven't looked into it but just thought about it as part of the changes we made recently in our http2 package and wanted to be careful since this is now shared from the runtime.

Yeah so this is one of the main reasons that I steered away from a previous approach that consisted of putting the temporary objects directly onto the stack. I suspect this is possible but not something we'd commonly encounter.

I think an operation like the one below might give us concurrent usage like you're suggesting

Promise.all([arr1.map(asyncFunc), arr2.map(asyncFunc)])

The end result is that we'd only be able to track a single map operation at a time. Unless we do something like create a unique hash for the map operation and track the values array under that specific hash.

Unless we do something like create a unique hash for the map operation and track the values array under that specific hash.

Well technically since these are references we'd probably be fine with something like [][]Values and the memory tracker might also need to strip out the nil references since those are no longer needed.

JS should be single-threaded, so this function should never be used concurrently within the same runtime unless we have another bug. I think it's safe to assume that this code will run on a single thread. Other members would run into data races as well if that wasn't the case.

My concern was that even though it is single threaded, that it would not finish the entire map operation and bounce around.

Wait.. I found a way to recreate what I was worried about

exports = async function(arg){ var arr = [1,2,3,4,5] await Promise.all([ arr.map(async x => { setTimeout(() => { console.log(x) }, 1000 - x) }), arr.map(async x => { setTimeout(() => { console.log(x) }, 1000 - x) }), arr.map(async x => { setTimeout(() => { console.log(x) }, 1000 - x) }), ]) };

> took 1.272834522s > logs: 5 5 5 4 3 4 4 2 2 3 3 2 1 1 1

Also TIL that these map functions execute in parallel for the entire array 👁️👄👁️

I'm going to create a follow-up to address this concern so that we can go ahead and address the immediate concern of map memory usage because we know we've OOMed because of this.

cc: @arahmanan for visibility because I know we briefly discussed potential solutions.

https://jira.mongodb.org/browse/BAAS-32408

Calvinnix · 2024-06-18T19:08:11Z

builtin_array.go

@@ -794,6 +795,7 @@ func (r *Runtime) arrayproto_map(call FunctionCall) Value {
 					values[k] = callbackFn(fc)


[important] the corresponding value that is created is tracked within the callbackFn function because the value is pushed on the stack BUT that value is popped off the stack at the end of the function call.

The only difference is now instead of just stopping tracking this value altogether, we are moving it to be tracked in the tmpValues array.

Calvinnix · 2024-06-18T19:08:55Z

builtin_array.go

@@ -794,6 +795,7 @@ func (r *Runtime) arrayproto_map(call FunctionCall) Value {
 					values[k] = callbackFn(fc)
 				}
 			}
+			r.vm.tmpValues = nil


nil out the tmpValues array because the values array will now be tracked when this function returns and the returned array object is pushed onto the stack.

vm.go

builtin_array.go

Gabri3l

Nice approach, do you mind also adding some comments in code so it's easier to track it down in the future and understand why we needed this?

Gabri3l · 2024-06-18T21:34:09Z

builtin_array.go

@@ -786,6 +786,7 @@ func (r *Runtime) arrayproto_map(call FunctionCall) Value {
 	if _, stdSrc := o.self.(*arrayObject); stdSrc {
 		if arr, ok := a.self.(*arrayObject); ok {
 			values := make([]Value, length)
+			r.vm.tmpValues = values


Is there any chance this might be used concurrently (I don't think so but you might know better at this point)? I haven't looked into it but just thought about it as part of the changes we made recently in our http2 package and wanted to be careful since this is now shared from the runtime.

vm.go

Calvinnix · 2024-06-24T20:33:04Z

memory_test.go

+				checkMem();
+				return "a"
+			})`,
+			expectedSizeDiff: 4 * (1 + SizeString),


The notable thing here is that we are now seeing a size diff (this is 0 before this fix)

Calvinnix · 2024-06-24T20:33:36Z

vm_test.go

@@ -561,6 +561,60 @@ func TestStashMemUsage(t *testing.T) {
 	}
 }

+func TestTmpValuesMemUsage(t *testing.T) {


copy pasta from the stack test 😄

arahmanan

LGTM! Just some minor stuff from my end.

arahmanan · 2024-06-24T20:51:53Z

memory_test.go

+				checkMem();
+				return "a"
+			})`,
+			expectedSizeDiff: 4 * (1 + SizeString),


are we multiplying by 4 because we're checking the memory before returning the last element (number 5)?

Yeah that's correct

arahmanan · 2024-06-24T20:54:07Z

vm.go

+	return ValuesMemUsage(s, ctx)
+}
+
+func ValuesMemUsage(s []Value, ctx *MemUsageContext) (memUsage uint64, err error) {


do we need to export this function?

Oh yeah goja is under the same package, yeah I'll make this a private function, nice catch!

arahmanan · 2024-06-24T20:56:58Z

vm_test.go

+			name:        "should exit early given tmpValues over the memory limit",
+			val:         []Value{valueInt(99), valueInt(99), valueInt(99), valueInt(99)},
+			memLimit:    0,
+			expectedMem: SizeInt,


[nit] thoughts on adding a similar test without the limit set to 0 to make sure that we're iterating over all values.

I updated the test above to account for multiple values, does that work for you?

💯 perfect!

Gabri3l

Super minor comments, 🚀

memory_test.go

vm.go

kpatel71716

LGTM! Nice stuff! I'm also good pushing the follow-up into BAAS-32408

Gabri3l

…e.map calls

Calvinnix commented Jun 18, 2024

View reviewed changes

vm.go Outdated Show resolved Hide resolved

Calvinnix commented Jun 18, 2024

View reviewed changes

Calvinnix requested review from Gabri3l, arahmanan and kpatel71716 June 18, 2024 19:09

arahmanan reviewed Jun 18, 2024

View reviewed changes

vm.go Outdated Show resolved Hide resolved

builtin_array.go Show resolved Hide resolved

Gabri3l reviewed Jun 18, 2024

View reviewed changes

Calvinnix commented Jun 24, 2024

View reviewed changes

Calvinnix requested review from arahmanan and Gabri3l June 24, 2024 20:33

arahmanan approved these changes Jun 24, 2024

View reviewed changes

Gabri3l approved these changes Jun 25, 2024

View reviewed changes

memory_test.go Show resolved Hide resolved

vm.go Show resolved Hide resolved

Calvinnix requested a review from Gabri3l June 26, 2024 18:57

kpatel71716 approved these changes Jun 26, 2024

View reviewed changes

Gabri3l approved these changes Jun 26, 2024

View reviewed changes

Calvinnix added 4 commits June 26, 2024 16:09

BAAS-32231: Track memory of temporary value created in Array.prototyp…

771a9ab

…e.map calls

add comments, unit tests, and refactor type

ade6829

code review changes

15b9438

include SizeEmptySlice in memory calculation, + add comments

62a1382

Calvinnix force-pushed the BAAS-32231 branch from 46099e3 to 62a1382 Compare June 26, 2024 20:09

Calvinnix merged commit fbae8e7 into mongodb-forks:realm Jun 26, 2024
2 of 6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BAAS-32231: Track memory of temporary value created in Array.prototype.map calls #123

BAAS-32231: Track memory of temporary value created in Array.prototype.map calls #123

Calvinnix commented Jun 18, 2024

Calvinnix Jun 18, 2024

Calvinnix Jun 18, 2024

Calvinnix Jun 18, 2024

Gabri3l Jun 18, 2024

Calvinnix Jun 24, 2024

Calvinnix Jun 24, 2024

arahmanan Jun 24, 2024

Calvinnix Jun 24, 2024

Calvinnix Jun 24, 2024 •

edited

Loading

Calvinnix Jun 25, 2024

Calvinnix Jun 26, 2024

Calvinnix Jun 26, 2024

Calvinnix Jun 18, 2024

Calvinnix Jun 18, 2024

Gabri3l left a comment

Gabri3l Jun 18, 2024

Calvinnix Jun 24, 2024

Calvinnix Jun 24, 2024

arahmanan left a comment

arahmanan Jun 24, 2024

Calvinnix Jun 24, 2024

arahmanan Jun 24, 2024

Calvinnix Jun 24, 2024

arahmanan Jun 24, 2024

Calvinnix Jun 24, 2024

arahmanan Jun 24, 2024

Gabri3l left a comment

kpatel71716 left a comment

Gabri3l left a comment

		@@ -794,6 +795,7 @@ func (r *Runtime) arrayproto_map(call FunctionCall) Value {
		values[k] = callbackFn(fc)

BAAS-32231: Track memory of temporary value created in Array.prototype.map calls #123

BAAS-32231: Track memory of temporary value created in Array.prototype.map calls #123

Conversation

Calvinnix commented Jun 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Calvinnix Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gabri3l left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arahmanan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gabri3l left a comment

Choose a reason for hiding this comment

kpatel71716 left a comment

Choose a reason for hiding this comment

Gabri3l left a comment

Choose a reason for hiding this comment

Calvinnix Jun 24, 2024 •

edited

Loading