[ATFE] Improve multilib common header optimisation - #923
Conversation
The script currently seeds the common header set by
comparing only the first two include directories
returned by os.listdir(). That makes the result
sensitive to directory order and can under-detect common
headers when one of those early directories is a sparse
overlay like minor_libs.
This PR improves multilib header optimisation for
Arm embedded toolchain builds by Updating common
header generation to select shared headers by
relative path and content hash across primary
multilib variants, while treating reduced
minor variants separately so they do not shrink the
common header set.
The common header generation script now:
- groups headers by relative path and SHA-256 content hash
- selects common headers from primary `stdlibs` variants only
- treats minor/reduced variants separately so missing headers
there do not reduce the shared header set
- keeps variant-specific copies when header contents differ from
the selected common version
- validates that each variant has a `Group` entry in `multilib.yaml
The CMake integration now drives common-header generation through a
stamp-file-backed custom command, making the generated output
dependency-tracked.
voltur01
left a comment
voltur01
left a comment
There was a problem hiding this comment.
Thank you for updating the script! As far as I can see, it should do the job, however I added a few ideas how to make the code easier to understand at least for me.
| "${CMAKE_CURRENT_BINARY_DIR}/multilib-optimised" | ||
| COMMAND ${CMAKE_COMMAND} -E touch "${multilib_common_headers_stamp}" | ||
| COMMENT "Generating common headers" | ||
| DEPENDS ${all_runtime_targets} multilib-yaml |
There was a problem hiding this comment.
What problem is this stamp solving and does it need to depend on the Python script itself? also the YAML file with library variants classification.
| def file_content_hash(path): | ||
| content_hash = hashlib.sha256() | ||
| with open(path, "rb") as file: | ||
| for chunk in iter(lambda: file.read(1024 * 1024), b""): |
There was a problem hiding this comment.
Minor: You may want to introduce a named constant like HASH_CHUNK_SIZE or such.
| return headers | ||
|
|
||
|
|
||
| def parse_yaml_scalar(value): |
There was a problem hiding this comment.
Minor: Maybe strip instead of parse?
|
|
||
| def collect_variant_groups(multilib_yaml): | ||
| # Navigate multilib.yaml, only paying attention to the Variants: section | ||
| # and skipping other sections. Inside Variants:, remember each Dir until |
There was a problem hiding this comment.
Nit: Colon is confusing here, at first it reads as an expanasion/expansion follows. For me, either "Variants:" with quotes or Variants without colon would be more clear.
| current_dir = None | ||
| in_variants = False | ||
|
|
||
| with open(multilib_yaml, encoding="utf-8") as file: |
There was a problem hiding this comment.
Does not LLVM libc build script already require a Python YAML parser module? Maybe we can also use it here, if it is easier/cleaner?
| def collect_variant_groups(multilib_yaml): | ||
| # Navigate multilib.yaml, only paying attention to the Variants: section | ||
| # and skipping other sections. Inside Variants:, remember each Dir until | ||
| # its matching Group is found, then record "target/variant" -> "group_name". |
There was a problem hiding this comment.
Would it be useful to add a snippet of the YAML file here as a comment to see the structure being parsed?
| in_variants = False | ||
|
|
||
| with open(multilib_yaml, encoding="utf-8") as file: | ||
| for line in file: |
There was a problem hiding this comment.
This loop seems to be a state machine and should be correct as far as I can tell, however may be a bit tricky to reason about. For me it maybe easier to have a helper like skip_until(tag) so that the logic could be similar to:
before the loop skip_until('Variants')
loop:
skip_until('Dir') -> remember the path
skip_until('Group') -> add the record to the map
BTW, there may be a safety mechanism added (additional parameter to the helper) when looking for the Group to make sure there is no another Dir on the way to the next Group.
| continue | ||
|
|
||
| if stripped.startswith("- Dir:"): | ||
| current_dir = parse_yaml_scalar(stripped.split(":", 1)[1]) |
There was a problem hiding this comment.
This can be another helper like get_attribute_value(line)
| copy_header(variant_header_path, variant_include_dir, header_name) | ||
|
|
||
| minor_headers = group_headers_by_name_and_content_hash(minor_includes) | ||
| for header_name in sorted(minor_headers): |
There was a problem hiding this comment.
This whole block looks very similar to the non-common headers above - is not it possible to make a helper to call there and then here?
|
|
||
| if common_group: | ||
| common_content_hash, common_entries = common_group | ||
| copy_header(common_entries[0][1], output_include_dir, header_name) |
There was a problem hiding this comment.
It would be useful to either add add a type annotation or a comment illustrating the structure of these maps to have a better idea what this refers to.
Or maybe a named helper function again with the structure explanation inside it.
2 participants
The script currently seeds the common header set by comparing only the first two include directories
returned by os.listdir(). That makes the result sensitive to directory order and can under-detect common headers when one of those early directories is a sparse overlay like minor_libs.
This PR improves multilib header optimisation for Arm embedded toolchain builds by updating common header generation to select shared headers by relative path and content hash across primary multilib variants, while treating reduced minor variants separately so they do not shrink the common header set.
The common header generation script now:
stdlibsvariants onlyGroupentry in `multilib.yamlThe CMake integration now drives common-header generation through a stamp-file-backed custom command, making the generated output dependency-tracked.