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

Add memory tracking for Domain. #4758

Merged
merged 16 commits into from
Mar 5, 2024
Merged

Add memory tracking for Domain. #4758

merged 16 commits into from
Mar 5, 2024

Conversation

shaunrd0
Copy link
Contributor

This adds a MemoryTracker to Domain and updates class members to use tdb::pmr containers.

TYPE: NO_HISTORY
DESC: Add memory tracking for Domain

@shaunrd0
Plumbing for MemoryTracker
2520965
@shaunrd0
Disable copy and move
3ec8b40
@shaunrd0
Use tdb::pmr for vector members
322d948
@shaunrd0
Update methods to use pmr::vector
cb4c75d 
Domain::overlap_ratio is passed Subarray::is_default_ as a parameter, so
this method was not updated. Once Subarray::is_default_ is a pmr::vector
this method can be updated to take a pmr::vector parameter.
@shaunrd0
Rename DOMAIN->DOMAINS
99ff2f5 
Avoids conflict with math.h definitions seen in Windows, OSX, and
manylinux CI

https://github.com/shaunrd0/TileDB/actions/runs/7993508034/job/21829493275#step:12:158
@shortcut-integration Shortcut Integration
Copy link

This pull request has been linked to Shortcut Story #41161: Add memory tracking for Domain..

@shaunrd0 shaunrd0 requested a review from davisp February 21, 2024 21:54
@shaunrd0 shaunrd0 marked this pull request as ready for review February 21, 2024 21:54
davisp
davisp reviewed 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.

A few notes. I mostly skimmed this cause as you point out it'll need rebasing after the ArraySchema merge and then I'll review closer. Though on a quick skim, I didn't find anything major.

tiledb/api/c_api/domain/test/compile_capi_domain_stub_main.cc Outdated Show resolved Hide resolved
tiledb/sm/array_schema/domain.h Show resolved Hide resolved
tiledb/sm/query/test/unit_query_condition.cc Outdated Show resolved Hide resolved
@shaunrd0 shaunrd0 force-pushed the smr/sc-41161/memory-domain branch from 0c594ff to ded935d Compare February 23, 2024 17:02
@shaunrd0 shaunrd0 requested a review from davisp February 23, 2024 17:44
davisp
davisp requested changes Feb 29, 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.

Just the one issue. Everything else looks good.

tiledb/sm/array_schema/domain.cc Outdated Show resolved Hide resolved
tiledb/sm/array_schema/domain.cc Outdated Show resolved Hide resolved
@shaunrd0 shaunrd0 requested a review from davisp March 1, 2024 18:21
davisp
davisp approved these changes Mar 4, 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

davisp
davisp requested changes Mar 4, 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.

Just remembered I forgot to double check the to string case list.

tiledb/common/memory_tracker.h Show resolved Hide resolved
davisp
davisp approved these changes Mar 5, 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 once conflicts are merged and CI is green.

@KiterLuc KiterLuc merged commit faa42b9 into dev Mar 5, 2024
57 checks passed
@KiterLuc KiterLuc deleted the smr/sc-41161/memory-domain branch March 5, 2024 17:47
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.
3 participants
@shaunrd0 @davisp @KiterLuc