Skip to content

[CORE] Align constant offsets to 8 B in .bin serialization #36733

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
LeoHeller wants to merge 5 commits into
base: master
Choose a base branch
from
Open

[CORE] Align constant offsets to 8 B in .bin serialization #36733

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ffdb0da
fix: align constant offsets to 8 B in .bin serialization
LeoHeller Jul 6, 2026
412e37a
test: verify constant data pointers are aligned after serialization
LeoHeller Jul 6, 2026
699b9f7
test: fix EmptyAndNotEmptyConstantsDifferentValues for alignment padding
LeoHeller Jul 6, 2026
7b3a32a
test: fix MSVC build, filesystem::path to string conversion
LeoHeller Jul 6, 2026
ada8283
Merge branch 'master' into fix/constant-bin-alignment
LeoHeller Jul 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 17 additions & 2 deletions src/core/src/xml_util/constant_writer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include "openvino/xml_util/constant_writer.hpp"

#include "openvino/core/except.hpp"
#include "openvino/core/memory_util.hpp"
#include "openvino/reference/convert.hpp"
#include "openvino/runtime/compute_hash.hpp"
#include "openvino/util/common_util.hpp"
Expand All @@ -27,18 +28,19 @@ ConstantWriter::FilePosition ConstantWriter::write(const char* ptr,
ov::element::Type src_type,
bool ptr_is_temporary) {
const FilePosition write_pos = m_binary_output.get().tellp();
const auto offset = write_pos - m_blob_offset;
const auto raw_offset = write_pos - m_blob_offset;

@praasz praasz Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const auto raw_offset = write_pos - m_blob_offset;
const FilePosition offset = m_binary_output.get().tellp() - m_blob_offset;

new_size = size;

const auto fp16_data = compress_to_fp16 ? compress_data_to_fp16(ptr, size, src_type, new_size) : nullptr;
const auto data_ptr = compress_to_fp16 ? fp16_data.get() : ptr;

HashValue hash = 0;
if (m_enable_compression) {
// This hash is weak (but efficient). For example current hash algorithms gives
// the same hash for {2, 2} and {0, 128} arrays.
// But even strong hashing algorithms sometimes give collisions.
// Therefore we always have to compare values when finding a match in the hash multimap.
const HashValue hash = ov::runtime::compute_hash(data_ptr, new_size);
hash = ov::runtime::compute_hash(data_ptr, new_size);

const auto found = m_hash_to_file_positions.equal_range(hash);
// iterate over all matches of the key in the multimap
Expand All @@ -47,6 +49,19 @@ ConstantWriter::FilePosition ConstantWriter::write(const char* ptr,
return it->second.first;
}
}
}

// Pad to 8 B to avoid UB from unaligned reinterpret_cast during mmap access.
constexpr size_t alignment = 8;
const auto pad = ov::util::align_padding_size(alignment, static_cast<size_t>(raw_offset));
if (pad > 0) {
constexpr char zeros[alignment] = {};
m_binary_output.get().write(zeros, pad);
}
const FilePosition aligned_pos = m_binary_output.get().tellp();
const auto offset = aligned_pos - m_blob_offset;

