-
Notifications
You must be signed in to change notification settings - Fork 1.9k
enhancement(vrl): Update to use VRL with Copy-on-Write optimization #22693
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
base: master
Are you sure you want to change the base?
Conversation
|
I would want to see if this regresses any of the existing soak test benchmarks before approving this to merge. @pront can we run those on a single PR on demand any more? |
|
@bruceg I think so. All variants of the |
Yes, for branches on |
|
I copied the branch into vectordotdev and rebased on the latest master. |
|
I see regression in |
It can be a flake but it can also be a legitimate issue, it's never obvious with such a wide range of use cases. I will kickoff a couple more runs. |
|
@pront Do I understand correctly that this is the reverse view? The baseline is my changes, and the comparison is |
Are you referring to this summary? If yes, it means only experiment was slower compared to master. |
15c6489 to
5ac5c88
Compare
|
I too had set up a PR that dropped the inner |
|
@pront Sorry, but I'm having hard time understanding what you mean. Regression test result shows my commit as the baseline and master as the comparison, so I assumed results are inverse (1 significant performance gain and 7+ regressions). Or am I missing something? |
|
@bruceg Oh, I see your test summary. I'll look into it. My wild guess is that it might be connected with codecs. While constructing the value, each insertion goes through Is there a way to access test events used in regression tests? I see some magic |
|
Kicked off another run here: https://github.com/vectordotdev/vector/actions/runs/14203583436 Just to manage expectations here, if the results mixed we might not accept this PR. But let's see the summary first. |
I think you're onto something. Anything where the value is created incrementally instead of using
The
lading is an open-source component, so you can try it out yourself. |
Let's see if @Zettroke can do something about improving the codec efficiency. If we can manage that, it could turn the numbers around. |
f13b711 to
3161400
Compare
1720078 to
ffe54be
Compare
|
Since the VRL PR hasn't been merged I'm converting this to a draft |
Summary
This is a paired request for vectordotdev/vrl#1349 adding support for VRL with Copy-on-Write.
I an opening it to make testing easier as we will have working
vectorversion using new VRL.Change Type
Is this a breaking change?
How did you test this PR?
Does this PR include user facing changes?
Checklist
make check-allis a good command to run locally. This check isdefined here. Some of these
checks might not be relevant to your PR. For Rust changes, at the very least you should run:
cargo fmt --allcargo clippy --workspace --all-targets -- -D warningscargo nextest run --workspace(alternatively, you can runcargo test --all)Cargo.lock), pleaserun
dd-rust-license-tool writeto regenerate the license inventory and commit the changes (if any). More details here.References