search

envoyproxy / envoy

Cloud-native high-performance edge/middle/service proxy
https://www.envoyproxy.io
Apache License 2.0
25.12k stars 4.82k forks source link

split the wasm Context implementation to different class #36907

Open wbpcode opened 1 month ago

wbpcode commented 1 month ago

Title: split the wasm Context implementation to different class

In the WASM implementation, the root context, the http filter context, the network filter context share same class Context. It make the code hard to read or to maintain.

It would be better to split the Context to different class like RootContext, HttpContext, NetworkContext, etc.

wbpcode commented 1 month ago

WDYT cc @mpwarres cc @kyessenov

kyessenov commented 4 weeks ago

Makes sense to me to have a base and subclasses if that is cleaner. I think it was done this way to reduce allocations so we shouldn't lose that.

wbpcode commented 4 weeks ago

Yeah, if there are shared part, then it would be great be a common base class. But anyway, they acutually are different abstractions and shouldn't share same implemention. (esp the root context and the stream context)

I think it was done this way to reduce allocations so we shouldn't lose that.

I didn't get this. What you mean by reduce allocations? I think it's weird for a HttpContext to contains network filter's callback or state. It acutally bring additional unnecessary memory allocations.

Another reason I think the refactoring make sense is because the hybrid Context make it's hard to read and to maintain. An example is the pending external http calls management (http_request_, etc). It's hard to get it's part of RootContext if we don't read the ABI code carefully.

kyessenov commented 4 weeks ago

I wasn't the author - but this shared context is effectively a global, and globals help with reducing temporaries - that's just my best guess why it's done this way.

IMO the ABI should have really focused on HTTP calls. Network filter and bootstrap extensions should have been an Envoy specific ABI, but I guess it was slapped on later over HTTP context.