[CORE] Align constant offsets to 8 B in .bin serialization#36733
[CORE] Align constant offsets to 8 B in .bin serialization#36733LeoHeller wants to merge 5 commits into
Conversation
ConstantWriter::write() previously wrote constant data at whatever stream position the topological traversal produced, yielding unaligned offsets when constants of different element sizes (bool 1 B, i32 4 B, i64 8 B) were serialized consecutively. At load time the runtime mmaps the .bin and constructs a SharedBuffer (subclass of AlignedBuffer) whose get_ptr<T>() does reinterpret_cast<T*> at the stored offset. When that offset is not a multiple of alignof(T) the reinterpret_cast and subsequent dereference are undefined behaviour. Fix inserts up to 7 zero-bytes of padding before each constant write to bring the offset to an 8-byte boundary (the maximum scalar alignment for any OpenVINO element type). Deduplication is checked before padding so that a duplicate hit does not leave orphaned padding bytes. Backward-compatible: only newly serialised .bin files get aligned offsets; old files continue to load (their offsets are unchanged). Uses the existing ov::util::align_padding_size utility already employed by CompiledBlobHeader for the same purpose.
|
please add tests to cover the change |
Add a round-trip test that serializes a model with mixed element-size constants (bool 1 B, i32 4 B, i64 8 B), reloads it, and asserts every constant's raw data pointer satisfies uintptr_t % element_type.size() == 0. Without the alignment fix this test fails for the i64 constant whose data lands at offset 5 in the .bin file.
Done. |
The test asserted an exact .bin file size of 2 bytes to verify two distinct 1-byte constants were not deduplicated. Alignment padding now adds 7 bytes between them. Replace the brittle size assertion with a check that the two reloaded constants have different data pointers, proving no false deduplication occurred.
|
build_jenkins |
m_out_xml_path is std::filesystem::path in SerializePassTest but readModel expects std::string. MSVC does not implicitly convert filesystem::path for overload resolution. Add .string() to match the existing call site at line 94.
| auto c = std::dynamic_pointer_cast<Constant>(node); | ||
| if (!c || c->get_element_type() == element::string) | ||
| continue; | ||
| auto alignment = c->get_element_type().size(); |
There was a problem hiding this comment.
Alignment value on writing is unconditional, while here it depends on type size. Would it pass without above changes in ConstantWriter::write?
There was a problem hiding this comment.
No, it would fail without the fix. it lands at offset 5 without the fix, and 5 % 8 != 0.
There was a problem hiding this comment.
Possibly in the future one could pass the element type/size to the write callsite to use minimal padding, but for now always padding to 8 seems more elegant and simpler.
|
Tests now pass locally, hopefully also in CI. |
|
@LeoHeller |
No. I believe on ARM64 it can deliver SIGBUS but usually it is fine. However we build openvino with the address sanitizer which then complains. Could you please take a look at the failed CI job, it seems to be unrelated to my changes? |
Yes, it's unrelated. We find this issue worth solving - thanks for catching it. Currently we're preparing for 2026.3 release and it's vacation seasons - we'll get back following weeks. |
|
build_jenkins |
5 participants
Details:
ConstantWriter::write()previously wrote constant data at whatever stream position the topological traversal produced, yielding unaligned offsets when constants of different element sizes are serialised consecutively..binand constructs aSharedBufferwhoseget_ptr<T>()performsreinterpret_cast<T*>(mmap_base + offset). An unaligned offset makes this undefined behaviour.ov::util::align_padding_sizeutility..binfiles get aligned offsets; old files load unchanged (their offsets are not retroactively fixed, but the reader does not validate alignment, so they remain loadable).Tickets:
AI Assistance: