Serialization {read,write}_trivial should take internal_only argument

Dan Bonachea

I spent some time looking at this and these two function are unfortunately just the tip of the iceberg.

The deliberately underspecified Reader/Writer classes (detail::serialization_{reader,writer}) live in the upcxx::detail namespace, but they are passed to user-provided custom serialization functions. The problem is they declare many deliberately unspecified member functions that are used internally in performance-critical paths, but are not currently protected or tagged in any way. Because these objects are passed to custom serialization functions, user code erroneously invoking these unspecified member functions need not even mention the detail namespace. Adding detail::internal_only arguments to all of these functions (our conventional approach) is ugly and makes the often-subtle serialization code harder to read. Also, I'm not fully convinced that optimizers would be smart enough to remove all the overhead associated with all the additional dead internal_only objects injected along the critical paths of Serialization.

I think there's also a non-zero risk that ExaBiome may have code currently relying on some of these unspecified internals.

I have some ideas on how to do this better, but it would not be a low-risk change so should probably be deferred to post-release. We should probably look at this during next release cycle, especially if we roll this together with extending the Serialization API to better support @Rob Egan 's use cases.

2022-03-23T19:41:50+00:00

Comments (7)

Dan Bonachea
- assigned issue to
  
  Colin MacLean
- 2021-09-14T02:35:43+00:00
Dan Bonachea
- changed milestone to 2022.3.0 release
Mass roll-over of open issues to next release milestone
- 2021-10-09T03:31:53+00:00
Dan Bonachea
I spent some time looking at this and these two function are unfortunately just the tip of the iceberg.

The deliberately underspecified Reader/Writer classes (detail::serialization_{reader,writer}) live in the upcxx::detail namespace, but they are passed to user-provided custom serialization functions. The problem is they declare many deliberately unspecified member functions that are used internally in performance-critical paths, but are not currently protected or tagged in any way. Because these objects are passed to custom serialization functions, user code erroneously invoking these unspecified member functions need not even mention the detail namespace. Adding detail::internal_only arguments to all of these functions (our conventional approach) is ugly and makes the often-subtle serialization code harder to read. Also, I'm not fully convinced that optimizers would be smart enough to remove all the overhead associated with all the additional dead internal_only objects injected along the critical paths of Serialization.

I think there's also a non-zero risk that ExaBiome may have code currently relying on some of these unspecified internals.

I have some ideas on how to do this better, but it would not be a low-risk change so should probably be deferred to post-release. We should probably look at this during next release cycle, especially if we roll this together with extending the Serialization API to better support @Rob Egan 's use cases.
- 2022-03-23T19:41:50+00:00
Dan Bonachea
- changed milestone to 2022.9.0 release
Mass roll-over of open issues to next release milestone
- 2022-04-06T18:15:08+00:00
Dan Bonachea
- changed milestone to 2023.3.0 release
Mass roll-over of open issues to next release milestone
- 2022-10-21T04:52:27+00:00
Dan Bonachea
- changed milestone to 2023.9.0 release
deferred until after pending Serialization Stream work
- 2022-11-01T21:41:51+00:00
Dan Bonachea
- removed milestone
Clear past Milestone for open issues
- 2024-01-03T22:19:57+00:00
Log in to comment