enhancement: Use new Lookup accessors on LogEvents and entire internal API. #5374
Conversation
Signed-off-by: Ana Hobden <operator@hoverbear.org>
Signed-off-by: Ana Hobden <operator@hoverbear.org>
Signed-off-by: Ana Hobden <operator@hoverbear.org>
Signed-off-by: Ana Hobden <operator@hoverbear.org>
Signed-off-by: Ana Hobden <operator@hoverbear.org>
Signed-off-by: Ana Hobden <operator@hoverbear.org>
Signed-off-by: Ana Hobden <operator@hoverbear.org>
Signed-off-by: Ana Hobden <operator@hoverbear.org>
Signed-off-by: Ana Hobden <operator@hoverbear.org>
Signed-off-by: Ana Hobden <operator@hoverbear.org>
Signed-off-by: Ana Hobden <operator@hoverbear.org>
Signed-off-by: Ana Hobden <operator@hoverbear.org>
Signed-off-by: Ana Hobden <operator@hoverbear.org>
Signed-off-by: Ana Hobden <operator@hoverbear.org>
Signed-off-by: Ana Hobden <operator@hoverbear.org>
Signed-off-by: Ana Hobden <operator@hoverbear.org>
Signed-off-by: Ana Hobden <operator@hoverbear.org>
Signed-off-by: Ana Hobden <operator@hoverbear.org>
Signed-off-by: Ana Hobden <operator@hoverbear.org>
Hoverbear
requested review from
a team and
leebenson
and removed request for
eeyun and
a team
|
I should note that currently this is pulling in the entire legacy Remap pest parser to parse the paths. This needs to be tidied up. It seems extravagant to have three different parsing libraries in a single project, so it would be useful if we could reuse some of the lalrpop code - or since it is quite a simple parsing task, perhaps we could use nom, which may also provide some performance benefits.. |
This strikes me as something that would be best tackled in a follow-up issue, to avoid this PR ballooning any further. |
|
Ah, hmm. #7014 collides with this change set as the metrics stuff is no longer in the top-level crate but |
Yeah, a lot of the conflicts this gets are caused by the move to shared. |
Indeed. This is a change I'd like to see unwound actually. My proposal for the core extraction is that core be the top-level crate and these are very much core-level concerns, to my mind. |
|
Okay, benchmarking process started. Should have numbers in a few hours. |
|
The results so far are not uniformly positive. I've made a comparison between 6c0df56 and cb12351 with this run script. The following gist has the vector configs referenced in the below. HyperfineThe first phase of the this benchmark uses the hyperfine tool to compare vector runtime versions in a stable way. The results: In the case of SummaryIn two of the three cases tested this PR is dramatically different than master. Unfortunately, in one case it is dramatically slower. The case in which this PR is dramatically slower is the "nohop" variant of a remap pipeline, likely to be the manner in which users configure their vector (as we've instructed them to). At least, this "nohop" configuration is not obviously wrong and the 28+/-5% difference is steep. Perf / FlamegraphThis flamegraphs generated from perf are here. The overall shape of the graphs for each variant are similar -- good -- although the timings are different. However, since the structure of the code has changed so dramatically between this PR and master it's quiet hard, overall, to draw any conclusions here. SummaryInconclusive. Comparisons of this kind are best with small differences. MassifThe massif results for each variant are available in this zip file: massif-results.zip The programs do not exhibit the leak fixed in #7014. In each variant across all configurations there is very little difference in the allocation pattern presented. SummaryNo difference between variants. |
|
Looking at the data here I'm inclined to say that we should close this PR out. While it's clear that there are performance gains to be made it's also clear that this PR causes a significant pessimization in a remap heavy configuration, something we're pushing for. I consider it likely that this pessimization is fixable and that the approach taken here has legs but this PR is too big. I do not have a hypothesis on where the pessimization is and while it's possible to get set up to make a hypothesis that in itself will be a non-trivial amount of work. As @FungusHumungus noted keeping this PR up to date with master branch is a daily feat. Further, but less concerning, we have not significantly reduced allocations in this PR relative to master, with regard to #6831. Saving allocations were part of this PR's goal set, I do believe. If we were to continue with this PR -- and I would like to acknowledge that a lot of work has gone into this PR -- I think we will need to:
The last point seems the hardest to achieve to me, though it's potentially addressed by the first. @FungusHumungus you've spent the most time wrestling with this code lately. Do you see the situation here differently than I do? |
|
Those are interesting findings. So it seems that the PR is significantly faster, unless it has to deal with Remap. This suggests that the issue is in the interface between Remap and the Lookup api. I would put my money on the culprit being here: https://github.com/timberio/vector/blob/new-accessors/lib/shared/src/lookup/lookup_buf/mod.rs#L107-L114 impl TryFrom<&vrl::Path> for LookupBuf {
type Error = LookupError;
fn try_from(target: &vrl::Path) -> Result<Self, Self::Error> {
let path_string = target.to_string();
trace!(path = %path_string, "Converting to LookupBuf.");
LookupBuf::from_str(&path_string)
}
}Any time Remap has to access a path in the event it needs to convert the Remap path into a Lookup path. It does this by dumping it to a string and then reparsing it. The main objective behind this lookup was to reduce these parsing costs. However, when using Remap, it actually now increases the amount of parsing that occurs. |
The one thing I would note here is that a few months ago — when both VRL and this work were in early development — it was decided to get this PR over the finish line without considering any performance regressions related to Vector <> VRL integration. That is to say, a naïve implementation of The thought process behind that decision was that there is nothing technically stopping us from using the same performance optimizations w.r.t. path lookup in both VRL and Vector, by extracting out that logic into its own crate, having both VRL and Vector depend on it, and allowing us to pass paths between both without any conversion or allocation needed. Obviously, this PR has lingered since, and VRL has shipped, so a lot has changed since then, but I never spent any large amount of time improving VRL's path lookup performance precisely because this work was on the horizon, so it might still be worth pushing this PR over the finish line if benchmarks show performance improvements leaving VRL out of the picture. It would require us to schedule work to actually improve VRL <> Lookup integration, since we can't ship this work in a new release without tackling that first. Having said that, the PR is massive (a lot of work went into getting this done, as you mentioned), and it feels somewhat like a daunting task to get this in and then iterate on it further. One way forward is to take the idea proven in this PR, and build a new lookup crate that can be worked on in isolation, iterating on it in small PRs, until it's at a stage where both Vector and VRL can use it, and we can have one or two relatively larger PRs to do all the legwork of updating all path references to the new lookup API. |
|
I would be interested if a naive implementation of TryFrom: impl TryFrom<&vrl::Path> for LookupBuf {
type Error = LookupError;
fn try_from(target: &vrl::Path) -> Result<Self, Self::Error> {
Ok(LookupBuf {
segments: target
.segments()
.iter()
.map(|segment| match segment {
vrl::Segment::Field(vrl::Field::Regular(field)) => SegmentBuf::Field {
name: field.clone(),
requires_quoting: false,
},
_ => unimplemented!(),
})
.collect(),
})
}
}had any impact. It's not perfect as there are still allocations, but it should be better than running it through pest. If it shifts things in the right direction, it would be indicative that the issue is here and would warrant further investigation. Note this only works with simple fields - no indexing or coalescing, but it looks like that is all the test conf uses anyway. The 28% potential improvement is IMO well worth pursuing. |
This strikes me as the proper way forward. The ideas in this PR are absolutely worth pursuing but we need to be careful to separate out the new work -- the lookup crate -- from the mechanical work of integrating it. These two things got integrated into one PR and it makes interrogating the new work quite hard. Personally, I'd be very happy to see an issue made to extract the path work here into a separate crate, PR and then a follow-up issue for integration into the wider project. Doing that we should be able to capture the performance gain of this PR without imposing the loss. I would also vote to prioritize this project. @FungusHumungus @JeanMertz @binarylogic I know we talked about this some out-of-ticket. Does this take capture your thoughts on the subject? |
Yes. I would suggest in the following order.
|
Could be I'm wrong there. I've only got vague notions for #7027 so far and this is part of that. As is, the only blocker I'm aware of with
FWIW if VRL could be made to parse by reference that would go a long way to addressing #6831.
I buy the general approach, yeah. I like that it can be done gradually. |
Vrl only needs the Lookup path data structures. It absolutely doesn't want anything to do with the Event. That way it can remain agnostic to the data it is running on top of. So the new crate should really only contain the core Lookup api. The actual lookup code, Event, Metrics etc can remain within Vector core. |
Rad. Even better. |
|
@binarylogic @JeanMertz @FungusHumungus I've made #7070 to replace this PR. I'd be grateful for your thoughts. |
|
Closing in favor of #7070. |
Closes #2843
Closes #2845
Closes #4005
This is a ... chonky PR.
Most of it is trivial (and largely mechanized) changes to some basic interfaces around Event types, as well as a bunch of new code. Much of the old code remains in deprecated state for
remap-langto fully adopt.Notable developer facing changes
Event,Value, and the newLookup/LookupBuftypes are now fully documented and tested, including several corner cases found during development.From<String>andFrom<&str>are no longer implemented onEvent.Eventtype was moved intosharedand can no longer access the globallog_schema. Yes, it was a lot of work.LookupBuf, a newStringanalog which pre-parses lookup paths and allows rich remap-syntax for APIs, has been added.Lookup, a new&stranalog which pre-parses lookup paths and allows rich remap-syntax for APIs, has been added. It can be trivially used fromLookupBuf, like a&strand aString.LookupBuftype is deserializable, as well as serializable (likeLookup). This meansConfigstructs can freely contain them and use them, without a conversion step.log_event! {}macro is much more prominent, use it, please!EventandValuehave (nearly) matching APIs aroundinsert/removeetc. OnValuethese return a result, as the action is not valid on 'primitives'.EventAPI uses cleanly, and gives us a lot more options around working with things.log_event.insert(String::from("foo.bar"), 1)will now insert at"foo.bar"the value 1, where prior it would insert"bar", set to 1, into"foo".log_event.insert(LookupBuf::from_str("foo.bar")?, 1)will now insert"bar", set to 1, into"foo", as strings did prior.EventandValuetypes have been moved intosharedto facilitate sharing between all our crates, making the type more globally accessible. They are re-exported in the old places, you shouldn't notice.LookandLookSegmenttrait are present to enforce parity between types, but it's preferred that the types implement functions directly, to ensure users always have function access (even without the traits imported).Reviewer Notes
I suggest treating this PR in three separate chunks:
shared. It will introduce to you the newEventtype and the related API.vectorto ensure we are not reading any raw data asLookuptypes.TODOs
Value::deserializeimpl.