Remove serialization non C.41 constructors from fragment_metadata_from_capnp #4682

teo-tsirpanis · 2024-01-29T22:48:00Z

TYPE: NO_HISTORY
DESC: Refactor the FragmentMetadata class to reduce mutable state.

shortcut-integration · 2024-01-29T22:48:04Z

This pull request has been linked to Shortcut Story #40131: Remove serialization non C.41 constructors from fragment_metadata_from_capnp..

… qualifiers and return small values by value.

tiledb/sm/serialization/fragment_metadata.cc

…t metadata in global order writer.

KiterLuc · 2024-01-31T13:36:24Z

@teo-tsirpanis let's not work more on this PR. I started to review and it became obvious to me we need to design something different here. The 30 parameters constructor doesn't look good.

teo-tsirpanis · 2024-01-31T17:04:13Z

Undrafting, regardless of whether we will take it, it is finished and ready for review.

tiledb/sm/query/strategy_base.h

tiledb/sm/serialization/fragment_metadata.cc

tiledb/sm/serialization/fragment_metadata.h

eric-hughes-tiledb · 2024-02-14T18:21:03Z

tiledb/sm/fragment/fragment_metadata.h

@@ -727,7 +775,9 @@ class FragmentMetadata {
  URI validity_uri(const std::string& name) const;

  /** Return the array schema name. */
-  const std::string& array_schema_name();
+  const std::string& array_schema_name() const {


Should be declared inline. Not always necessary with modern code generation, but it remains valuable for its documentary value.

eric-hughes-tiledb · 2024-02-14T18:21:50Z

tiledb/sm/fragment/fragment_metadata.h

-  /** gt_offsets_ accessor */
-  inline GenericTileOffsets& generic_tile_offsets() {
-    return gt_offsets_;
+  const shared_ptr<const ArraySchema>& array_schema() const {


eric-hughes-tiledb · 2024-02-14T18:50:42Z

tiledb/sm/query/strategy_base.cc

@@ -61,6 +63,10 @@ StrategyBase::StrategyBase(
    , offsets_bitsize_(constants::cell_var_offset_size * 8) {
 }

+ContextResources& StrategyBase::resources() const {


This should be inline in the header.

I initially did it this way because moving the function to the header would include storage_manager.h, but now done.

eric-hughes-tiledb · 2024-02-14T19:14:56Z

tiledb/sm/serialization/fragment_metadata.h

 */
-Status fragment_metadata_from_capnp(
+shared_ptr<FragmentMetadata> fragment_metadata_from_capnp(


It would be much better to have this function return just FragmentMetadata and have the caller allocate it in shared_ptr if it needs to. I looked at its uses, and it looks like at least some of them don't need to allocate (usually because they already allocate, say, within vector).

We don't want to expand the scope of the present PR to change all the classes where there's spurious allocation. On the other hand we don't want to change this function signature and not get it write. The change requires only a handful of make_shared calls at the call sites of this function.

This cannot happen as long as tiledb::common::pmr::vector (and consequently FragmentMetadata) is not move-constructible.

@teo-tsirpanis can we change the return value here now that we changed the constructor?

I changed the return type and restored the move constructors on FragmentMetadata and got errors that a copy constructor is deleted. All members of the class are move-constructible; I have no idea why it fails.

Patch applied

diff --git a/tiledb/sm/fragment/fragment_metadata.h b/tiledb/sm/fragment/fragment_metadata.h index 92dcea4..0637d4d 100644 --- a/tiledb/sm/fragment/fragment_metadata.h +++ b/tiledb/sm/fragment/fragment_metadata.h @@ -152,7 +152,9 @@ class FragmentMetadata { ~FragmentMetadata(); DISABLE_COPY_AND_COPY_ASSIGN(FragmentMetadata); - DISABLE_MOVE_AND_MOVE_ASSIGN(FragmentMetadata); + + FragmentMetadata(FragmentMetadata&&) = default; + FragmentMetadata& operator=(FragmentMetadata&&) = default; /* ********************************* */ /* TYPE DEFINITIONS */ diff --git a/tiledb/sm/serialization/array.cc b/tiledb/sm/serialization/array.cc index 8957209..813851b 100644 --- a/tiledb/sm/serialization/array.cc +++ b/tiledb/sm/serialization/array.cc @@ -299,11 +299,13 @@ Status array_from_capnp( auto fragment_metadata_all_reader = array_reader.getFragmentMetadataAll(); fragment_metadata.reserve(fragment_metadata_all_reader.size()); for (auto frag_meta_reader : fragment_metadata_all_reader) { - auto meta = fragment_metadata_from_capnp( - array->array_schema_latest_ptr(), - frag_meta_reader, - &storage_manager->resources(), - array->memory_tracker()); + auto meta = make_shared<FragmentMetadata>( + HERE(), + fragment_metadata_from_capnp( + array->array_schema_latest_ptr(), + frag_meta_reader, + &storage_manager->resources(), + array->memory_tracker())); if (client_side) { meta->set_rtree_loaded(); } diff --git a/tiledb/sm/serialization/fragment_info.cc b/tiledb/sm/serialization/fragment_info.cc index 66f39fd..4f6942b 100644 --- a/tiledb/sm/serialization/fragment_info.cc +++ b/tiledb/sm/serialization/fragment_info.cc @@ -228,11 +228,13 @@ single_fragment_info_from_capnp( "Missing fragment metadata from single fragment info capnp reader"), nullopt}; } - shared_ptr<FragmentMetadata> meta = fragment_metadata_from_capnp( - schema->second, - single_frag_info_reader.getMeta(), - fragment_info->resources(), - fragment_info->resources()->create_memory_tracker()); + auto meta = make_shared<FragmentMetadata>( + HERE(), + fragment_metadata_from_capnp( + schema->second, + single_frag_info_reader.getMeta(), + fragment_info->resources(), + fragment_info->resources()->create_memory_tracker())); auto expanded_non_empty_domain = meta->non_empty_domain(); if (meta->dense()) { diff --git a/tiledb/sm/serialization/fragment_metadata.cc b/tiledb/sm/serialization/fragment_metadata.cc index 3d828d4..fa10d7c 100644 --- a/tiledb/sm/serialization/fragment_metadata.cc +++ b/tiledb/sm/serialization/fragment_metadata.cc @@ -166,7 +166,7 @@ static FragmentMetadata::GenericTileOffsets generic_tile_offsets_from_capnp( return gt_offsets; } -shared_ptr<FragmentMetadata> fragment_metadata_from_capnp( +FragmentMetadata fragment_metadata_from_capnp( const shared_ptr<const ArraySchema>& array_schema, const capnp::FragmentMetadata::Reader& frag_meta_reader, ContextResources* resources, @@ -198,8 +198,7 @@ shared_ptr<FragmentMetadata> fragment_metadata_from_capnp( deserializer, &array_schema->domain(), constants::format_version); } - return make_shared<FragmentMetadata>( - HERE(), + return FragmentMetadata( resources, memory_tracker, array_schema, diff --git a/tiledb/sm/serialization/fragment_metadata.h b/tiledb/sm/serialization/fragment_metadata.h index d8f7da8..cc76f05 100644 --- a/tiledb/sm/serialization/fragment_metadata.h +++ b/tiledb/sm/serialization/fragment_metadata.h @@ -58,9 +58,9 @@ namespace serialization { * @param frag_meta_reader cap'n proto class * @param resources ContextResources associated * @param memory_tracker memory tracker associated - * @return shared pointer to deserialized FragmentMetadata + * @return deserialized FragmentMetadata */ -shared_ptr<FragmentMetadata> fragment_metadata_from_capnp( +FragmentMetadata fragment_metadata_from_capnp( const shared_ptr<const ArraySchema>& array_schema, const capnp::FragmentMetadata::Reader& frag_meta_reader, ContextResources* resources, diff --git a/tiledb/sm/serialization/query.cc b/tiledb/sm/serialization/query.cc index fda96c0..a76fcbb 100644 --- a/tiledb/sm/serialization/query.cc +++ b/tiledb/sm/serialization/query.cc @@ -2995,11 +2995,13 @@ Status global_write_state_from_capnp( write_state->last_hilbert_value_ = state_reader.getLastHilbertValue(); if (state_reader.hasFragMeta()) { - write_state->frag_meta_ = fragment_metadata_from_capnp( - query.array_schema_shared(), - state_reader.getFragMeta(), - &global_writer->resources(), - global_writer->resources().create_memory_tracker()); + write_state->frag_meta_ = make_shared<FragmentMetadata>( + HERE(), + fragment_metadata_from_capnp( + query.array_schema_shared(), + state_reader.getFragMeta(), + &global_writer->resources(), + global_writer->resources().create_memory_tracker())); } // Deserialize the multipart upload state @@ -3118,11 +3120,13 @@ Status unordered_write_state_from_capnp( // Fragment metadata is not allocated when deserializing into a new Query // object. if (unordered_writer->frag_meta() == nullptr) { - unordered_writer->set_frag_meta(fragment_metadata_from_capnp( - query.array_schema_shared(), - state_reader.getFragMeta(), - &unordered_writer->resources(), - unordered_writer->resources().create_memory_tracker())); + unordered_writer->set_frag_meta(make_shared<FragmentMetadata>( + HERE(), + fragment_metadata_from_capnp( + query.array_schema_shared(), + state_reader.getFragMeta(), + &unordered_writer->resources(), + unordered_writer->resources().create_memory_tracker()))); } }

auto meta = make_shared<FragmentMetadata>(... is written with = like an assignment, so it might be doing copy-initialization. Another possibility is that there's a copy happening with make_shared, since I don't see move being used to ask for the move constructor.

There are bigger hammers available with things like allocate_shared_for_overwrite and placement new.

According to Visual Studio the parameter passed to make_shared is already a FragmentMetadata&&. I tried to pass std::move and it didn't change anything.

This is the compiler's stack trace, it uses FragmentMetadata&& all the way down to std::construct_at:

C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.39.33519\include\xutility(241): error C2280: 'tiledb::sm::FragmentMetadata::FragmentMetadata(const tiledb::sm::FragmentMetadata &)': attempting to reference a deleted function C:\Users\teo\code\TileDB\tiledb/sm/fragment/fragment_metadata.h(154): note: see declaration of 'tiledb::sm::FragmentMetadata::FragmentMetadata' C:\Users\teo\code\TileDB\tiledb/sm/fragment/fragment_metadata.h(154): note: 'tiledb::sm::FragmentMetadata::FragmentMetadata(const tiledb::sm::FragmentMetadata &)': function was explicitly deleted C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.39.33519\include\xutility(241): note: the template instantiation context (the oldest one first) is C:\Users\teo\code\TileDB\tiledb\sm\serialization\array.cc(302): note: see reference to function template instantiation 'std::shared_ptr<tiledb::sm::FragmentMetadata> tiledb::common::make_shared<tiledb::sm::FragmentMetadata,62,tiledb::sm::FragmentMetadata>(const char (&)[62],tiledb::sm::FragmentMetadata &&)' being compiled C:\Users\teo\code\TileDB\tiledb/common/dynamic_memory/dynamic_memory.h(266): note: see reference to function template instantiation 'std::shared_ptr<tiledb::sm::FragmentMetadata> tiledb::common::`anonymous-namespace'::AllocationFunctions<tiledb::sm::FragmentMetadata>::make_shared<62,_Ty>(const char (&)[62],_Ty &&)' being compiled with [ _Ty=tiledb::sm::FragmentMetadata ] C:\Users\teo\code\TileDB\tiledb/common/dynamic_memory/dynamic_memory.h(238): note: see reference to function template instantiation 'std::shared_ptr<tiledb::sm::FragmentMetadata> tiledb::common::`anonymous-namespace'::AllocationFunctions<tiledb::sm::FragmentMetadata>::make_shared_notrace<_Ty>(_Ty &&)' being compiled with [ _Ty=tiledb::sm::FragmentMetadata ] C:\Users\teo\code\TileDB\tiledb/common/dynamic_memory/dynamic_memory.h(198): note: see reference to function template instantiation 'std::shared_ptr<tiledb::sm::FragmentMetadata> std::allocate_shared<T,tiledb::common::GovernedAllocator<T,tiledb::common::`anonymous-namespace'::TiledbTracedAllocator,tiledb::common::Governor>,_Ty>(const _Alloc &,_Ty &&)' being compiled with [ T=tiledb::sm::FragmentMetadata, _Ty=tiledb::sm::FragmentMetadata, _Alloc=tiledb::common::GovernedAllocator<tiledb::sm::FragmentMetadata,tiledb::common::`anonymous-namespace'::TiledbTracedAllocator,tiledb::common::Governor> ] C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.39.33519\include\memory(2840): note: see reference to function template instantiation 'void std::_Construct_in_place<std::_Ref_count_obj_alloc3<tiledb::sm::FragmentMetadata,tiledb::common::GovernedAllocator<T,tiledb::common::`anonymous-namespace'::TiledbTracedAllocator,tiledb::common::Governor>>,const _Alloc&,_Ty>(std::_Ref_count_obj_alloc3<_Ty,_Alloc> &,const _Alloc &,_Ty &&) noexcept(false)' being compiled with [ T=tiledb::sm::FragmentMetadata, _Alloc=tiledb::common::GovernedAllocator<tiledb::sm::FragmentMetadata,tiledb::common::`anonymous-namespace'::TiledbTracedAllocator,tiledb::common::Governor>, _Ty=tiledb::sm::FragmentMetadata ] C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.39.33519\include\xutility(255): note: see reference to function template instantiation 'std::_Ref_count_obj_alloc3<tiledb::sm::FragmentMetadata,tiledb::common::GovernedAllocator<T,tiledb::common::`anonymous-namespace'::TiledbTracedAllocator,tiledb::common::Governor>>::_Ref_count_obj_alloc3<_Ty>(const _Alloc &,_Ty &&)' being compiled with [ T=tiledb::sm::FragmentMetadata, _Ty=tiledb::sm::FragmentMetadata, _Alloc=tiledb::common::GovernedAllocator<tiledb::sm::FragmentMetadata,tiledb::common::`anonymous-namespace'::TiledbTracedAllocator,tiledb::common::Governor> ] C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.39.33519\include\memory(2452): note: see reference to function template instantiation 'void std::_Normal_allocator_traits<_Alloc>::construct<_Ty,_Ty>(_Alloc &,_Ty *,_Ty &&)' being compiled with [ _Alloc=tiledb::common::GovernedAllocator<tiledb::sm::FragmentMetadata,tiledb::common::`anonymous-namespace'::TiledbTracedAllocator,tiledb::common::Governor>, _Ty=tiledb::sm::FragmentMetadata ] C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.39.33519\include\xmemory(610): note: see reference to function template instantiation '_Ty *std::construct_at<_Ty,_Ty,0x0>(_Ty *const ,_Ty &&) noexcept(<expr>)' being compiled with [ _Ty=tiledb::sm::FragmentMetadata ]

Turns out FragmentMetadata has a mutex, which is neither copy nor move-constructible. We cannot remove the shared_ptr.

tiledb/sm/fragment/fragment_metadata.cc

teo-tsirpanis · 2024-04-19T13:37:46Z

I restored some move constructors and am getting errors like static_assert failed: 'T must be constructible from either (allocator_arg_t, const Alloc&, Types...) or (Types..., const Alloc&) if uses_allocator_v<remove_cv_t<T>, Alloc> is true'. Any idea on how to fix them?

KiterLuc · 2024-04-19T17:05:51Z

I restored some move constructors and am getting errors like static_assert failed: 'T must be constructible from either (allocator_arg_t, const Alloc&, Types...) or (Types..., const Alloc&) if uses_allocator_v<remove_cv_t<T>, Alloc> is true'. Any idea on how to fix them?

@davisp any ideas here?

davisp · 2024-04-29T14:15:33Z

That’s a rule for constructors on PMR containers stored inside other pmr containers. Either the constructor has to have the last argument for the Allocator, or the first argument must be the special trait marker and the second argument is the allocator argument. So likely this was adding the move constructor without providing an argument to accept the allocator.

davisp · 2024-04-29T14:21:26Z

Pablo Halpern covers the container constructor stuff during this part of his cppcon talk: https://youtu.be/v3dz-AKOVL8?t=2360&si=qKn94l2h8x9uwtHT

teo-tsirpanis · 2024-04-29T19:58:35Z

Thanks for the help! The problem was in d4d9007, and after a couple more modifications it builds locally.

All feedback was addressed, except for one item which is not immediately actionable.

teo-tsirpanis · 2024-05-13T18:37:25Z

CI is green. @KiterLuc can we merge it?

teo-tsirpanis added 2 commits January 29, 2024 18:47

Unstatus deserialize_non_empty_domain and make it non-generic.

8c00ca3

Unstatus serialize_non_empty_domain and make it non-generic.

a6f0b29

teo-tsirpanis force-pushed the teo/fragment-metadata-c.41 branch from 7bf109f to 9a4c573 Compare January 29, 2024 22:59

teo-tsirpanis added 3 commits January 30, 2024 01:21

Add C.41 constructor for FragmentMetadata and use it in serialization.

2767d48

Fix compile errors.

a2b1933

Clean-up FragmentMetadata: remove unused methods, add missing const…

f2728ba

… qualifiers and return small values by value.

teo-tsirpanis force-pushed the teo/fragment-metadata-c.41 branch from 9a4c573 to f2728ba Compare January 29, 2024 23:22

davisp reviewed Jan 30, 2024

View reviewed changes

tiledb/sm/serialization/fragment_metadata.cc Outdated Show resolved Hide resolved

teo-tsirpanis force-pushed the teo/fragment-metadata-c.41 branch 2 times, most recently from dc18af8 to f3b3138 Compare January 30, 2024 18:00

Fix compile errors.

9a3ab85

teo-tsirpanis force-pushed the teo/fragment-metadata-c.41 branch from f3b3138 to 9a3ab85 Compare January 30, 2024 18:45

teo-tsirpanis added 5 commits January 30, 2024 22:13

Fix tests.

9f87c6a

Fix more tests.

a609b10

Move UnorderedWriter::resources to StrategyBase.

2d8b4ea

Fix segfaults by passing context resources when deserializing fragmen…

3aec56d

…t metadata in global order writer.

clang-format

c9ffdb0

teo-tsirpanis marked this pull request as ready for review January 31, 2024 17:04

KiterLuc requested a review from eric-hughes-tiledb February 8, 2024 15:16

KiterLuc reviewed Feb 12, 2024

View reviewed changes

tiledb/sm/query/strategy_base.h Outdated Show resolved Hide resolved

tiledb/sm/serialization/fragment_metadata.cc Outdated Show resolved Hide resolved

tiledb/sm/serialization/fragment_metadata.h Show resolved Hide resolved

teo-tsirpanis added 2 commits February 13, 2024 15:29

Address PR feedback.

43d8738

Merge branch 'dev' into teo/fragment-metadata-c.41

d6af715

teo-tsirpanis requested a review from KiterLuc February 13, 2024 17:57

eric-hughes-tiledb previously requested changes Feb 14, 2024

View reviewed changes

teo-tsirpanis added 2 commits February 15, 2024 14:43

Fix most compile errors.

0ad881e

Address some PR feedback.

2f29592

teo-tsirpanis commented Feb 15, 2024

View reviewed changes

tiledb/sm/fragment/fragment_metadata.cc Show resolved Hide resolved

teo-tsirpanis mentioned this pull request Feb 21, 2024

Remove move from Tile. #4749

Merged

teo-tsirpanis added 2 commits April 17, 2024 14:21

Merge branch 'dev' into teo/fragment-metadata-c.41

c5d8c38

Restore some deleted move constructors.

f894873

teo-tsirpanis force-pushed the teo/fragment-metadata-c.41 branch from eae03a6 to f894873 Compare April 17, 2024 12:44

Fix some errors.

bcedcbe

teo-tsirpanis added 3 commits April 29, 2024 22:42

Fix compile errors.

d4d9007

Merge branch 'dev' into teo/fragment-metadata-c.41

51902e9

Instrument fragment mins/maxs.

491ebf5

Merge branch 'dev' into teo/fragment-metadata-c.41

790e370

teo-tsirpanis marked this pull request as draft May 23, 2024 14:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove serialization non C.41 constructors from fragment_metadata_from_capnp #4682

Remove serialization non C.41 constructors from fragment_metadata_from_capnp #4682

teo-tsirpanis commented Jan 29, 2024 •

edited by KiterLuc

shortcut-integration bot commented Jan 29, 2024

KiterLuc commented Jan 31, 2024

teo-tsirpanis commented Jan 31, 2024

eric-hughes-tiledb Feb 14, 2024

teo-tsirpanis Feb 15, 2024

eric-hughes-tiledb Feb 14, 2024

teo-tsirpanis Feb 15, 2024

eric-hughes-tiledb Feb 14, 2024

teo-tsirpanis Feb 15, 2024

eric-hughes-tiledb Feb 14, 2024

teo-tsirpanis Feb 15, 2024

KiterLuc May 1, 2024

teo-tsirpanis May 1, 2024 •

edited

eric-hughes-tiledb May 1, 2024

teo-tsirpanis May 1, 2024

teo-tsirpanis May 1, 2024

teo-tsirpanis May 13, 2024

teo-tsirpanis commented Apr 19, 2024

KiterLuc commented Apr 19, 2024

davisp commented Apr 29, 2024

davisp commented Apr 29, 2024

teo-tsirpanis commented Apr 29, 2024

teo-tsirpanis commented May 13, 2024

Remove serialization non C.41 constructors from fragment_metadata_from_capnp #4682

Are you sure you want to change the base?

Remove serialization non C.41 constructors from fragment_metadata_from_capnp #4682

Conversation

teo-tsirpanis commented Jan 29, 2024 • edited by KiterLuc

shortcut-integration bot commented Jan 29, 2024

KiterLuc commented Jan 31, 2024

teo-tsirpanis commented Jan 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teo-tsirpanis May 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teo-tsirpanis commented Apr 19, 2024

KiterLuc commented Apr 19, 2024

davisp commented Apr 29, 2024

davisp commented Apr 29, 2024

teo-tsirpanis commented Apr 29, 2024

teo-tsirpanis commented May 13, 2024

teo-tsirpanis commented Jan 29, 2024 •

edited by KiterLuc

teo-tsirpanis May 1, 2024 •

edited