cruise-automation / rosbag.js

ROS bag file reader for JavaScript 👜
Apache License 2.0
186 stars 43 forks source link

More efficient parsing of message strings #76

Closed davidswinegar closed 4 years ago

davidswinegar commented 4 years ago

This should take the same runtime but should reduce memory pressure and garbage collection when reading ROS strings by not doing string concatenation on every character.

Test plan: added test for long strings

hhsaez commented 4 years ago

@davidswinegar I ran your branch for a different scenario where I'm having lots of big strings. I can confirm that your changes improve GC usage a lot. In my case, it reduced the average processing time around 50%.

Here's an example of what I was seeing before your changes:

Screen Shot 2020-06-30 at 11 43 27 AM

The orange boxes are all GC alls (mostly minor, but there are some major GC calls too)

And here's after:

Screen Shot 2020-07-01 at 12 54 57 PM

Notice there are a lot less calls to GC

MatthewSteel commented 4 years ago

Sorry to ask for more, but please check performance for short strings too -- in my microbenchmarks TextDecoder added substantial overhead for them. (Might be fine, though -- outside of microbenchmarks it might not add up to much. I guess we can look at profiles for representative numbers?)

davidswinegar commented 4 years ago

@MatthewSteel Updated this to use one TextDecoder per message definition - I saw the same thing from benchmarks. Since we have a small number of total topics I ran it with ~800 and it seemed to be pretty quick - and we'd likely have have only a fraction of those topics actually enabled.