if (m_enable_compression) {

@praasz praasz Jul 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead break main body append insert aligned bytes at end.

   const auto pad_size = align_padding_size(8, static_cast<size_t>(offset));
    std::fill_n(std::ostream_iterator<char>(m_binary_output.get()), pad_size, 0);
    m_binary_output.get().write(data_ptr, new_size);
    return offset + pad_size;

Then other part should not be required to modify and have same logic:

  1. prepare
  2. hashing
  3. write

if (!ptr_is_temporary) {
// Since fp16_compressed data will be disposed at exit point and since we cannot reread it from the
// ostream, we store pointer to the original uncompressed blob.
Expand Down
15 changes: 9 additions & 6 deletions src/core/tests/pass/serialization/const_compression.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,22 +357,25 @@ TEST_F(SerializationConstantCompressionTest, EmptyAndNotEmptyConstantSameValues)
}

TEST_F(SerializationConstantCompressionTest, EmptyAndNotEmptyConstantsDifferentValues) {
constexpr int unique_const_count = 2;
auto A = ov::op::v0::Constant::create(ov::element::i32, ov::Shape{0}, std::vector<int32_t>{});
auto B = ov::op::v0::Constant::create(ov::element::i8, ov::Shape{1}, std::vector<int8_t>{1});

auto model_initial = std::make_shared<ov::Model>(ov::OutputVector{A, B}, ov::ParameterVector{});

ov::pass::Serialize(m_out_xml_path_1, m_out_bin_path_1).run_on_model(model_initial);

std::ifstream xml_1(m_out_xml_path_1, std::ios::binary);
std::ifstream bin_1(m_out_bin_path_1, std::ios::binary);

ASSERT_EQ(file_size(bin_1), unique_const_count * sizeof(int8_t));

ov::Core core;
auto model_imported = core.read_model(m_out_xml_path_1, m_out_bin_path_1);

// Verify the two constants were not deduplicated (different data, different pointers)
std::vector<const void*> ptrs;
for (auto& node : model_imported->get_ops()) {
if (auto c = std::dynamic_pointer_cast<ov::op::v0::Constant>(node))

@praasz praasz Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (auto c = std::dynamic_pointer_cast<ov::op::v0::Constant>(node))
if (auto c = ov::as_type_ptr<ov::op::v0::Constant>(node))

ptrs.push_back(c->get_data_ptr());
}
ASSERT_EQ(ptrs.size(), 2);
ASSERT_NE(ptrs[0], ptrs[1]);

const auto& [success, message] = compare_functions(model_initial, model_imported, true, true, false, true, true);
ASSERT_TRUE(success) << message;
}
Expand Down
32 changes: 32 additions & 0 deletions src/core/tests/pass/serialization/serialize.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "openvino/core/graph_util.hpp"
#include "openvino/core/version.hpp"
#include "openvino/op/add.hpp"
#include "openvino/op/convert.hpp"
#include "openvino/pass/serialize.hpp"
#include "openvino/runtime/core.hpp"
#include "openvino/runtime/tensor.hpp"
Expand Down Expand Up @@ -917,4 +918,35 @@ TEST_F(UndefinedTypeDynamicTypeSerializationTests, compare_dynamic_type_undefine
ASSERT_TRUE(files_equal(m_dynamic_type_out_xml_path, m_undefined_type_out_xml_path))
<< "Serialized XML files are different: dynamic type vs undefined type";
}
TEST_F(SerializePassTest, constant_data_pointers_are_aligned) {
// bool (1 B), i32 (4 B), i64 (8 B) serialised consecutively.
// Without alignment padding the i64 lands at offset 5, which is UB.
auto param = std::make_shared<Parameter>(element::f32, Shape{1});

auto bc = std::make_shared<Constant>(element::boolean, Shape{}, std::vector<char>{0});
auto ic4 = std::make_shared<Constant>(element::i32, Shape{}, std::vector<int32_t>{1});
auto ic8 = std::make_shared<Constant>(element::i64, Shape{}, std::vector<int64_t>{2});

auto cvt_b = std::make_shared<op::v0::Convert>(bc, element::f32);
auto cvt_i4 = std::make_shared<op::v0::Convert>(ic4, element::f32);
auto cvt_i8 = std::make_shared<op::v0::Convert>(ic8, element::f32);

auto a1 = std::make_shared<op::v1::Add>(param, cvt_b);
auto a2 = std::make_shared<op::v1::Add>(a1, cvt_i4);
auto a3 = std::make_shared<op::v1::Add>(a2, cvt_i8);

m_model = std::make_shared<Model>(OutputVector{a3}, ParameterVector{param});
pass::Serialize(m_out_xml_path, m_out_bin_path).run_on_model(m_model);

@praasz praasz Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use ov::save_model the xml_path is enough the bin file will be created with same name.


auto reloaded = test::readModel(m_out_xml_path, m_out_bin_path);
for (auto& node : reloaded->get_ops()) {
auto c = std::dynamic_pointer_cast<Constant>(node);
if (!c || c->get_element_type() == element::string)
continue;
Comment on lines +943 to +945

@praasz praasz Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove continue

Suggested change
auto c = std::dynamic_pointer_cast<Constant>(node);
if (!c || c->get_element_type() == element::string)
continue;
if (auto c = ov::as_type_ptr<Constant>(node); !c || check type) {
...
}

Why string is removed from testing?

auto alignment = c->get_element_type().size();

@t-jankowski t-jankowski Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alignment value on writing is unconditional, while here it depends on type size. Would it pass without above changes in ConstantWriter::write?

@LeoHeller LeoHeller Jul 6, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it would fail without the fix. it lands at offset 5 without the fix, and 5 % 8 != 0.

@LeoHeller LeoHeller Jul 6, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@praasz praasz Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using size() will make more sense if this test will parametrized TEST_P and we created test model with different types of constant to check each type is serialized with aligned address.

Then we can validate all precision

auto ptr = c->get_data_ptr();
EXPECT_EQ(reinterpret_cast<uintptr_t>(ptr) % alignment, 0)
<< "Constant " << c->get_friendly_name() << " data pointer is not aligned to " << alignment << " bytes";
Comment on lines +948 to +949

@praasz praasz Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
EXPECT_EQ(reinterpret_cast<uintptr_t>(ptr) % alignment, 0)
<< "Constant " << c->get_friendly_name() << " data pointer is not aligned to " << alignment << " bytes";
EXPECT_EQ(reinterpret_cast<uintptr_t>(ptr) % alignment, 0)
<< c << " data pointer is not aligned to " << alignment << " bytes";

It will give more information about failing node

}
}
} // namespace ov::test
Loading