Skip to content

Clean up serializer class#1353

Merged
feldergast merged 2 commits intosstsimulator:develfrom
leekillough:cleanup_serializer
Jun 25, 2025
Merged

Clean up serializer class#1353
feldergast merged 2 commits intosstsimulator:develfrom
leekillough:cleanup_serializer

Conversation

@leekillough
Copy link
Contributor

@leekillough leekillough commented Jun 7, 2025

This PR cleans up the serializer class:

  • The current mode is controlled by a std::variant<pvt::ser_sizer, pvt::ser_packer, pvt::ser_unpacker, pvt::ser_mapper> ser_ member, each variant of which holds all data necessary for the current mode.

  • The variants have indices of SIZER, PACK, UNPACK, and MAP, and the current mode is the one returned by ser_.index().

  • The serializer data for a mode is accessed like std::get<UNPACK>(ser_), where ser_ is the std::variant member.

  • Since their names are shorter, and they already existed, the accessors sizer(), packer(), unpacker() and mapper() are used everywhere, and they call std:get<MODE>(ser_). What was previously sizer_ is now sizer().

  • If a method is called which is not valid for the current mode, a std::bad_variant_access exception is thrown. This
    error checking is automatically provided by std::get<MODE>(std::variant).

  • All pointer tracking data structures, such as ser_pointer_set and ser_pointer_map, have been moved to pvt::sizer, pvt::ser_packer, pvt::ser_unpacker, and pvt:ser_mapper, so that they are private to each mode, and are automatically initialized/destroyed when the mode changes, without requiring explicit initialization or destruction/clearing in the serializer class. This was motivated by std::shared_ptr tracking requiring additional tracking containers, and requiring different ones for different serialization modes.

  • set_mode() is marked deprecated, because it was not very useful, was not used anywhere in Core or Elements, and in the new code, we cannot change the mode without changing and initializing the new variant. set_mode() will still change the mode, but the variant of the new mode will be default-initialized, which means pvt::ser_packer will have a max_size of 0, etc. There was no way in the old interface to change max_size except start_packing(), and if you changed modes, some of the settings could be uninitialized or have bad data from the old mode. Now switching modes is clean, and data for each mode is clearly separated.

  • reset() is not used anywhere either, and is marked deprecated.

  • If it is not important that we keep set_mode() and reset(), I vote to remove them immediately.

  • start_sizing(), start_packing(), start_unpacking(), and start_mapping() change the active mode, and
    initialize the mode's data structures by calling the variant's constructor.

  • The .init() methods of the pvt:: serializers have been replaced by constructors. For example,

    void start_unpacking(char* buffer, size_t size) { ser_.emplace<UNPACK>(buffer, size); }

In this example, start_unpacking() changes the mode to UNPACK, automatically calling the destructors of any data in the previous mode, and calls pvt::ser_unpacker's constructor with buffer and size arguments, instead of
separately calling an .init() method.

  • pvt::ser_packer and pvt::ser_unpacker, which are derived from ser_buffer_accessor, use C++11 inherited constructors so that the pvt::ser_packer(buffer, size) and pvt::ser_unpacker(buffer, size) constructors are automatically created, and pass their arguments to the inherited ser_buffer_accessor constructor.

  • Non-templated serialization functions which aren't very short were moved from serializer.h to serializer.cc.

  • To avoid having to overload sizer(), packer(), unpacker() and mapper() with const versions in addition to their non-const versions, const has been removed from the size() method, which was the only method which used const on *this. Since almost all uses of size() will be ser.size() where ser is a non-const reference to a serializer (all cases in Core and Elements so far), this will not cause a problem. Only if you had const serializer& ser and needed to call ser.size() would a const method of size() be needed. If this is not acceptable, we can either duplicate the accessors with const versions, or we can change size() to not use the accessors but to use std::get<>(ser_) directly. I like the accessors as being a single place where the variants are extracted, and they replace names like sizer_ with sizer(), packer_ with packer(), etc.

  • check_pointer_pack() was split into check_pointer_pack() and check_pointer_sizer(), and the std::set was moved from serializer into pvt::ser_sizer and pvt::ser_packer, so that SIZE and PACK modes have their own pointer tracking sets, and the right check_pointer_* for the current mode is called.

  • The maps used in mapping mode and unpacking mode were moved to pvt::ser_mapper and pvt::ser_unpacker, and although the map has the same declaration in both, the way the map is used is totally different in unpacking and mapping mode. Best to separate them.

  • The new code is 40+ lines shorter and much cleaner. Adding serialization of std::shared_ptr and std::unique_ptr will be simpler since now the tracking data is separately in each mode's pvt:: class, and is cleanly initialized/destroyed when switching modes.

