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.

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

tbv2: calculate total memory footprint #448

Merged
merged 1 commit into from
Jan 4, 2024

Conversation

JakeHillion
Copy link
Contributor

@JakeHillion JakeHillion commented Dec 29, 2023

tbv2: calculate total memory footprint

Add the option to calculate total size (inclusive size) by wrapping the
existing iterator. This change provides a new iterator, SizedIterator, which
wraps an existing iterator and adds a new field size to the output element.

This is achieved with a two pass algorithm on the existing iterator:

  1. Gather metadata for each element. This includes the total size up until that
    element and the range of elements that should be included in the size.
  2. Return the result from the underlying iterator with the additional
    field.

This algorithm is O(N) time on the number of elements in the iterator and
O(N) time, storing 16 bytes per element. This isn't super expensive but is a
lot more than the current algorithm which requires close to constant space.
Because of this I've implemented it as a wrapper on the iterator rather than on
by default, though it is now on in every one of our integration test cases.

Test plan:

  • Added to the integration tests for full coverage.

Stack created with Sapling. Best reviewed with ReviewStack.

@JakeHillion JakeHillion force-pushed the pr448 branch 3 times, most recently from 8d1b505 to 9a0c672 Compare January 2, 2024 21:41
@JakeHillion JakeHillion marked this pull request as ready for review January 2, 2024 21:41
@JakeHillion JakeHillion requested a review from arsarwade January 2, 2024 21:41
@codecov-commenter
Copy link

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (beb404e) 44.63% compared to head (e7adc06) 44.95%.

Files Patch % Lines
include/oi/exporters/Json.h 86.07% 1 Missing and 10 partials ⚠️
test/integration/runner_common.cpp 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #448      +/-   ##
==========================================
+ Coverage   44.63%   44.95%   +0.31%     
==========================================
  Files         121      123       +2     
  Lines       11905    11975      +70     
  Branches     1947     1953       +6     
==========================================
+ Hits         5314     5383      +69     
+ Misses       5738     5737       -1     
- Partials      853      855       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Add the option to calculate total size (inclusive size) by wrapping the
existing iterator. This change provides a new iterator, `SizedIterator`, which
wraps an existing iterator and adds a new field `size` to the output element.

This is achieved with a two pass algorithm on the existing iterator:
1. Gather metadata for each element. This includes the total size up until that
   element and the range of elements that should be included in the size.
2. Return the result from the underlying iterator with the additional
   field.

This algorithm is `O(N)` time on the number of elements in the iterator and
`O(N)` time, storing 16 bytes per element. This isn't super expensive but is a
lot more than the current algorithm which requires close to constant space.
Because of this I've implemented it as a wrapper on the iterator rather than on
by default, though it is now on in every one of our integration test cases.

Test plan:
- Added to the integration tests for full coverage.
@JakeHillion JakeHillion merged commit 71e734b into facebookexperimental:main Jan 4, 2024
5 checks passed
@JakeHillion JakeHillion deleted the pr448 branch January 4, 2024 09:22
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.

4 participants