search

proxy-wasm / proxy-wasm-cpp-host

WebAssembly for Proxies (C++ host implementation)
Apache License 2.0
84 stars 69 forks source link

Cache canary results. #351

Closed kyessenov closed 1 year ago

kyessenov commented 1 year ago

Fixes #350

PiotrSikora commented 1 year ago

@ingwonsong @mpwarres @martijneken could you take a look? Thanks!

ingwonsong commented 1 year ago

IIUC, with this PR, we don't make a canary for each VMs. (just doing canary for each plugin). Would there be any chance to use heterogenous VMs with the same Plugin? (I don't think this situation is realistic, but just want to check your thoughts.)

kyessenov commented 1 year ago

Can you elaborate on what you mean by a heterogeneous VM? I think the behavior should be the same with or without this PR since we simply cache the result of a computation, assuming plugin_key uniquely identifies an instance.

In the future, this needs to be completely changed so that we only configure once on the main instead of the thread-locals. But given that we have a separate thread local cache already, I think we can delay that.

ingwonsong commented 1 year ago

I meant multiple types of wasm vm engines with "heterogenous" Wasm VM. BTW, I agree that it would be very rare case, and even if it happen, the difference of VM engine should not affect the result of canary.

LGTM.

ingwonsong commented 1 year ago

After having more chat with @kyessenov, I understood that the canary is cached per WasmHandle, so even though there are multiple types of VM engines, each of them will take canary tests.

kyessenov commented 1 year ago

@PiotrSikora main merge broke CLA :)

PiotrSikora commented 1 year ago

@PiotrSikora main merge broke CLA :)

Argh, I don't know why GitHub UI decided to use my @google.com email instead of the primary one.

Could you remove my commit and push your own merge?

kyessenov commented 1 year ago

@PiotrSikora done and force-pushed.