Edit 6/10:

  • EMPTY has been added as the default serializer mode.
  • A std::monostate type has been added as the default EMPTY type of the std::variant.
  • Any attempt to access sizer(), packer(), unpacker() or mapper() during a different mode than SIZER, PACK ,UNPACK or MAP, respectively, causes a runtime error.
  • The set_mode() and reset() methods have been removed.
  • A finalize() method has been added which, right now, simply sets the mode to EMPTY and destroys any state object of the old mode. Later it might be changed to do more, such as considering the current mode and doing something special for it, such as finalization of mapping mode.
  • In past discussions, a comment has been made that switching to SIZER mode is sometimes used to clear the old state; however, SIZER mode has its own state (a std::set which is empty on construction). An explicit EMPTY mode separate from other modes is preferable.
  • To prevent warnings about EMPTY not being listed in switch cases, EMPTY has been defined outside of the enum.
  • All pvt::ser_packer functions defined in serializer.cc have been moved to a new file impl/packer.cc.
  • All pvt::ser_unpacker functions defined in serializer.cc have been moved to a new file impl/unpacker.cc
  • The ser_buf_accessor has been made much simpler, with a single method buf_next(size) which returns the buffer address and advances it by size, making sure that it does not overflow. This simplifies the multiple methods which were used before, such as special methods for std::string.

@github-actions github-actions bot added AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) AT: CLANG-FORMAT PASS AT: CMAKE-FORMAT PASS and removed AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) labels Jun 7, 2025
@github-actions
Copy link

github-actions bot commented Jun 7, 2025

CMAKE-FORMAT TEST - PASSED

@github-actions
Copy link

github-actions bot commented Jun 7, 2025

CLANG-FORMAT TEST - PASSED

@sst-autotester
Copy link
Contributor

Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging
NO INSPECTION HAS BEEN PERFORMED ON THIS PULL REQUEST! - This PR must be inspected by setting label 'AT: PRE-TEST INSPECTED'.

@leekillough leekillough force-pushed the cleanup_serializer branch from fbbd165 to 3d23134 Compare June 7, 2025 14:50
@github-actions github-actions bot added AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) AT: CLANG-FORMAT FAIL and removed AT: CLANG-FORMAT PASS labels Jun 7, 2025
@github-actions
Copy link

github-actions bot commented Jun 7, 2025

CLANG-FORMAT TEST - FAILED (on last commit):
Run > ./scripts/clang-format-test.sh using clang-format v20 to check formatting

@github-actions
Copy link

github-actions bot commented Jun 7, 2025

CMAKE-FORMAT TEST - PASSED

@github-actions github-actions bot removed the AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) label Jun 7, 2025
@sst-autotester
Copy link
Contributor

Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging
NO INSPECTION HAS BEEN PERFORMED ON THIS PULL REQUEST! - This PR must be inspected by setting label 'AT: PRE-TEST INSPECTED'.

@leekillough leekillough force-pushed the cleanup_serializer branch from 3d23134 to 982c11e Compare June 7, 2025 14:53
@github-actions github-actions bot added AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) AT: CLANG-FORMAT PASS and removed AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) AT: CLANG-FORMAT FAIL labels Jun 7, 2025
@github-actions
Copy link

github-actions bot commented Jun 7, 2025

CLANG-FORMAT TEST - PASSED

@github-actions
Copy link

github-actions bot commented Jun 7, 2025

CMAKE-FORMAT TEST - PASSED

@leekillough leekillough force-pushed the cleanup_serializer branch from 982c11e to 9d06c0e Compare June 7, 2025 14:58
@github-actions github-actions bot added AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) and removed AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) labels Jun 7, 2025
@github-actions
Copy link

github-actions bot commented Jun 7, 2025

CLANG-FORMAT TEST - PASSED

@github-actions
Copy link

github-actions bot commented Jun 7, 2025

CMAKE-FORMAT TEST - PASSED

@leekillough leekillough force-pushed the cleanup_serializer branch from 9d06c0e to b079687 Compare June 7, 2025 18:04
@github-actions github-actions bot added AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) and removed AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) labels Jun 7, 2025
@github-actions
Copy link

CLANG-FORMAT TEST - PASSED

@github-actions github-actions bot removed the AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) label Jun 17, 2025
@github-actions
Copy link

CMAKE-FORMAT TEST - PASSED

@sst-autotester
Copy link
Contributor

Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging
NO INSPECTION HAS BEEN PERFORMED ON THIS PULL REQUEST! - This PR must be inspected by setting label 'AT: PRE-TEST INSPECTED'.

