BAAS-22214: capture memory used by objectGoMapSimple and objectGoSlice #88

nathantfrank · 2023-07-31T17:01:46Z

No description provided.

Gabri3l · 2023-08-01T15:44:18Z

value.go

-	case *objectGoMapSimple:
-		return SizeEmpty, nil


[major] The fact that no test is failing worries me as this is a "breaking" change. Can we add tests even just around this MemUsage function? You can look at toValue to see how it's used or leverage that directly in the test body.

I think this will require to add a MemUsage function to objectGoMapSimple. But in order to calculate it you would call the MemUsage function from the embedded baseObject + you have to calculate it for the data field.

The catch here is that you have to loop through key/values and calculate the memory usage for each pair. Keys mem usage is calculated as uint64(len(k)) + SizeString, but SizeString is not available yet and it's part of my PR (you can just copy paste it here, it's a constant value anyway). For the values it's probably easier to convert them to a value (call ToValue) and then use the MemUsage on them (I think you can just call o._getStr(k) and use the return value to call MemUsage on it.

Let me know if you have any questions!

Roger, Hmmm okay, yeah it appeared to be incrementing the fields when I ran this in the debugger against a VM test, but it also didn't fail the test. I'm now wondering if the MemUsage is already managing to take into account what's contained in the map... I'll write some tests as I investigate.

I ran the debugger for the BAAS test TestMemUsageBSON found here. I noticed it looks like if we add this we will also start tracking the memory used by our context, console, etc... For example:

Wasn't sure if this would be an unexpected change and whether we want to capture it or not.

Just wanted to confirm if you are correct, for my example test here, it looks like the default memory usage implementation for this object is substantially larger than if we implemented it ourselves. evg result

The much higher usage is normal, if you look at some of tests in memory_test.go you see that we do a diff of mem usages there because of the common objects that get initialized with a VM (and other init stuff). You can get an idea of it by looking at the toValue function where the init() function gets called on the objectGoMapSimple struct.

In this case to get a clear understanding of the mem usage for it you can look at my PR here to see how to test this out. I'd recommend instantiating the struct in the test, something like:

m := &objectGoMapSimple{ baseObject: baseObject{ val: &Object{runtime: vm}, }, data: origMap, }

and then possibly call the MemUsage(memCtx) function on it. This way you isolate it from anything around it and make sure your calculations are correct. If we end up lazy loading the objects we inject to the VM (console, services, ...) then all is good, otherwise we will need to subtract our objects mem usage from the overall calculation but this would be done in baas. We will have to check with the function engine improvements project to make sure. Let me know if that doesn't make sense!

object_gomap_test.go

Gabri3l

You won the lottery and get to write another MemUsage function! Thank you for looking into this and navigating Goja!

object_gomap_test.go

object_gomap.go

nathantfrank · 2023-08-14T15:39:11Z

object_gomap_test.go

+		{
+			name:      "should account for nested parsed map",
+			threshold: 100,
+			val: &objectGoMapSimple{
+				baseObject: baseObject{
+					val: &Object{runtime: vm},
+				},
+				data: map[string]interface{}{
+					"test": &objectGoMapSimple{ // <- this is treated as a objectGoMapReflect
+						baseObject: baseObject{
+							val: &Object{runtime: vm},
+						},
+						data: map[string]interface{}{
+							"subTest1": valueInt(99),
+							"subTest2": valueInt(99),
+							"subTest3": valueInt(99),
+							"subTest4": valueInt(99),
+						},
+					},
+				},
+			},
+			// overhead + len("test") + reflectedMap
+			expectedMem: SizeEmptyStruct + 4 + SizeEmptyStruct,
+			// overhead + len("test") with string overhead + reflectedMap
+			expectedNewMem: SizeEmptyStruct + 4 + SizeString + SizeEmptyStruct,
+			errExpected:    nil,
+		},


[Q] is this test case even possible/valid?

Not really, this type is only generated when as a goja user I call ToValue on a map[string]interface{} but the type is otherwise not exposed.

Forgot to add more to this, this test is still valid but only if instead of using a reference to objectGoMapSimple we use a reference to a map[string]interfeace{} that's more realistic to what a user could do. That's where it would get converted to objectGoMapReflect. If you look at the ToValue function, there's a switch case at the bottom that resolves to create a objectGoMapReflect item.

