Conversation
Signed-off-by: John Plevyak <jplevyak@gmail.com>
|
Could you remove |
| } | ||
|
|
||
| uint32_t Context::defineMetric(MetricType type, absl::string_view name) { | ||
| auto stat_name = wasm_->stat_name_set_.getStatName(name); |
There was a problem hiding this comment.
Generally looks correct. If you know a priori some stat-names you want to use, even if it's the context of concatenating them with other stat-names, you may want to have an interface to 'remember' them into the StatNameSet to avoid taking locks here.
I think to do this you'd need to carve out more of your integer space below for names that could be explicitly declared and saved by WASM code.
The current interface you provide here where you take a string will definitely wind up taking a lock. Not sure how important that is for wasm performance.
There was a problem hiding this comment.
I'd rather avoid the lock or share the set in some way. In the normal case we are going to have 1 wasm VM per thread/silo and they will be registering the same stats but the stats are not known a priori (this is a dynamic plugin system).
So either std::shared_ptr over a shared StatNameSet or I reimplement it without a lock (since the VMs are always single threaded).
Alternatively I can have a separate interface for pre-registering strings for stats. That would add a good deal more complexity. We are already planning making tags first class entities. It seems like a string registration facility would fit well in that model.
However, in the short term I would like to just move to the new stat system so perhaps this code is a good interm step.
There was a problem hiding this comment.
Sharing or replicating the StatNameSet is not going to help, because there will be locks on the underlying symbol table.
I'm not sure exactly what it means to have tags be first class entities, but that sounds promising :) As long as you are not converting string_view to StatName on the hot-path it should be fine.
There was a problem hiding this comment.
Sorry, reflecting on my statement above, it is incorrect :)
Replicating a StatNameSet per thread will indeed eliminate global symbol-lock contention (once the system is warm, but at the cost of a lot of memory. The StatName/SymbolTable infrastructure is designed to avoid keeping elaborated stat names in memory, and StatNameSet defeats that. It was introduced for a few specific use-cases where some of the tokens used to compose stat-names are discovered only in the request, but are expected to be small in number.
There was a problem hiding this comment.
So this PR will work but it no optimal in memory use. That is fine for now since we will have to change the way WebAssembly modules are initialized in order to take advantage of per-registration of stats/symbols and we would like to avoid the global locking.
There was a problem hiding this comment.
Add TODO(jplevyak): ... to all those calls to replace it with something better in the future?
There was a problem hiding this comment.
Added a comment block.
Signed-off-by: John Plevyak <jplevyak@gmail.com>
Signed-off-by: John Plevyak <jplevyak@gmail.com>
Signed-off-by: John Plevyak <jplevyak@gmail.com>
Cherry-pick envoyproxy/envoy#8803 and add type to WasmState and make it serializable. Signed-off-by: Lizan Zhou <lizan@tetrate.io>
3 participants
Signed-off-by: John Plevyak jplevyak@gmail.com
For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md
Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]