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

Directly generate batch without intermediate entries #12

Merged
merged 2 commits into from
Dec 3, 2021

Conversation

mstoykov
Copy link
Contributor

@mstoykov mstoykov commented Dec 2, 2021

Fixes #10

@mstoykov mstoykov force-pushed the optimizeGenerateEntries branch 2 times, most recently from a55bf6d to 74b587f Compare December 2, 2021 15:25
Copy link
Contributor

@vlad-diachenko vlad-diachenko left a comment

Choose a reason for hiding this comment

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

LGTM

batch.go Outdated
Comment on lines 148 to 150
line = flog.NewLog(string(labels[model.LabelName("format")]), time.Now())
stream.Entries = append(stream.Entries, logproto.Entry{
Timestamp: time.Now(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These two calls to time.Now() are currently the biggest CPU user if you just run the examples/simple.js - we can make it into one in the begging of the for loop, but I guess you want to have the time in the line?

Copy link
Collaborator

@chaudum chaudum left a comment

Choose a reason for hiding this comment

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

Could you write a simple benchmark so we have a comparison between before and after the change?

batch.go Outdated Show resolved Hide resolved
batch.go Outdated Show resolved Hide resolved
@chaudum
Copy link
Collaborator

chaudum commented Dec 2, 2021

In general LGTM!

@mstoykov
Copy link
Contributor Author

mstoykov commented Dec 3, 2021

I did add a basic benchmark, I would've made it a table-driven one and added a bunch of variants, but I have no idea what will be good values for that, so if you provide more I will update it.

I can rearrange and squash commits if we want to keep some kind of history or not if we are just going to merge squash this in the end - I don't know what you prefer, but I have no problems with either.

p.s. the commit message has the results

@mstoykov mstoykov requested a review from chaudum December 3, 2021 09:30
@chaudum
Copy link
Collaborator

chaudum commented Dec 3, 2021

I can rearrange and squash commits if we want to keep some kind of history or not if we are just going to merge squash this in the end - I don't know what you prefer, but I have no problems with either.

We can merge squash the commits.
We disabled squash merge on this repo. Could you squash manually please? I'd keep the fix for the simple.js in a single commit and the rest in another one.

p.s. the commit message has the results

They look great!

Great work! 🥳

mstoykov and others added 2 commits December 3, 2021 14:16
Fixes #10

Basic benchmark results:
old are the results with making entries and then batch,
old- is only making the entries
new is the new variant where we directly make the batch

as can be seen there is around 50% reduction

name \ time/op     old.bench    old-.bench   new.bench
GenerateEntries-8   255µs ± 9%   202µs ± 4%  138µs ± 5%

name \ alloc/op    old.bench    old-.bench   new.bench
GenerateEntries-8  20.2kB ± 0%  12.5kB ± 0%  9.3kB ± 0%

name \ allocs/op   old.bench    old-.bench   new.bench
GenerateEntries-8     435 ± 0%     213 ± 0%    216 ± 0%

Co-authored-by: Christian Haudum <christian.haudum@gmail.com>
@mstoykov
Copy link
Contributor Author

mstoykov commented Dec 3, 2021

@chaudum merge when you want if you don't want more changes

@chaudum chaudum merged commit 7bc1f61 into main Dec 3, 2021
@chaudum chaudum deleted the optimizeGenerateEntries branch December 3, 2021 13:07
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.

Generate Batch without intermediate steps
3 participants