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

chore: Speed up LogEvent::rename_key by 57% #9226

Merged
merged 1 commit into from
Sep 20, 2021
Merged

Conversation

blt
Copy link
Contributor

@blt blt commented Sep 18, 2021

This commit speeds up the rename_key function of LogEvent by 57%, avoiding
the use of path iteration code to achieve this. This results in +2MB/s in #8512.

Signed-off-by: Brian L. Troutwine brian@troutwine.us

This commit speeds up the `rename_key` function of `LogEvent` by 57%, avoiding
the use of path iteration code to achieve this. This results in +2MB/s in #8512.

Signed-off-by: Brian L. Troutwine <brian@troutwine.us>
@netlify
Copy link

netlify bot commented Sep 18, 2021

✔️ Deploy Preview for vector-project ready!

🔨 Explore the source changes: 42bb5ce

🔍 Inspect the deploy log: https://app.netlify.com/sites/vector-project/deploys/6145383a060640000764f3c2

😎 Browse the preview: https://deploy-preview-9226--vector-project.netlify.app

Copy link
Member

@jszwedko jszwedko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be nice to test that the remove is flat, otherwise 👍 . Nice find.

@@ -137,12 +137,17 @@ impl LogEvent {
K: AsRef<str> + Into<String> + PartialEq + Display,
{
if from_key != to_key {
if let Some(val) = self.remove(from_key) {
if let Some(val) = self.fields.as_map_mut().remove(from_key.as_ref()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we update the tests below to catch this in the future? Presumably self.remove() was not right since it would have interpreted the key as a path.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the long-term I'd prefer to see the _flat functions disappear. Right now we avoid them to get a performance boost while the two lookup situations are in play. I do see your point about checking that we don't interpret the path, though. I'd like to gate that work on #9131 since it'll be a natural consequence of doing that work well.

@blt blt merged commit bd3d58c into master Sep 20, 2021
@blt blt deleted the log_event_key_rename_speedup branch September 20, 2021 16:36
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.

3 participants