Skip to content

[Core] Fix string Constant serialization incorrect ConstantWriter deduplication#34955

Open
p-wysocki wants to merge 13 commits intoopenvinotoolkit:masterfrom
p-wysocki:serialization_bug
Open

[Core] Fix string Constant serialization incorrect ConstantWriter deduplication#34955
p-wysocki wants to merge 13 commits intoopenvinotoolkit:masterfrom
p-wysocki:serialization_bug

Conversation

@p-wysocki
Copy link
Copy Markdown
Contributor

Details:

  • String constant blobs (header + raw string bytes) were written as separate ConstantWriter::write() calls
  • ConstantWriter deduplicates by data hash: a string shared between two constants (ex. "UNKNOWN") would be deduped - write() returned the earlier file position instead of appending bytes into the current constant's region, while the header's intra-blob offsets were still computed as if the strings followed contiguously after the header
  • On read, the offsets resolved to unrelated bytes within the constant's own blob region, producing corrupted deserialization
  • Fix: pack header + all strings into one std::vector<char>, single write() call, deduplication is now all or nothing per string constant

Tickets:

AI Assistance:

  • AI assistance used: yes
  • Understanding how Constant depopulation works and adding test cases

Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
@p-wysocki p-wysocki added this to the 2026.2 milestone Mar 26, 2026
@p-wysocki p-wysocki requested review from barnasm1, olpipi and praasz March 26, 2026 12:31
@p-wysocki p-wysocki requested a review from a team as a code owner March 26, 2026 12:31
@github-actions github-actions bot added the category: Core OpenVINO Core (aka ngraph) label Mar 26, 2026
num_elements = a2->get()->get_num_elements();
}

std::vector<char> packed(header_size);
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.

This create memory use over head.
If constant is big we have 2x memory usage

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Corrected, now the strings are being compared using pointers and combined hash instead of copying them into a container.

Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
@p-wysocki p-wysocki requested review from a team as code owners March 30, 2026 11:54
@github-actions github-actions bot added category: CPU OpenVINO CPU plugin category: NPU OpenVINO NPU plugin labels Mar 30, 2026
Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes incorrect IR serialization/deserialization of ov::element::string constants when ConstantWriter deduplication is enabled, by ensuring the packed-string header and the raw string bytes are written atomically (so deduplication cannot occur mid-blob and invalidate intra-blob offsets).

Changes:

  • Add a scatter/gather write API (ConstantWriter::write_scatter) and use it for string-constant serialization to write header + strings as one logical blob.
  • Update plugin-local WeightlessWriter implementations to override the new write path.
  • Add core regression tests covering round-trip of multiple string constants (including shared strings) and identical string constants.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/plugins/intel_npu/src/compiler_adapter/include/xml_serializer.hpp Extend NPU WeightlessWriter to override write_scatter (weightless serialization path).
src/plugins/intel_cpu/src/utils/graph_serializer/serializer.cpp Extend CPU WeightlessWriter to override write_scatter and handle skip-weights mode.
src/core/tests/pass/serialization/const_compression.cpp Add regression tests for string constant serialization round-trips.
src/core/src/xml_util/xml_serialize_util.cpp Switch string-constant serialization to a single write_scatter call (header + all strings).
src/core/src/xml_util/constant_writer.cpp Implement ConstantWriter::write_scatter.
src/core/dev_api/openvino/xml_util/constant_writer.hpp Add Chunk type and declare new virtual write_scatter API.

Comment on lines +74 to +95
// Compute a combined hash over all chunks in sequence
HashValue hash = 0;
for (const auto& chunk : chunks) {
hash = util::u64_hash_combine(hash, ov::runtime::compute_hash(chunk.data, chunk.size));
}

// Check whether an identical contiguous blob was written before
const auto found = m_hash_to_file_positions.equal_range(hash);
for (auto it = found.first; it != found.second; ++it) {
const char* stored = static_cast<const char*>(it->second.second);
bool match = true;
for (const auto& chunk : chunks) {
if (memcmp(chunk.data, stored, chunk.size) != 0) {
match = false;
break;
}
stored += chunk.size;
}
if (match) {
return it->second.first;
}
}
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

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

