Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
wasm: allow execution of multiple instances of the same plugin. #13753
wasm: allow execution of multiple instances of the same plugin. #13753
Changes from 2 commits
697dc19
672c1ff
8d49c1d
9f98a40
60003c1
6f3392a
33f8b48
375b02d
8ad9cbd
3a89a49
2facb41
4c0ed76
88d5dc0
40015d0
e94fd82
6d978fc
62a015e
487d930
930282d
cd04f3b
9965dcd
f16ba7d
3d4e28e
d128aaf
7bc7c9a
e15e498
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you merge main to pick up @jmarantz changes on this? We have the typed wrappers now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, although it doesn't look like the typed wrappers support storing
nullptr
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct. having nullptr as a possibility was not a requirement for any of the existing uses, unless you are not storing a value at all. Is that a problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you added a layer of indirection through a handle which seems like a waste. We could change runOnAllThreads send in a typed pointer allowing nullptr or absl::optional ref or add a couple of new variants of runOnAllThreads. @PiotrSikora I'm not sure what the use-case is though for allowing null. Wouldn't you want to always set() your tls container on startup?
@mattklein123 WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like allowing storing nullptr in the slot should be OK? Should we just fix that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah my preference would be for not having new variants. Maybe I'm missing something but can't we just make it so that shared_ptr that is stored can be null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shared_ptr can be stored as null, but the call to asType() in the template wrapper does a dynamic cast and ASSERT. That's easy to fix but then we have to pass the thing as something that can be checked for null
T*
orabsl::optional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Yeah I would probably just do optional shared_ptr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry optional reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#13883 prototypes that change. Would like to get some quick feedback on the direction prior to filling out comments and adding a unit test for new thin abstraction
struct OptRef
.