I think from the look of it that the memory usage of the entity pointed to should still be computed because the ToValue method extracts the element pointed to and instantiates an objectSomethingSomething based on its type. This will use memory.

I'm OK if you only want to focus on goMapSimple and goSlice with this PR and we can do the rest in a dedicated ticket.

nathantfrank · 2023-08-14T15:42:03Z

object_gomap_test.go

+		{
+			name:      "should account for nested key value pairs",
+			threshold: 100,
+			val: &objectGoMapSimple{
+				baseObject: baseObject{
+					val: &Object{runtime: vm},
+				},
+				data: map[string]interface{}{
+					"test": map[string]interface{}{ // is this ever valid/should we test this situation?
+						"subTest1": valueInt(99),
+						"subTest2": valueInt(99),
+						"subTest3": valueInt(99),
+						"subTest4": valueInt(99),
+					},
+				},
+			},
+			// overhead + len("test") + VM runtime base object + values
+			expectedMem: SizeEmptyStruct + 4 + 2204 + ((8 + SizeInt) * 4),
+			// overhead + len("testN") with string overhead + VM runtime base object with overhead + values with string overhead
+			expectedNewMem: SizeEmptyStruct + (4 + SizeString) + 4716 + ((8 + SizeString + SizeInt) * 4),
+			errExpected:    nil,
+		},


[Q] Same as above, is this test case even possible/valid?

Also just want to point out that when nested there is a significant size difference? Is this because there is like a duplicate vm/runtime instance. I tried following down the debug breakpoint rabbit hole, but definitely gets confusing with all the recursive calls.

I think this is a valid test case and I'm looking to see why there's so much additional memory.

Ah! The problem here is that when we loop through the "test" map, we rely on our MemUsage function to convert it to a value (_getStr calls ToValue), but if you look closely:

// ...other stuff inside ToValue case map[string]interface{}: if i == nil { return _null } obj := &Object{runtime: r} m := &objectGoMapSimple{ baseObject: baseObject{ val: obj, extensible: true, }, data: i, } obj.self = m m.init() // <-- uh oh return obj

There's a call to init which then sets the prototype to o.val.runtime.global.ObjectPrototype which is pretty large. That said this is totally fine, I'll review the rest and I think we should keep this test because this is a plausible scenario for a user to run into. We don't see this much in other tests because most of them are for goja types which do not need a conversion with ToValue.

nathantfrank · 2023-08-14T15:43:03Z

object_goslice_test.go

+		{
+			name:      "should account for objectGoSlice", // treated as a objectGoSliceReflect
+			threshold: 100,
+			val: &objectGoSlice{
+				baseObject: baseObject{
+					val: &Object{runtime: vm},
+				},
+				data: &[]interface{}{
+					&objectGoSlice{
+						baseObject: baseObject{
+							val: &Object{runtime: vm},
+						},
+						data: &[]interface{}{
+							valueInt(99),
+							valueInt(99),
+							valueInt(99),
+							valueInt(99),
+						},
+					},
+				},
+			},
+			// overhead + nested overhead
+			expectedMem: SizeEmptyStruct + SizeEmptyStruct,
+			// overhead + nested overhead
+			expectedNewMem: SizeEmptyStruct + SizeEmptyStruct,
+			errExpected:    nil,
+		},


[Q] Similar to the map Q of nested pre-parsed objects.

This is also similar and I would use a pointer to a slice of interfaces rather than the internal objectGoSlice type. Again this test is correct but if we use a pointer it better represent what a user could do (where the user is us in baas eheh)

nathantfrank · 2023-08-14T15:43:25Z

object_goslice_test.go

+		{
+			name:      "should account for nested slices",
+			threshold: 100,
+			val: &objectGoSlice{
+				baseObject: baseObject{
+					val: &Object{runtime: vm},
+				},
+				data: &[]interface{}{
+					[]interface{}{
+						valueInt(99),
+						valueInt(99),
+						valueInt(99),
+						valueInt(99),
+					},
+				},
+			},
+			// overhead + (value + len("length") + "length".value + prototype + ints)
+			expectedMem: SizeEmptyStruct + (SizeEmptyStruct + 6 + SizeEmptyStruct + (SizeEmptyStruct + SizeEmptyStruct) + SizeNumber*4),
+			// overhead + (value + len("length") with string overhead + "length".value + prototype + ints)
+			expectedNewMem: SizeEmptyStruct + (SizeEmptyStruct + (6 + SizeString) + SizeEmptyStruct + (SizeEmptyStruct + SizeEmptyStruct) + SizeNumber*4),
+			errExpected:    nil,
+		},


[Q] Similar Q to the nested maps.

This is valid test case for me.

Yeah this is a possible scenario but I would only go one layer down like you did here.

nathantfrank · 2023-08-18T16:13:16Z

@Gabri3l mind leaving a comment where you are at with investigating the answer to my questions?

Gabri3l · 2023-08-18T20:35:40Z

@Gabri3l mind leaving a comment where you are at with investigating the answer to my questions?

Adding a few more people to the PR as I'm catching up with a few things. I'll try and dive into this as soon as I can.

LijieZhang1998 · 2023-08-24T18:17:27Z

object_gomap_test.go

+	// the baseObject is quite large when ToValue is called due to the functions on the object
+	// calculating ahead of time for test case
+	nestedMapAsObject := vm.ToValue(nestedMap)
+	nestedMapMemUsage, nestedMapNewMemUsage, err := nestedMapAsObject.MemUsage(vmCtx)


This is nice!!

Gabri3l

Thank you for tackling this, it's more challenging than what it appeared. Left a few comments to get the ball rolling!

object_gomap_test.go

Gabri3l · 2023-08-24T19:09:03Z

object_gomap_test.go

+		{
+			name:      "should account for nested reflect object",
+			threshold: 100,
+			val: &objectGoMapSimple{
+				baseObject: baseObject{
+					val: &Object{runtime: vm},
+				},
+				data: map[string]interface{}{
+					"test": &objectGoMapSimple{
+						baseObject: baseObject{
+							val: &Object{runtime: vm},
+						},
+						data: map[string]interface{}{
+							"subTest1": valueInt(99),
+							"subTest2": valueInt(99),
+							"subTest3": valueInt(99),
+							"subTest4": valueInt(99),
+						},
+					},
+				},
+			},
+			// overhead + len("test") + reflectObject
+			expectedMem: SizeEmptyStruct + 4 + SizeEmptyStruct,
+			// overhead + len("test") with string overhead + reflectObject
+			expectedNewMem: SizeEmptyStruct + 4 + SizeString + SizeEmptyStruct,
+			errExpected:    nil,
+		},


[minor] Kind of like I mentioned in the answers to your questions I would convert this to have "test": &nestedMap

object_goslice_test.go

Gabri3l · 2023-08-24T19:13:50Z

object_goslice_test.go

+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			total, newTotal, err := tc.val.MemUsage(NewMemUsageContext(vm, 100, 100, 100, tc.threshold, nil))


[minor] IF you feel adventurous and want to add estimates to the objectGoSlice mem usage functions then you would use the threshold on the array and not on the object props like you did here. Otherwise you can just remove the threshold field in the test struct altogether and hardcode it.

Suggested change

total, newTotal, err := tc.val.MemUsage(NewMemUsageContext(vm, 100, 100, 100, tc.threshold, nil))

total, newTotal, err := tc.val.MemUsage(NewMemUsageContext(vm, 100, 100, tc.threshold, 100, nil))

arahmanan · 2023-08-25T16:34:29Z

object_goslice.go

+		return 0, 0, err
+	}
+	for _, datum := range *o.data {
+		memValue, newMemValue, err := o.val.runtime.ToValue(datum).MemUsage(ctx)


[q] this could be an expensive operation for large arrays. Should we estimate the usage instead? Example here. Same question for the objectGoMapSimple.

Nathan just filed https://jira.mongodb.org/browse/BAAS-24348 which I will pick up pretty much right away!

Gabri3l

I'd still like for the two unresolved changes to be made, then we're good to go!

Gabri3l

Great work digging into this, thank you so much!

nathantfrank requested a review from Gabri3l July 31, 2023 17:04

Gabri3l requested changes Aug 1, 2023

View reviewed changes

nathantfrank force-pushed the realm branch from 13161c0 to 0635576 Compare August 10, 2023 17:40

nathantfrank commented Aug 10, 2023

View reviewed changes

object_gomap_test.go Outdated Show resolved Hide resolved

Gabri3l requested changes Aug 11, 2023

View reviewed changes

object_gomap_test.go Outdated Show resolved Hide resolved

object_gomap.go Outdated Show resolved Hide resolved

nathantfrank commented Aug 14, 2023

View reviewed changes

Gabri3l requested review from kpatel71716 and LijieZhang1998 August 18, 2023 20:34

nathantfrank added 6 commits August 24, 2023 13:07

feature: capture memory used by objectGoMapSimple

805a1a0

chore: initial test example

e84d2c1

feature: add MemUsage to goMapSimple

be2e914

chore: move test and update

4ec1f54

chore: add mem usage for go slices and testing

34da410

chore: update test descriptions and calculation

41c1780

nathantfrank force-pushed the realm branch from e885c4a to 41c1780 Compare August 24, 2023 18:07

LijieZhang1998 approved these changes Aug 24, 2023

View reviewed changes

Gabri3l requested changes Aug 24, 2023

View reviewed changes

chore: pr feedback for tests

ad9d1a9

arahmanan reviewed Aug 25, 2023

View reviewed changes

chore: prevent nil pointer panic

2236f93

nathantfrank requested a review from Gabri3l August 28, 2023 14:42

Gabri3l reviewed Aug 29, 2023

View reviewed changes

chore: remove thresholds from test cases and add nested pointer map

eb9c967

Gabri3l approved these changes Aug 29, 2023

View reviewed changes

nathantfrank changed the title ~~BAAS-22214: capture memory used by objectGoMapSimple~~ BAAS-22214: capture memory used by objectGoMapSimple and objectGoSlice Aug 29, 2023

nathantfrank merged commit fcdf373 into mongodb-forks:realm Aug 29, 2023
0 of 4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BAAS-22214: capture memory used by objectGoMapSimple and objectGoSlice #88

BAAS-22214: capture memory used by objectGoMapSimple and objectGoSlice #88

nathantfrank commented Jul 31, 2023

Gabri3l Aug 1, 2023 •

edited

Loading

nathantfrank Aug 1, 2023

nathantfrank Aug 2, 2023 •

edited

Loading

nathantfrank Aug 2, 2023

nathantfrank Aug 3, 2023

Gabri3l Aug 7, 2023

Gabri3l left a comment

nathantfrank Aug 14, 2023

Gabri3l Aug 24, 2023 •

edited

Loading

Gabri3l Aug 24, 2023

nathantfrank Aug 14, 2023

Gabri3l Aug 24, 2023

Gabri3l Aug 24, 2023

nathantfrank Aug 14, 2023

Gabri3l Aug 24, 2023

nathantfrank Aug 14, 2023

LijieZhang1998 Aug 24, 2023

Gabri3l Aug 24, 2023

nathantfrank commented Aug 18, 2023

Gabri3l commented Aug 18, 2023

LijieZhang1998 Aug 24, 2023

Gabri3l left a comment

Gabri3l Aug 24, 2023

Gabri3l Aug 24, 2023

arahmanan Aug 25, 2023

Gabri3l Aug 25, 2023

Gabri3l left a comment

Gabri3l left a comment

	total, newTotal, err := tc.val.MemUsage(NewMemUsageContext(vm, 100, 100, 100, tc.threshold, nil))
	total, newTotal, err := tc.val.MemUsage(NewMemUsageContext(vm, 100, 100, tc.threshold, 100, nil))

BAAS-22214: capture memory used by objectGoMapSimple and objectGoSlice #88

BAAS-22214: capture memory used by objectGoMapSimple and objectGoSlice #88

Conversation

nathantfrank commented Jul 31, 2023

Gabri3l Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathantfrank Aug 2, 2023 • 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

Gabri3l left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gabri3l Aug 24, 2023 • 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

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

nathantfrank commented Aug 18, 2023

Gabri3l commented Aug 18, 2023

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

Choose a reason for hiding this comment

Gabri3l left a comment

Choose a reason for hiding this comment

Gabri3l left a comment

Choose a reason for hiding this comment

Gabri3l Aug 1, 2023 •

edited

Loading

nathantfrank Aug 2, 2023 •

edited

Loading

Gabri3l Aug 24, 2023 •

edited

Loading