BAAS-23396: add strings overhead to mem usage calculations #86

Gabri3l · 2023-07-24T21:13:10Z

This is a small ma pretty important change. Strings in go have an overhead of 16bytes, currently an empty string adds up to 0 bytes from the way we calculate memory usage which is not correct. With this change every native string or object key (which are also strings) will also account for the overhead defined in SizeString.

EDIT

To understand the impact of this change all MemUsage functions return the existing value + an adjusted one. This will help us understand the impact of such a change before we roll it out to customers. I split the commit logically for easier review but beware that the whole test suite only passes with the last commits.

Original commit that has been already reviewed
Update the interfaces to the new MemUsage signature
Update every single MemUsage function to use the new signature + add tests
A small refactor based on Nick's comment

Generally I noticed a few missing nil checks and other incorrect MemUsage functions, the diff should be easy to spot from the single commits.

LijieZhang1998

What a wonderful find! I only have one question but not really related to your change.

memory_test.go

object.go

memory_test.go

jwongmongodb

LGTM! nothing else to add on my side. Great find!

kpatel71716

LGTM! 🤑

LijieZhang1998

LGTM 🥇 I just have one question about the MemUsageLimitExceeded check.

LijieZhang1998 · 2023-08-07T15:24:21Z

array.go

 		}
-		if exceeded := ctx.MemUsageLimitExceeded(total); exceeded {
-			return total, nil
+		if exceeded := ctx.MemUsageLimitExceeded(memUsage); exceeded {


[q] should we use mewMemUsage instead of memUsage?

Good question, eventually the newMem and mem will have the same value but we are using this PR to collect data about the gap between the two values for customers to get an idea of the impact of this change. Until we apply this new changes all the memory logic will rely on the current memUsage.

value.go

nickpoindexter

LGTM! Just had some questions and optional things.

array.go

builtin_map.go

array_test.go

date.go

mem_context.go

string_ascii_test.go

string_imported_test.go

nickpoindexter · 2023-08-07T17:35:55Z

string_unicode_test.go

+		{
+			name:        "should have a value given the length of the string",
+			val:         unicodeString{' ', 'y', 'o'},
+			expected:    2,


[q] why 2 instead of 3? Does space not count?

When this gets converted to a string (see the String() method on this type) we skip the first item which can represent the little-indian/big-indian convention but not the actual content of the string. Not 100% that's what the first char is used for but I know that even comparisons between strings skip the fist char if it's a unicode string.

jwongmongodb

LGTM!

string_test.go

builtin_map.go

runtime.go

nickpoindexter

LGTM! Will be interesting to see the results for customers

jwongmongodb

LGTM!

jwongmongodb · 2023-08-08T18:12:49Z

memory_test.go

 			nv = new MyNativeVal()
 			checkMem();`,
-			testNativeValueMemUsage + 2,
+			expectedSizeDiff: testNativeValueMemUsage +
+				2, // "nv"


[q] what does nv mean here? Native Value?

this is for the variable name nv, so it's the len("nv")

kpatel71716

LGTM!

…as separate mem usage value (#86)

Gabri3l requested review from kpatel71716, nickpoindexter, LijieZhang1998 and jwongmongodb July 28, 2023 20:16

LijieZhang1998 approved these changes Jul 28, 2023

View reviewed changes

memory_test.go Outdated Show resolved Hide resolved

nickpoindexter approved these changes Jul 30, 2023

View reviewed changes

object.go Outdated Show resolved Hide resolved

memory_test.go Outdated Show resolved Hide resolved

jwongmongodb approved these changes Jul 31, 2023

View reviewed changes

kpatel71716 approved these changes Jul 31, 2023

View reviewed changes

Gabri3l force-pushed the BAAS-23396 branch 2 times, most recently from 35e28b0 to 78f13e3 Compare August 1, 2023 18:49

Gabri3l requested review from kpatel71716, nickpoindexter, jwongmongodb and LijieZhang1998 August 4, 2023 16:33

LijieZhang1998 approved these changes Aug 7, 2023

View reviewed changes

Gabri3l commented Aug 7, 2023

View reviewed changes

value.go Outdated Show resolved Hide resolved

nickpoindexter approved these changes Aug 7, 2023

View reviewed changes

jwongmongodb approved these changes Aug 7, 2023

View reviewed changes

string_test.go Outdated Show resolved Hide resolved

builtin_map.go Outdated Show resolved Hide resolved

runtime.go Outdated Show resolved Hide resolved

Gabri3l added 12 commits August 8, 2023 09:31

BAAS-23396: add strings overhead to mem usage calculations

9dc4a8a

feat: update MemUsge signature to include new tracked value

26c164d

feat: return dual value for mem usage in added_values

bc3d9ae

feat: return dual value for mem usage in stringObject

088cc45

feat: return dual value for mem usage in array

1a40557

feat: return dual value for mem usage in sparse_array

7f9a405

feat: return dual value for mem usage in builtin_map

24afd12

feat: return dual value for mem usage in nativeProxyHandler

9658848

feat: return dual value for mem usage in program

956d42d

feat: return dual value for mem usage in dateObject

b1556d2

feat: return dual value for mem usage in destructKeyedSource

2604585

feat: return dual value for mem usage in func

44ed025

Gabri3l added 17 commits August 8, 2023 09:31

feat: return dual value for mem usage in string_*

5ee274f

feat: return dual value for mem usage in object

d6cbb39

feat: return dual value for mem usage in object_dyamic

1071e79

feat: return dual value for mem usage in object_lazy

759f845

feat: return dual value for mem usage in proxy

f7a932c

feat: return dual value for mem usage in typedarrays

d9baeac

feat: return dual value for mem usage in runtime

d6c8890

feat: return dual value for mem usage in vm

6b8a3d5

feat: return dual value for mem usage in value

8dc3a6c

feat: return dual value for mem usage in stash

d9030ba

refactor: update memory test to account for dual value

93144e6

refactor: extract mem usage estimate computation

39cd477

docs: update comment on SizeString

d8582a4

refactor: rename SizeEmpty to SizeEmptyStruct

12cb93f

refactor: ensure all test structs are named and consistent

e769d6a

refactor: fix mem usage returns, update estimation functions

e71d678

feat: add empty slice overhead for arrayObject

3e71d66

Gabri3l force-pushed the BAAS-23396 branch from 8ca0545 to 3e71d66 Compare August 8, 2023 14:31

Gabri3l requested review from jwongmongodb and nickpoindexter August 8, 2023 14:32

nickpoindexter approved these changes Aug 8, 2023

View reviewed changes

jwongmongodb approved these changes Aug 8, 2023

View reviewed changes

kpatel71716 approved these changes Aug 8, 2023

View reviewed changes

Gabri3l merged commit 1e62d92 into mongodb-forks:realm Aug 8, 2023
0 of 4 checks passed

Gabri3l deleted the BAAS-23396 branch August 8, 2023 19:29

andrewzlchen mentioned this pull request Aug 10, 2023

BAAS-23935: Lazyload object prototype functions and Object namespace #94

Closed

Gabri3l added a commit that referenced this pull request Aug 30, 2023

BAAS-23396: add string and slice overheaed to mem usage calculations …

f9192da

…as separate mem usage value (#86)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BAAS-23396: add strings overhead to mem usage calculations #86

BAAS-23396: add strings overhead to mem usage calculations #86

Gabri3l commented Jul 24, 2023 •

edited

Loading

LijieZhang1998 left a comment

jwongmongodb left a comment

kpatel71716 left a comment

LijieZhang1998 left a comment

LijieZhang1998 Aug 7, 2023

Gabri3l Aug 7, 2023

nickpoindexter left a comment

nickpoindexter Aug 7, 2023

Gabri3l Aug 7, 2023

jwongmongodb left a comment

nickpoindexter left a comment

jwongmongodb left a comment

jwongmongodb Aug 8, 2023

Gabri3l Aug 8, 2023

kpatel71716 left a comment

BAAS-23396: add strings overhead to mem usage calculations #86

BAAS-23396: add strings overhead to mem usage calculations #86

Conversation

Gabri3l commented Jul 24, 2023 • edited Loading

LijieZhang1998 left a comment

Choose a reason for hiding this comment

jwongmongodb left a comment

Choose a reason for hiding this comment

kpatel71716 left a comment

Choose a reason for hiding this comment

LijieZhang1998 left a comment

Choose a reason for hiding this comment

LijieZhang1998 Aug 7, 2023

Choose a reason for hiding this comment

Gabri3l Aug 7, 2023

Choose a reason for hiding this comment

nickpoindexter left a comment

Choose a reason for hiding this comment

nickpoindexter Aug 7, 2023

Choose a reason for hiding this comment

Gabri3l Aug 7, 2023

Choose a reason for hiding this comment

jwongmongodb left a comment

Choose a reason for hiding this comment

nickpoindexter left a comment

Choose a reason for hiding this comment

jwongmongodb left a comment

Choose a reason for hiding this comment

jwongmongodb Aug 8, 2023

Choose a reason for hiding this comment

Gabri3l Aug 8, 2023

Choose a reason for hiding this comment

kpatel71716 left a comment

Choose a reason for hiding this comment

Gabri3l commented Jul 24, 2023 •

edited

Loading