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

Go: Allocate less in indexed message chunk loading #950

Merged
merged 5 commits into from
Aug 24, 2023

Conversation

wkalt
Copy link
Contributor

@wkalt wkalt commented Aug 23, 2023

Prior to this commit, when reading a chunk into memory we would read it (internally, in io.ReadAll) into a resizing buffer and need to resize it several times up to final size. Since we know the final size in advance we can preallocate and save those allocations/copies.

@wkalt wkalt force-pushed the ticket/fg-4715/reader-less-allocs-go branch from dad9b02 to 8644365 Compare August 23, 2023 02:19
@wkalt
Copy link
Contributor Author

wkalt commented Aug 23, 2023

simple timings

main

[~/work/mcap/go/cli/mcap] (main) $ time mcap cat cal_loop.mcap > /dev/null
real    0m5.047s
user    0m4.719s
sys     0m1.083s

patch

[~/work/mcap/go/cli/mcap] (ticket/fg-4715/reader-less-allocs-go) $ time mcap cat cal_loop.mcap > /dev/null

real    0m3.084s
user    0m2.716s
sys     0m0.532s

40% faster

Prior to this commit, when reading a chunk into memory we would read it
(internally, in io.ReadAll) into a resizing buffer and need to resize it
several times up to final size. Since we know the final size in advance
we can preallocate and save those allocations/copies.
@wkalt
Copy link
Contributor Author

wkalt commented Aug 23, 2023

lz4 numbers

main

[~/work/mcap/go/cli/mcap] (main) $ time mcap cat cal_loop.lz4.mcap > /dev/null

real    0m3.802s
user    0m3.563s
sys     0m0.622s

patch

[~/work/mcap/go/cli/mcap] (ticket/fg-4715/reader-less-allocs-go) $ time mcap cat cal_loop.lz4.mcap > /dev/null

real    0m2.372s
user    0m2.151s
sys     0m0.354s

@wkalt
Copy link
Contributor Author

wkalt commented Aug 23, 2023

it seems like there would be further savings if we could reuse the chunk buffers, but I think this is too complicated by the heap. I also noticed the Next() method still takes a buffer which is now unused (I think originally that was passed to the lexer's Next method to conserve allocations) but I think resurrecting it is complicated now. Maybe we can sunset it, or maybe it actually is used for the unindexed iterator (need to check).

@wkalt
Copy link
Contributor Author

wkalt commented Aug 23, 2023

last commit shaves a bit more off

[~/work/mcap/go/cli/mcap] (ticket/fg-4715/reader-less-allocs-go) $ time mcap cat cal_loop.mcap > /dev/null

real    0m2.734s
user    0m2.527s
sys     0m0.286s

@wkalt
Copy link
Contributor Author

wkalt commented Aug 23, 2023

with final commit, I got a

[~/work/mcap/go/cli/mcap] (ticket/fg-4715/reader-less-allocs-go) $ time mcap cat cal_loop.mcap > /dev/null

real    0m2.515s
user    0m2.374s
sys     0m0.219s

which is close to 50% better than the first observation

@wkalt wkalt changed the title Allocate less in indexed message chunk loading Go: Allocate less in indexed message chunk loading Aug 23, 2023
@wkalt
Copy link
Contributor Author

wkalt commented Aug 24, 2023

there is still some preexisting issue with very large inputs, where reading becomes sluggish after a while. I need to debug it further to understand what is happening. Won't merge this til I understand better.

@wkalt
Copy link
Contributor Author

wkalt commented Aug 24, 2023

ok I think ^^ is just my terminal emulator, I verified with pv there's no actual slowdown and if I use xterm it appears to read at a consistent speed too. Merging.

@wkalt wkalt merged commit 9491d4f into main Aug 24, 2023
24 checks passed
@wkalt wkalt deleted the ticket/fg-4715/reader-less-allocs-go branch August 24, 2023 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants