Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lazily allocate faked ctx variables #2310

Closed
wants to merge 1 commit into from

Conversation

emcfarlane
Copy link
Contributor

This avoids NewContext creating all ctx variables. Getters will lazy initialize as needed. The biggest cultprit is the call to NewFakeRandSource() which allocates a rand.NewSource that has a 607 width array. Small reduction in memory for unused context methods.

This avoids init creating all ctx variables. On get it will lazy
initialize as needed. The biggest cultprit is the call to rand.New which
allocates a new array per instance. Small perf improvement.

Signed-off-by: Edward McFarlane <emcfarlane@buf.build>
@mathetake
Copy link
Member

i think this could be problematic when threads enabled. cc @anuraaga

@anuraaga
Copy link
Contributor

anuraaga commented Sep 6, 2024

Yeah, we don't do any syscalls in the threads tests we have, perhaps we should. If we did, I think the race detector would call out these mutations if we don't use atomic to write them.

IIUC, it doesn't seem difficult for a user to provide lazy versions or even no-op versions when known they're not needed. For random specifically, in fact, it seems to not happen if passing rand.Reader which I think is common/required for applications that need random, and would otherwise be allocation-free for applications that don't need random. So perhaps this optimization isn't needed?

@emcfarlane
Copy link
Contributor Author

Reading from RandSource() is not safe for concurrent use so presumed it must not be called concurrently. How would the getter be accessed in a concurrent way? Saying that happy to close! Only noticed on a microbenchmarks where the largest allocations are from this.

@anuraaga
Copy link
Contributor

anuraaga commented Sep 6, 2024

Reading from RandSource() is not safe for concurrent use so presumed it must not be called concurrently.

AFAIK, the default rand source (used by the free functions in the rand package) is safe for concurrent use, and it's only constructed ones that aren't, so rand.Reader would be fine for concurrent use while also providing proper random numbers. Just as a reference, I always pass it "by default" when working with arbitrary wasm when I don't know if it will be used or not.

https://github.com/wasilibs/go-protoc-gen-builtins/blob/main/internal/runner/runner.go#L42

@emcfarlane
Copy link
Contributor Author

Closing! Need to think through the concurrency model.

@emcfarlane emcfarlane closed this Oct 3, 2024
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