Skip to content

BAAS-35403 add lock to runtime maxStackObjectMemUsage#133

Merged
kpatel71716 merged 1 commit into
mongodb-forks:realmfrom
kpatel71716:BAAS-35403_lock
Apr 14, 2025
Merged

BAAS-35403 add lock to runtime maxStackObjectMemUsage#133
kpatel71716 merged 1 commit into
mongodb-forks:realmfrom
kpatel71716:BAAS-35403_lock

Conversation

@kpatel71716
Copy link
Copy Markdown

No description provided.

Comment thread runtime.go

func (r *Runtime) setMaxStackObjectMemUsage(mem uint64) {
r.maxStackObjectMemLock.Lock()
r.maxStackObjectMem = mem
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic here is a bit different, not sure it makes a difference (e.g. not using math.Max)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, just using this as a setter here! And used a logical check over Math.max elsewhere to reduce the number of times we call into this to get the lock

Copy link
Copy Markdown

@Gabri3l Gabri3l left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides one comment this looks good, hopefully no/low perf impact.

Copy link
Copy Markdown

@LijieZhang1998 LijieZhang1998 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kpatel71716
Copy link
Copy Markdown
Author

Besides one comment this looks good, hopefully no/low perf impact.

Yep good callout. I ran the benchmarks and the impact of the lock was negligible here so that's good

@kpatel71716 kpatel71716 merged commit b67c240 into mongodb-forks:realm Apr 14, 2025
2 of 6 checks passed
@kpatel71716 kpatel71716 deleted the BAAS-35403_lock branch April 14, 2025 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants