Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Integrate std::pmr memory tracking for class ArraySchema. #4696

Merged
merged 16 commits into from Feb 23, 2024
Merged

Integrate std::pmr memory tracking for class ArraySchema. #4696

merged 16 commits into from Feb 23, 2024

Conversation

abigalekim
Copy link
Contributor

@abigalekim abigalekim commented Feb 2, 2024 •
edited by KiterLuc

Adding PMR tracking for all vector and unordered map variables of the ArraySchema class.

TYPE: NO_HISTORY
DESC: Integrate std::pmr memory tracking for class ArraySchema.

@shortcut-integration Shortcut Integration
Copy link

This pull request has been linked to Shortcut Story #40462: Integrate std::prm memory tracking for class ArraySchema..

@davisp davisp changed the base branch from dev to pd/sc-36075/implement-pmr-based-memory-tracker February 2, 2024 20:47
@abigalekim abigalekim changed the base branch from pd/sc-36075/implement-pmr-based-memory-tracker to abigalekim/sc-40506/pmr-unordered-map February 3, 2024 01:13
@KiterLuc KiterLuc changed the title Integrate std::prm memory tracking for class ArraySchema. Integrate std::pmr memory tracking for class ArraySchema. Feb 5, 2024
davisp
davisp requested changes Feb 6, 2024
View reviewed changes
Copy link
Contributor

@davisp davisp left a comment

Choose a reason for hiding this comment

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

Looks pretty good overall, but when you go to rebase this you'll end up with two main issues. First, all the test code that's currently calling make_shared<MemoryTracker> is going to need to be updated to use tiledb::test::create_test_memory_tracker(). Secondly, the deserialization code paths will quickly show that you'll need to add a shared_ptr<MemoryTracker> argument in a number of places.

Given all of that is my fault, feel free to ask for any help with making those changes.

test/src/unit-enumerations.cc Outdated Show resolved Hide resolved
test/src/unit-enumerations.cc Outdated Show resolved Hide resolved
tiledb/sm/array_schema/array_schema.cc Outdated Show resolved Hide resolved
tiledb/sm/array_schema/array_schema.cc Outdated Show resolved Hide resolved
tiledb/sm/array_schema/array_schema.cc Show resolved Hide resolved
tiledb/sm/array_schema/dimension_label.cc Outdated Show resolved Hide resolved
tiledb/sm/c_api/tiledb.cc Outdated Show resolved Hide resolved
tiledb/sm/c_api/tiledb_filestore.cc Outdated Show resolved Hide resolved
tiledb/sm/consolidator/fragment_consolidator.cc Show resolved Hide resolved
tiledb/sm/rest/rest_client.cc Show resolved Hide resolved
@davisp
Copy link
Contributor

davisp commented Feb 6, 2024

Also, given the base branch setup, its vaguely odd that this contains the tdb::pmr::unordered_map change. We'll need to make sure and get that figure out since you've got it in a separate PR (which is great!). For now it shouldn't hurt anything, but just let me know when you're ready and I can rework the history to get that straightened out.

@abigalekim abigalekim force-pushed the pd/abigalekim/sc-40462/pmr-memory-tracking-arrayschema branch from d7c80de to 084b2a3 Compare February 7, 2024 20:17
@abigalekim abigalekim marked this pull request as draft February 7, 2024 21:21
@abigalekim abigalekim marked this pull request as ready for review February 7, 2024 22:51
@abigalekim abigalekim force-pushed the pd/abigalekim/sc-40462/pmr-memory-tracking-arrayschema branch from 0f136fd to 1711586 Compare February 7, 2024 22:56
@abigalekim abigalekim force-pushed the abigalekim/sc-40506/pmr-unordered-map branch from 06776da to 7676d6e Compare February 7, 2024 23:00
@davisp davisp force-pushed the abigalekim/sc-40506/pmr-unordered-map branch from 7676d6e to 2e0e36d Compare February 12, 2024 20:55
@abigalekim abigalekim force-pushed the pd/abigalekim/sc-40462/pmr-memory-tracking-arrayschema branch from 1711586 to a0b1fbe Compare February 12, 2024 22:52
Base automatically changed from abigalekim/sc-40506/pmr-unordered-map to dev February 13, 2024 07:05
@abigalekim
array schema plumbing
8abe584
@abigalekim
Merging changes
50036be
@abigalekim
format
0b57ae3
@abigalekim
Some code review changes have been addressed:
07b9f08 
- Deleted default constructor of class tiledb::sm::ArraySchema
- Changed copy_with_new_memory_tracker to clone
- Deleted constructor with pmr vector.

Problems remaining:
- had to make copy constructor public since a protected one does not work with our version of make_shared.
@abigalekim
array schema unordered map:
1701cce
@abigalekim
some code changes
c3b63f6
@abigalekim
everything but serialization
6f1053a
@abigalekim
addressed pauls changes
ee51025
@abigalekim abigalekim force-pushed the pd/abigalekim/sc-40462/pmr-memory-tracking-arrayschema branch from a0b1fbe to ee51025 Compare February 16, 2024 03:07
@abigalekim abigalekim force-pushed the pd/abigalekim/sc-40462/pmr-memory-tracking-arrayschema branch from 841f930 to e80e797 Compare February 16, 2024 06:59
davisp
davisp requested changes Feb 16, 2024
View reviewed changes
Copy link
Contributor

@davisp davisp left a comment

Choose a reason for hiding this comment

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

Looks pretty good overall. I can not stress enough that the ArraySchema constructor argument is absolutely not something you have to do yourself. I can handle it after we merge as a follow up since its grunt work and mostly just to start following a pattern I hadn't decided on when you started this work.

Let me know if you need any help fixing up the last of those CI errors.

test/src/unit-enumerations.cc Outdated Show resolved Hide resolved
test/src/unit-filter-pipeline.cc Outdated Show resolved Hide resolved
tiledb/sm/array/test/CMakeLists.txt Outdated Show resolved Hide resolved
tiledb/sm/array_schema/array_schema.cc Outdated Show resolved Hide resolved
tiledb/sm/array_schema/array_schema.cc Outdated Show resolved Hide resolved
tiledb/sm/consolidator/test/CMakeLists.txt Outdated Show resolved Hide resolved
tiledb/sm/rest/rest_client.cc Show resolved Hide resolved
tiledb/sm/rest/rest_client.cc Show resolved Hide resolved
tiledb/sm/rest/rest_client.h Show resolved Hide resolved
@shaunrd0 shaunrd0 mentioned this pull request Feb 21, 2024
Merged
davisp
davisp approved these changes Feb 22, 2024
View reviewed changes
Copy link
Contributor

@davisp davisp left a comment

Choose a reason for hiding this comment

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

+1 Awesome work!

@KiterLuc KiterLuc merged commit 2f8188e into dev Feb 23, 2024
57 checks passed
@KiterLuc KiterLuc deleted the pd/abigalekim/sc-40462/pmr-memory-tracking-arrayschema branch February 23, 2024 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davisp davisp davisp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@abigalekim @davisp @KiterLuc