odb: Modernize dbOStream and dbIStream with C++20 concepts and 64KB buffering - #11190
odb: Modernize dbOStream and dbIStream with C++20 concepts and 64KB buffering#11190debayanbandyopadhyay wants to merge 2 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modernizes the database streaming classes dbOStream and dbIStream by introducing buffering, using C++20 concepts and templates for arithmetic types, and utilizing std::string_view to avoid unnecessary allocations. It also adds a comprehensive suite of unit tests for these streams. The review feedback correctly identifies that the new template constraints on operator<< and operator>> exclude enums (such as dbObjectType), which will cause compilation failures, and suggests expanding the constraints to include std::is_enum_v<T>. Additionally, a syntax error was found in src/odb/test/cpp/CMakeLists.txt due to a stray line.
36affd5 to
5849dde
Compare
…uffering - Add 64KB internal buffering in dbOStream for scalar types. - Replace SFINAE with C++20 requires clauses (including enums). - Use std::span and std::string_view for zero-copy streaming. - Add TestDbStream unit test. Signed-off-by: Debayan Bandyopadhyay <dbandyopadhyay@google.com>
5849dde to
ad2a743
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad2a743606
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| using Position = std::ostream::pos_type; | ||
|
|
||
| dbOStream(_dbDatabase* db, std::ostream& f); | ||
| ~dbOStream() { flush(); } |
There was a problem hiding this comment.
When the underlying stream throws on write, any final payload smaller than 64 KB is now written by ~dbOStream(). Destructors are implicitly noexcept, so this exception calls std::terminate instead of propagating; dbDatabase::writeEco enables failbit | badbit exceptions and relies on this destructor flush, making disk-full or other I/O failures an uncatchable process abort. Flush explicitly before destruction, as dbDatabase::write now does, and avoid throwing from the destructor.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done. Wrapped flush() in ~dbOStream() with a try-catch block to prevent exceptions from propagating and causing aborts. Added explicit stream.flush() calls in dbDatabase::write, dbDatabase::writeEco, and relevant test cases to ensure any I/O errors during write are still caught and handled when it is safe.
| add_executable(TestSwapMasterUnusedPort TestSwapMasterUnusedPort.cpp) | ||
| add_executable(TestWriteReadDbHier TestWriteReadDbHier.cpp) | ||
| add_executable(TestObjectType TestObjectType.cpp) | ||
| add_executable(TestDbStream TestDbStream.cpp) |
There was a problem hiding this comment.
The new executable is declared only in CMake: a repo-wide search finds no TestDbStream target in src/odb/test/cpp/BUILD and no entry in src/odb/test/BUILD's cpp_tests suite. Consequently Bazel CI neither builds nor runs this regression; add the corresponding cc_test target and suite entry.
AGENTS.md reference: AGENTS.md:L28-L31
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done. Added the TestDbStream cc_test target definition to src/odb/test/cpp/BUILD.
maliberty
left a comment
maliberty
left a comment
There was a problem hiding this comment.
Thanks this code is quite old.
|
|
||
| dbOStream& operator<<(int64_t c) | ||
| void write_bytes(std::span<const char> bytes) |
There was a problem hiding this comment.
Match the surrounding naming style writeBytes
There was a problem hiding this comment.
Done. Renamed write_bytes to writeBytes in dbStream.h and updated all usages in tests.
| } | ||
|
|
||
| dbOStream& operator<<(uint16_t c) | ||
| void flush() const |
There was a problem hiding this comment.
flush() is not const on an iostream and I don't think it should be there. I think that would allow buffer_pos_ to not require mutable
There was a problem hiding this comment.
Done. Made flush() and pos() non-const, and removed the mutable qualifier from buffer_pos_.
There was a problem hiding this comment.
Please add the test to BUILD for bazel
There was a problem hiding this comment.
Done. Added the test to the cpp_tests suite in src/odb/test/BUILD and defined the cc_test target in src/odb/test/cpp/BUILD.
f70b561 to
3f9be03
Compare
…rite_bytes Signed-off-by: Debayan Bandyopadhyay <dbandyopadhyay@google.com>
3f9be03 to
641f596
Compare
2 participants
Summary
This PR modernizes the database stream classes (
dbOStreamanddbIStream) inodbusing modern C++20 features and introduces a 64KB internal buffer fordbOStreamto improve serialization performance duringwrite_db:dbOStreaminto a 64KB buffer to minimize underlyingstd::ostreamoverhead, while allowing large byte payloads to bypass directly.requires): Replaces verbose SFINAE (std::enable_if_t) with explicit C++20requiresclauses for cleaner compile diagnostics.std::span<const char>for raw byte writes andstd::string_viewfor string serialization to avoid temporarystd::stringallocations.TestDbStream.cppwith comprehensive test coverage for fundamental types and standard containers (std::vector,std::map,std::tuple,std::variant).Type of Change
Impact
write_dbruntime by ~15% to 30% across flow checkpoints (synthesis, floorplan, placement).Verification
./etc/Build.sh).TestDbStream,TestObjectType, and ODB regressions).src/odb/test/cpp/TestDbStream.cpp).Related Issues
N/A