@github-actions github-actions bot added AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) and removed AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) labels Jun 17, 2025
@github-actions
Copy link

CLANG-FORMAT TEST - PASSED

@github-actions
Copy link

CMAKE-FORMAT TEST - PASSED

@sst-autotester
Copy link
Contributor

Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging
NO INSPECTION HAS BEEN PERFORMED ON THIS PULL REQUEST! - This PR must be inspected by setting label 'AT: PRE-TEST INSPECTED'.

@leekillough
Copy link
Contributor Author

UNSTARTED is the name of the default mode of the serializer, and finalize() changes the mode back to UNSTARTED, destroying any tracking object which holds state for the current mode.

For std::shared_ptr, finalize() will need to be called after all unpacking is done, so that all of the copies of std::shared_ptr owners stored in pvt::ser_unpacker are destroyed at the end of deserialization, and the reference counts of the std::shared_ptr will be the same as they were when they were serialized. Otherwise the reference counts will be +1 because of an extra copy in pvt::ser_unpacker. The copy is needed throughout the entire deserialization sequence so that std::shared_ptr and std::weak_ptr can be deserialized with the same shared ownership relationships that they had during serialization.

@github-actions github-actions bot added AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) and removed AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) labels Jun 23, 2025
@github-actions
Copy link

CLANG-FORMAT TEST - PASSED

@github-actions github-actions bot added AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) and removed AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) labels Jun 23, 2025
@github-actions
Copy link

CMAKE-FORMAT TEST - PASSED

@sst-autotester
Copy link
Contributor

Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging
NO INSPECTION HAS BEEN PERFORMED ON THIS PULL REQUEST! - This PR must be inspected by setting label 'AT: PRE-TEST INSPECTED'.

@sst-autotester
Copy link
Contributor

Status Flag 'Pre-Test Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED by label AT: PRE-TEST INSPECTED! Autotester is Removing Label; this inspection will remain valid until a new commit to source branch is performed.

@sst-autotester
Copy link
Contributor

Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects:

Pull Request Auto Testing STARTING (click to expand)

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements

  • Build Num: 2101
  • Status: STARTED

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements_MR-2

  • Build Num: 2057
  • Status: STARTED

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements_MT-2

  • Build Num: 2056
  • Status: STARTED

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-core_Make-Dist

  • Build Num: 822
  • Status: STARTED

Build Information

Test Name: SST__AutotestGen2_NewFW_OSX-14-XC15-ARM2_OMPI-4.1.6_PY3.10_sst-elements

  • Build Num: 612
  • Status: STARTED

Using Repos:

Repo: CORE (leekillough/sst-core)
  • Branch: cleanup_serializer
  • SHA: 147192a
  • Mode: TEST_REPO
Repo: SQE (sstsimulator/sst-sqe)
  • Branch: devel
  • SHA: bb4b04da3cbca0dbf36fff9d79593f5f1db96034
  • Mode: SUPPORT_REPO
Repo: ELEMENTS (sstsimulator/sst-elements)
  • Branch: devel
  • SHA: 958a2e069e079e69996ec262cb2ce331a564c43b
  • Mode: SUPPORT_REPO
Repo: MACRO (sstsimulator/sst-macro)
  • Branch: devel
  • SHA: 31e2c16aa4d07b502bcd9c97b1872a0329ed0b38
  • Mode: SUPPORT_REPO

Pull Request Author: leekillough

@sst-autotester
Copy link
Contributor

Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED

Pull Request Auto Testing has PASSED (click to expand)

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements

  • Build Num: 2101
  • Status: PASSED

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements_MR-2

  • Build Num: 2057
  • Status: PASSED

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements_MT-2

  • Build Num: 2056
  • Status: PASSED

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-core_Make-Dist

  • Build Num: 822
  • Status: PASSED

Build Information

Test Name: SST__AutotestGen2_NewFW_OSX-14-XC15-ARM2_OMPI-4.1.6_PY3.10_sst-elements

  • Build Num: 612
  • Status: PASSED

@sst-autotester
Copy link
Contributor

Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ feldergast ]!

@sst-autotester
Copy link
Contributor

Status Flag 'Pull Request AutoTester' - Pull Request MUST BE MERGED MANUALLY BY Project Team - This Repo does not support Automerge

@feldergast feldergast merged commit 5f87e5d into sstsimulator:devel Jun 25, 2025
7 checks passed
@leekillough leekillough deleted the cleanup_serializer branch October 5, 2025 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants