Add cache mechanism for rosidl code generation#934
Add cache mechanism for rosidl code generation#934otamachan wants to merge 4 commits intoros2:rollingfrom
Conversation
3a6255c to
a70f0be
Compare
Signed-off-by: Tamaki Nishino <otamachan@gmail.com>
a70f0be to
35baa7a
Compare
|
Do we also want to consider using the generator version as part of the cache key? This probably wouldn't affect most people, but for those of us who also make changes in the |
|
Thanks for the feedback! I agree that including the generator version in the cache key would make it more robust. I see two possible approaches to retrieve the generator version via Option 1: Use Option 2: Resolve the caller module from the call stack Does either approach sound reasonable? |
rosidl_parser/rosidl_parser/cache.py
Outdated
| return None | ||
| try: | ||
| with open(cache_file, 'rb') as f: | ||
| return pickle.load(f) |
There was a problem hiding this comment.
Are we worried at all about unpickling being unsafe?
There was a problem hiding this comment.
Thanks for the feedback! To address the unpickling safety concern, we could use JSON serialization instead of pickle here. rosidl_parser.definition contains only simple data classes with primitive attributes, so by embedding a __class__ field during serialization, we can build safe deserialization without touching definition.py.
Here's the approach:
import json
import rosidl_parser.definition as _def
_CLASSES = {
name: obj for name in dir(_def)
if isinstance(obj := getattr(_def, name), type)
}
def _decode(data):
if isinstance(data, dict):
cls_name = data.get('__class__')
entries = {k: _decode(v) for k, v in data.items() if k != '__class__'}
if cls_name:
obj = _CLASSES[cls_name].__new__(_CLASSES[cls_name])
obj.__dict__.update(entries)
return obj
return entries
if isinstance(data, list):
return [_decode(v) for v in data]
return dataSince json.loads only produces built-in Python types, and class reconstruction is limited to known classes from rosidl_parser.definition, this will be safer than unpickling.
wjwwood
left a comment
There was a problem hiding this comment.
Thanks for putting this forward. A performance improvement on this scale would be a huge improvement.
I'm torn about whether this is the right way to implement the caching. I think the pros of this approach is that it's non-invasive and gives the user control over how much storage is used and where it's located, a la ccache.
This is good, but I can't help but think that it would be even better if we could preserve and distribute these cache files, so that users can benefit from the caching without explicitly setting it up locally. However, there are some unknowns (to me) that might make my idea impractical.
I'd like to know, if you or anyone else has an idea:
- How big are these files
- I saw you have a default ~1GB max size, is that arbitrary or based on experience?
- How would switching to json from pickle impact this, if at all?
- Does the caching take into account the source code that parses the interface files?
- Put another way, would changing the code that parses rosidl files invalidate the cache?
- Seems like "no" from a cursory look, but maybe I missed something
If the files are relatively small, and we can somehow work the hash of the parsing code into the cache key, then I think we could do something like:
- Check paths in each
AMENT_PREFIX_PATHfor cached files first- Cache miss might be due to no cache files found, a change in the rosidl parser invalidates it, or a local change (e.g. someone sudo-edited a
.msgfile in/opt)
- Cache miss might be due to no cache files found, a change in the rosidl parser invalidates it, or a local change (e.g. someone sudo-edited a
- If not found, look in the build folder for the current package for cached files
- Finally if neither of those have a cache hit, parse the file and store it in the build folder
- Then afterwards, install cache files for interface files of the current package so they can be found by future packages.
This would move this idea towards building/installing precompiled headers or something akin to .pyc files, rather than a model that more closely resembles ccache (in my opinion).
If there's no appetite to do this, then this pr as-is is better than what we have now for sure. I can imagine push back because what I described would be more complicated, requiring cmake changes and wiring those installed files into the command line tool so it can find them.
But I'm interested to hear what others think of what I've said here.
|
One other thought, which is more of a "yes and...", is that if we can get such a big win by caching (which itself requires cache hit checking and then parsing the cache file) the idl parsing, is there perhaps some low hanging fruit in the idl parsing code that could speed that process up considerably as well? It's already useful to know that this part of the process is so expensive, so thanks again @otamachan. |
|
To answer my own question, I see now from the original issue (#931 (comment)) that cache size (for 1 package?) is like ~72MB, which is bigger than I expected, to be honest. To big, I would say, to distribute the cache without more thought. I'm curious though, based on your comment #934 (comment), whether or not the json approach would be a lot smaller since you're theoretically storing a lot less than blindly pickling the whole data structure. |
|
Thank you for the thoughtful feedback and for considering alternative caching strategies — I really appreciate it. Cache file size:The 72MB reported in the original issue is for Default max size (~1GB):This is an arbitrary value, loosely based on ccache's default of 5GB. I chose a smaller default since rosidl cache files are generally much smaller than compiled object files. JSON vs pickle size impact:I haven't measured this yet but plan to. Since the Parser source code in cache key:You're right — currently the cache key only includes the IDL file content, so changes to the parser itself would not invalidate the cache. As discussed in #934 (comment), I think including the parser package version in the cache key (similar to what was suggested for generators) would be a good way to address this. On the ccache vs install/distribute approachI think the .pyc-like approach of storing cache files in the build/install space and looking them up via AMENT_PREFIX_PATH is a good idea — it would allow users to benefit from caching without any explicit setup. A few things I'd like to share for consideration:
I'd be happy to hear your thoughts on this. |
Signed-off-by: Tamaki Nishino <otamachan@gmail.com>
|
Added two commits on top of the existing PR:
Benchmark results (px4_msgs, -j8, ccache + rosidl cache):
|
…pgrades Signed-off-by: Tamaki Nishino <otamachan@gmail.com>
a0bd221 to
47707c4
Compare
…y changes Signed-off-by: Tamaki Nishino <otamachan@gmail.com>
Description
Add a cache mechanism for rosidl code generation to speed up rebuilds when IDL files and templates have not changed.
This caches two stages of the rosidl pipeline (see #931 for motivation and benchmarks):
rosidl_parser/parser.py): Caches theIdlFileobject returned byparse_idl_file().rosidl_pycommon/__init__.py): Caches the output files produced by each generator (e.g.,rosidl_generator_py,rosidl_generator_cpp).The cache is opt-in and disabled by default. It is only activated when
ROSIDL_CACHE_DIRorROSIDL_CACHE_CONFIGis set.Is this user-facing behavior change?
Users can set the
ROSIDL_CACHE_DIRenvironment variable to enable caching of rosidl code generation, which can speed up incremental builds.Did you use Generative AI?
Claude Opus 4.6 was used for code review feedback.
Additional Information