Skip to content
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

Get property without Vec allocation #216

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

andresmargalef
Copy link

This patch add a new struct PropertyPath and a macro used to create a PropertyPath with a slice of &str using const generics to prevent allocations in runtime.

Before:

let property_path = vec!["path", "to", "property"];
let property_bytes = match proxy_wasm::hostcalls::get_property(property_path) {
    Ok(value) => value,
    _ => return Action::Continue,
};

New method:

let property_path = create_property_path!("path", "to", "property");
let property_bytes = match proxy_wasm::hostcalls::get_property_by_path(&property_path) {
    Ok(value) => value,
    _ => return Action::Continue,
};

…sed.

This patch add a new struct PropertyPath and a macro used to create a PropertyPath with a slice of &str using const generics to prevent allocations in runtime.

Signed-off-by: Andres Margalef <[email protected]>
@andresmargalef andresmargalef changed the title The current code needs to allocate a new Vec when a property is accessed Get property without allocating a vec Nov 4, 2023
@andresmargalef andresmargalef changed the title Get property without allocating a vec Get property without Vec allocation Nov 4, 2023
@PiotrSikora
Copy link
Member

@andresmargalef this looks great, but I'm wondering what's the motivation behind this change? Does this result in measurable performance changes in your plugins?

@andresmargalef
Copy link
Author

The motivation is latency reduction removing extra work like allocations, right now i'm building an example application using wasmtime to test locally ours filters using this patch vs master. I will share any results here :D

@andresmargalef
Copy link
Author

The latency reduction is ~30%, the "toy" benchmark runs 1000 iterations and calculate the avg in nanos. The test call only one get_property using a vec vs get_property_path, and on the host side always returns 1 (NotFound).
I need to improve de code and use criterion to get better numbers.

request_metrics.wasm.optimized[1000 iterations]: 452 avg nanos
request_metrics.wasm.slow[1000 iterations]: 510 avg nanos
request_metrics.wasm.optimized[1000 iterations]: 278 avg nanos
request_metrics.wasm.slow[1000 iterations]: 634 avg nanos
request_metrics.wasm.optimized[1000 iterations]: 230 avg nanos
request_metrics.wasm.slow[1000 iterations]: 426 avg nanos
request_metrics.wasm.optimized[1000 iterations]: 196 avg nanos
request_metrics.wasm.slow[1000 iterations]: 278 avg nanos
request_metrics.wasm.optimized[1000 iterations]: 191 avg nanos
request_metrics.wasm.slow[1000 iterations]: 289 avg nanos
request_metrics.wasm.optimized[1000 iterations]: 191 avg nanos
request_metrics.wasm.slow[1000 iterations]: 276 avg nanos
request_metrics.wasm.optimized[1000 iterations]: 188 avg nanos
request_metrics.wasm.slow[1000 iterations]: 276 avg nanos
request_metrics.wasm.optimized[1000 iterations]: 188 avg nanos
request_metrics.wasm.slow[1000 iterations]: 277 avg nanos

@andresmargalef
Copy link
Author

Hi @PiotrSikora , do you think it makes sense to merge this pull request? Do not hesitate to tell me about any changes you see that need to be made as well as any other perf test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants