search

Comcast / sirius

A distributed system library for managing application reference data
http://comcast.github.io/sirius/
Apache License 2.0
298 stars 49 forks source link

Switching from RandomAccessFile to SeekableByteChannel for uberstore I/O... #24

Closed mattinger closed 5 years ago

mattinger commented 10 years ago

....

This will give us inherent buffering and memory mapped I/O, and should speed up the uberstore I/O operations significantly.

I say about a 36% speedup locally loading in a 5.6GB file, with around 6M records on my local machine. Further speed tests should be performed on various hardware configurations.

clinedome-work commented 10 years ago

This is an updated version of PR #7.

mbyron01 commented 10 years ago

Matt do you have some metrics around the speedup? I'd love to see some performance testing number around this pull request. If you need help with the testing part let us know.

mattinger commented 10 years ago

All i did was ad hoc testing using xtvapi. I have not run a formal load test or anything. We'd still need to do that, but i don't see anything that exists in the sirius repository yet to facilitate that. We should get together on monday and discuss an approach for load testing this. i'm not anticipating this being merged right away. i figure it will have to undergo an extensive load test. if we don't see significant enough gains, it wouldn't be worth the risk to change the code.

On Thu, Feb 13, 2014 at 11:26 AM, Maulan Byron notifications@github.comwrote:

Matt do you have some metrics around the speedup? I'd love to see some performance testing number around this pull request. If you need help with the testing part let us know.

Reply to this email directly or view it on GitHubhttps://github.com/Comcast/sirius/pull/24#issuecomment-34995863 .

jonm commented 10 years ago

Can this change be restructured in a way that allows a configuration parameter to opt into this implementation? Memory-mapped I/O benefits are often highly context dependent, so this may not be a win for everyone. @mattinger: If you've seen benefits, then it seems reasonable to include this as an option.