mcap
mcap copied to clipboard
foxglove
Remove unnecessary array copy in parseRecord
Public-Facing Changes Hopefully none, other than speed-ups.
Description This was a significant source of slow-down in reading MCAP files. Not sure if there's some subtle reason why doing this copy was important, but by removing it things still seem to work and now most of the time in my logs is spent in deserialization.
I haven't had a chance to test this very carefully.
There's something causing issues in the test with trying to avoid the copy with at least reading the chunks (which is the most important part...). I won't have a chance to spend time on this for a week or two myself, but certainly wouldn't object to someone else doing so.
Not sure if there's some subtle reason why doing this copy was important
It's there because the underlying buffer may be reused between reads by the reader, so if the client keeps a long-lived reference to the data, it may observe the data being overwritten with something else later. Also, if the client were holding onto a view of a larger buffer, the whole buffer will live on in memory until the reference can be garbage collected.
You're right that this slows things down, though. The copying could be made optional, or we could foist the responsibility to copy onto the client. I still think Studio would probably end up need to do the copy because it preloads many messages into memory.
cc @defunctzombie @jhurliman for ideas
Studio currently needs the copies because it will turn uint8[] fields into Uint8Array views without copying during deserialization, which points directly at the underlying ArrayBuffer that is overwritten when decoding the next message/chunk/etc. There is also a lazy message decoder for ROS1-encoded messages that creates a proxy-like object that references the underlying byte array on demand. Unfortunately, those optimizations are forcing every raw message payload to be memcpy'd which will tank performance, probably to the point of nullifying the performance gains of the optimizations.
I don't have an idea for a quick fix, but we should take time to review the full lifecycle of messages from .mcap file on disk to decoded message in Studio and consider if there is a better way of approaching this.
More explicitly defining lifetimes to have well defined behavior would be nice.
Having a well defined behavior for how to implement lazy decoders sounds good. E.g. for flatbuffers I'd like to only have to decode message fields we actually need. But that shouldn't tank performance for decoders that just decide to deserialize everything instantly.
I also suspect there may be some outright inefficiencies, especially when it comes to chunked files (+/- dealing with decompression of compressed chunks).
But at least I know but to spend the time to try to get this correct on my own.
@jameskuszmaul-brt could you elaborate on This was a significant source of slow-down in reading MCAP files.. Any particular numbers or flame graphs you can share? I could certainly believe it but it would be good to contextualize it some more.
It's there because the underlying buffer may be reused between reads by the reader, so if the client keeps a long-lived reference to the data, it may observe the data being overwritten with something else later. Also, if the client were holding onto a view of a larger buffer, the whole buffer will live on in memory until the reference can be garbage collected.
The tradeoff I suppose would be making a new buffer for reads by the reader? @jhurliman how are you handling this in the c++ reader? Are you not allowing messages to exist beyond the lifetime of another read happening? In Studio we have limited control of the lifetime of a single message.
Where would be a good place to document the future work here?
Another github issue on this repo would be fine if its related to apis or work on mcap. If you think its a broader issue with Studio an issue on studio that references this one would be fine too.
@jameskuszmaul-brt could you elaborate on
This was a significant source of slow-down in reading MCAP files.. Any particular numbers or flame graphs you can share? I could certainly believe it but it would be good to contextualize it some more.
I was using Chrome's performance tab which included the percentage of time spent in various functions, and when altering this saw a significant change. I didn't document this in detail, but could.
Another github issue on this repo would be fine if its related to apis or work on mcap. If you think its a broader issue with Studio an issue on studio that references this one would be fine too.
It sounds like the interface with studio is highly relevant here, so filed https://github.com/foxglove/studio/issues/3531