[Merged by Bors] - feat: introduce `ByteBuf` for `SmartModuleSpec` #2738

EstebanBorai · 2022-10-22T04:49:13Z

In order to have a more performant implementation on WASM bytes, a
dedicated type is introduced which provides an specific implementation
for bytes representing a WASM file owned by a SmartModule instance.

Resolves: #2700

crates/fluvio-controlplane-metadata/src/smartmodule/spec.rs

crates/fluvio-protocol/src/core/mod.rs

crates/fluvio-protocol/src/core/wasm_bytes.rs

sehz

Much better. Couple of issues remain

crates/fluvio-protocol/src/core/wasm_bytes.rs

crates/fluvio-protocol/src/core/bytebuf.rs

EstebanBorai · 2022-10-28T23:08:38Z

Implemented tests to make sure the output is the same as for encoding/decoding from Vec<u8>.

crates/fluvio-protocol/src/core/bytebuf.rs

morenol · 2022-11-04T01:24:53Z

@sehz @morenol new implementation is looking very good!
test vecu8 encoding ... bench:      414738 ns/iter (+/- 29440)

test vecu8 decoding ... bench:      355882 ns/iter (+/- 11905)

test bytebuf encoding ... bench:        7909 ns/iter (+/- 235)

test bytebuf decoding ... bench:        7684 ns/iter (+/- 293)
These are the results benchmarking with Criterion as of b8f77a5

Very promising, I think that we should use ByteBuf in SmartModuleInput and maybe other places of the code where we use Vec<u8> and have Encode/Decode

EstebanBorai · 2022-11-04T01:28:29Z

@sehz @morenol new implementation is looking very good!
test vecu8 encoding ... bench:      414738 ns/iter (+/- 29440)

test vecu8 decoding ... bench:      355882 ns/iter (+/- 11905)

test bytebuf encoding ... bench:        7909 ns/iter (+/- 235)

test bytebuf decoding ... bench:        7684 ns/iter (+/- 293)
These are the results benchmarking with Criterion as of b8f77a5
Very promising, I think that we should use ByteBuf in SmartModuleInput and maybe other places of the code where we use Vec

Totally agree!

morenol · 2022-11-04T01:56:47Z

I think that we could also grab Encoder implementation from this PR and add that to RecordData

fluvio/crates/fluvio-protocol/src/record/data.rs

Line 169 in 3d5f0ec

impl Encoder for RecordData {

EstebanBorai · 2022-11-04T01:57:06Z

that's 50x improvement. Can you benchmark with a bytes buffer with 100k, which is typical for SmartModule?

Im using a SmartModule WASM file for testing. Here is the file in question: https://github.com/EstebanBorai/fluvio/blob/feat/wasmbytes-for-smartmodulespec/crates/fluvio-protocol/fixtures/smartmodule.wasm.

The source for this file is actually a filter SmartModule example. Perhaps we could try with the RegExp example for a more "real-life" example?

sehz · 2022-11-04T02:03:50Z

That's good enough!

sehz

Great progress and very nice work. Just need couple of refinements

crates/fluvio-controlplane-metadata/Cargo.toml

crates/fluvio-protocol/src/core/bytebuf.rs

morenol

Nice improvement, it looks good to me just a couple of nits.

I see that CI test that checks crate version reports crates that need to be updated but it is not failing, maybe is it an issue with the exit code? CC @tjtelan

https://github.com/infinyon/fluvio/actions/runs/3395995671/jobs/5646561976

crates/fluvio-protocol/src/core/bytebuf.rs

crates/fluvio-controlplane-metadata/src/smartmodule/spec.rs

sehz

few remaining nits

sehz · 2022-11-04T22:17:05Z

crates/fluvio-controlplane-metadata/src/smartmodule/spec.rs

@@ -109,7 +110,7 @@ pub struct SmartModuleWasmSummary {
 pub struct SmartModuleWasm {
    pub format: SmartModuleWasmFormat,
    #[cfg_attr(feature = "use_serde", serde(with = "base64"))]
-    pub payload: Vec<u8>,
+    pub payload: ByteBuf,


let's remove pub signature, we don't need to expose it

The fluvio-sc-schema uses the field:

fluvio/crates/fluvio-sc-schema/src/smartmodule/mod.rs

Line 21 in 6c57ed0

wasm_length: self.wasm.payload.len() as u32,

I think we could expose it via method call (similar to the getter pattern)?

we could create a fn payload_len

Its good for this use case, but we also consume the payload directly for the SmartModuleInvocationWasm enum here:

error[E0616]: field `payload` of struct `SmartModuleWasm` is private --> crates/fluvio-spu/src/smartengine/context.rs:253:78 | 253 | wasm: SmartModuleInvocationWasm::AdHoc(smartmodule.spec.wasm.payload.into()),

I think we could think of an API for these use case on a next PR? In order to encapsulate these fields, and
also provide some unit tests to these.

crates/fluvio-protocol/Cargo.toml

crates/fluvio-controlplane-metadata/Cargo.toml

crates/fluvio-protocol/src/core/bytebuf.rs

sehz

Very close. Last nit

crates/fluvio-sc-schema/Cargo.toml

sehz

Nice work. Tremendous improvement. Ship it!

EstebanBorai · 2022-11-04T23:27:18Z

bors r+

In order to have a more performant implementation on WASM bytes, a dedicated type is introduced which provides an specific implementation for bytes representing a WASM file owned by a `SmartModule` instance. Resolves: #2700

EstebanBorai · 2022-11-04T23:27:48Z

Nice work. Tremendous improvement. Ship it!

Thanks! And to everyone involved!

bors · 2022-11-05T00:06:27Z

Pull request successfully merged into master.

Build succeeded:

Done

EstebanBorai force-pushed the feat/wasmbytes-for-smartmodulespec branch from 7d72265 to 31b3a93 Compare October 22, 2022 05:42

EstebanBorai commented Oct 22, 2022

View reviewed changes

crates/fluvio-controlplane-metadata/src/smartmodule/spec.rs Outdated Show resolved Hide resolved

EstebanBorai commented Oct 22, 2022

View reviewed changes

crates/fluvio-controlplane-metadata/src/smartmodule/spec.rs Outdated Show resolved Hide resolved

EstebanBorai marked this pull request as ready for review October 22, 2022 15:59

EstebanBorai requested a review from sehz October 22, 2022 16:08

sehz requested changes Oct 22, 2022

View reviewed changes

EstebanBorai force-pushed the feat/wasmbytes-for-smartmodulespec branch 2 times, most recently from e405058 to c92bb75 Compare October 22, 2022 19:48

EstebanBorai commented Oct 22, 2022

View reviewed changes

crates/fluvio-protocol/src/core/mod.rs Outdated Show resolved Hide resolved

EstebanBorai commented Oct 22, 2022

View reviewed changes

crates/fluvio-protocol/src/core/wasm_bytes.rs Outdated Show resolved Hide resolved

EstebanBorai requested a review from sehz October 22, 2022 20:07

sehz requested changes Oct 22, 2022

View reviewed changes

crates/fluvio-protocol/src/core/wasm_bytes.rs Outdated Show resolved Hide resolved

crates/fluvio-protocol/src/core/wasm_bytes.rs Outdated Show resolved Hide resolved

EstebanBorai changed the title ~~feat: introduce WasmBytes for SmartModuleSpec~~ feat: introduce ByteBuf for SmartModuleSpec Oct 22, 2022

EstebanBorai force-pushed the feat/wasmbytes-for-smartmodulespec branch 4 times, most recently from 2e6cbb4 to 7594c1d Compare October 23, 2022 04:18

EstebanBorai commented Oct 23, 2022

View reviewed changes

crates/fluvio-protocol/src/core/bytebuf.rs Outdated Show resolved Hide resolved

EstebanBorai force-pushed the feat/wasmbytes-for-smartmodulespec branch from 72c11e3 to 2199aec Compare October 24, 2022 14:39

EstebanBorai requested a review from sehz October 24, 2022 14:47

EstebanBorai force-pushed the feat/wasmbytes-for-smartmodulespec branch from 2199aec to 42ccaeb Compare October 25, 2022 21:17

EstebanBorai marked this pull request as draft October 25, 2022 21:17

EstebanBorai force-pushed the feat/wasmbytes-for-smartmodulespec branch 5 times, most recently from 44ab15b to 0e067f4 Compare October 27, 2022 20:00

EstebanBorai commented Oct 28, 2022

View reviewed changes

crates/fluvio-protocol/src/core/bytebuf.rs Show resolved Hide resolved

morenol reviewed Oct 28, 2022

View reviewed changes

crates/fluvio-protocol/src/core/bytebuf.rs Outdated Show resolved Hide resolved

sehz modified the milestone: 0.10.1 Nov 4, 2022

sehz requested changes Nov 4, 2022

View reviewed changes

crates/fluvio-controlplane-metadata/Cargo.toml Outdated Show resolved Hide resolved

crates/fluvio-protocol/src/core/bytebuf.rs Show resolved Hide resolved

crates/fluvio-protocol/src/core/bytebuf.rs Outdated Show resolved Hide resolved

EstebanBorai added 2 commits November 4, 2022 14:37

feat: create ByteBuf from Bytes

9d73483

feat: move serde from protocol to controlplane

630aa06

morenol reviewed Nov 4, 2022

View reviewed changes

EstebanBorai added 2 commits November 4, 2022 17:16

fix: more accurate tests

e0e8a56

chore: bump versions for protocol

bd55b27

EstebanBorai requested review from morenol and sehz November 4, 2022 21:59

sehz requested changes Nov 4, 2022

View reviewed changes

EstebanBorai added 3 commits November 4, 2022 19:44

fix: serde not being used anymore

a63b4b2

fix: bump patch over minor

c069b27

fix: documentation not specialized

398e679

EstebanBorai requested a review from sehz November 4, 2022 22:49

sehz requested changes Nov 4, 2022

View reviewed changes

crates/fluvio-sc-schema/Cargo.toml Show resolved Hide resolved

chore: bump version for fluvio-sc-schema

1ebd052

EstebanBorai requested a review from sehz November 4, 2022 23:11

sehz approved these changes Nov 4, 2022

View reviewed changes

bors bot changed the title ~~feat: introduce ByteBuf for SmartModuleSpec~~ [Merged by Bors] - feat: introduce ByteBuf for SmartModuleSpec Nov 5, 2022

bors bot closed this Nov 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - feat: introduce `ByteBuf` for `SmartModuleSpec` #2738

[Merged by Bors] - feat: introduce `ByteBuf` for `SmartModuleSpec` #2738

EstebanBorai commented Oct 22, 2022

sehz left a comment

EstebanBorai commented Oct 28, 2022

morenol commented Nov 4, 2022 •

edited

EstebanBorai commented Nov 4, 2022

morenol commented Nov 4, 2022

EstebanBorai commented Nov 4, 2022

sehz commented Nov 4, 2022

sehz left a comment

morenol left a comment •

edited

sehz left a comment

sehz Nov 4, 2022

EstebanBorai Nov 4, 2022

morenol Nov 4, 2022

EstebanBorai Nov 4, 2022

sehz left a comment

sehz left a comment

EstebanBorai commented Nov 4, 2022

EstebanBorai commented Nov 4, 2022

bors bot commented Nov 5, 2022

[Merged by Bors] - feat: introduce ByteBuf for SmartModuleSpec #2738

[Merged by Bors] - feat: introduce ByteBuf for SmartModuleSpec #2738

Conversation

EstebanBorai commented Oct 22, 2022

sehz left a comment

Choose a reason for hiding this comment

EstebanBorai commented Oct 28, 2022

morenol commented Nov 4, 2022 • edited

EstebanBorai commented Nov 4, 2022

morenol commented Nov 4, 2022

EstebanBorai commented Nov 4, 2022

sehz commented Nov 4, 2022

sehz left a comment

Choose a reason for hiding this comment

morenol left a comment • edited

Choose a reason for hiding this comment

sehz left a comment

Choose a reason for hiding this comment

sehz Nov 4, 2022

Choose a reason for hiding this comment

EstebanBorai Nov 4, 2022

Choose a reason for hiding this comment

morenol Nov 4, 2022

Choose a reason for hiding this comment

EstebanBorai Nov 4, 2022

Choose a reason for hiding this comment

sehz left a comment

Choose a reason for hiding this comment

sehz left a comment

Choose a reason for hiding this comment

EstebanBorai commented Nov 4, 2022

EstebanBorai commented Nov 4, 2022

bors bot commented Nov 5, 2022

[Merged by Bors] - feat: introduce `ByteBuf` for `SmartModuleSpec` #2738

[Merged by Bors] - feat: introduce `ByteBuf` for `SmartModuleSpec` #2738

morenol commented Nov 4, 2022 •

edited

morenol left a comment •

edited