[HIGH] write_scatter() deduplication currently can't work: it computes a per-chunk combined hash that doesn't match the write() hash scheme, and the function never inserts the newly written blob into m_hash_to_file_positions, so later write_scatter() calls will never find a match. Consider either (a) implementing scatter dedupe by hashing the concatenated byte sequence and storing an owned contiguous copy for comparisons, or (b) dropping dedupe logic from write_scatter() and using a single contiguous write() from the serializer if dedupe isn't required for string constants.

Suggested change
// Compute a combined hash over all chunks in sequence
HashValue hash = 0;
for (const auto& chunk : chunks) {
hash = util::u64_hash_combine(hash, ov::runtime::compute_hash(chunk.data, chunk.size));
}
// Check whether an identical contiguous blob was written before
const auto found = m_hash_to_file_positions.equal_range(hash);
for (auto it = found.first; it != found.second; ++it) {
const char* stored = static_cast<const char*>(it->second.second);
bool match = true;
for (const auto& chunk : chunks) {
if (memcmp(chunk.data, stored, chunk.size) != 0) {
match = false;
break;
}
stored += chunk.size;
}
if (match) {
return it->second.first;
}
}
// Compute a combined hash over all chunks in sequence and fold it into m_data_hash.
// Note: deduplication against previously written blobs is intentionally not performed here.
HashValue hash = 0;
for (const auto& chunk : chunks) {
hash = util::u64_hash_combine(hash, ov::runtime::compute_hash(chunk.data, chunk.size));
}

Copilot uses AI. Check for mistakes.
ov::element::Type src_type = ov::element::dynamic,
bool ptr_is_temporary = false);

virtual FilePosition write_scatter(const std::vector<Chunk>& chunks, size_t& new_size);
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

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

[BLOCKER] Adding a new virtual method (write_scatter) to OPENVINO_API-exported ov::util::ConstantWriter is an ABI-breaking change (vtable layout changes) for any out-of-tree code inheriting from it via the developer package headers. If ABI compatibility is required here, prefer a non-virtual helper (free function) or implement the string-constant fix in the serializer by assembling a contiguous buffer and calling the existing write() API.

Suggested change
virtual FilePosition write_scatter(const std::vector<Chunk>& chunks, size_t& new_size);
FilePosition write_scatter(const std::vector<Chunk>& chunks, size_t& new_size);

Copilot uses AI. Check for mistakes.
Comment on lines +54 to +63
new_size = 0;
for (const auto& chunk : chunks) {
new_size += chunk.size;
}

if (m_skip_weights) {
FilePosition offset = m_offset;
m_offset += new_size;
return offset;
}
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

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

[HIGH] In WeightlessWriter::write_scatter(), when m_skip_weights is true, new_size is left as the summed chunk size, unlike write() which sets new_size = 0 when skipping. This risks emitting a non-zero size attribute for weightless constants even though no bytes are written. Consider preserving the write() semantics: compute the total skipped size for offset accounting, but report new_size = 0 to the caller when skipping.

Suggested change
new_size = 0;
for (const auto& chunk : chunks) {
new_size += chunk.size;
}
if (m_skip_weights) {
FilePosition offset = m_offset;
m_offset += new_size;
return offset;
}
size_t total_size = 0;
for (const auto& chunk : chunks) {
total_size += chunk.size;
}
if (m_skip_weights) {
FilePosition offset = m_offset;
m_offset += total_size;
new_size = 0;
return offset;
}
new_size = total_size;

Copilot uses AI. Check for mistakes.
ov::element::Type src_type = ov::element::dynamic,
bool ptr_is_temporary = false);

virtual FilePosition write_scatter(const std::vector<Chunk>& chunks, size_t& new_size);
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
virtual FilePosition write_scatter(const std::vector<Chunk>& chunks, size_t& new_size);
virtual FilePosition write(const std::vector<std::string_view>& chunks, size_t& new_size);

Or if is possible make special version for string buffer?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Implemented.

Comment on lines +87 to +88
match = false;
break;
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.

Should be return instead break?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Logic moved, I think this comment is no longer relevant

Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category: Core OpenVINO Core (aka ngraph) category: CPU OpenVINO CPU plugin category: NPU OpenVINO NPU plugin

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants