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

Refactor to save / print shmem usage stats using common struct. #610

Merged
merged 1 commit into from
Jan 25, 2024

Conversation

gapisback
Copy link
Collaborator

@gapisback gapisback commented Jan 24, 2024

This commit refactors shared memory usage stats fields to drive-off shminfo_usage_stats{} struct entirely. Add platform_save_usage_stats(), used by platform_shm_print_usage().

This refactoring paves the way for upcoming PR #569 which is adding more memory-usage stats fields.


NOTE To the reviewer:

There is zero logic change here. It's mainly a data structural change, rearranging where the memory-usage stats fields are tracked. They now belong to a common struct, so most of the diffs are to access the new usage.<field> reference.

Minor enhancements in the memory usage stats tracking / printing code is done, for improved understanding of memory usage metrics.

This commit refactors shared memory usage stats fields to
drive-off shminfo_usage_stats{} struct entirely. Add
platform_save_usage_stats(), used by platform_shm_print_usage().

This refactoring paves the way for upcoming PR #569 which
is adding more memory-usage stats fields.
Copy link

netlify bot commented Jan 24, 2024

Deploy Preview for splinterdb canceled.

Name Link
🔨 Latest commit 29337e8
🔍 Latest deploy log https://app.netlify.com/sites/splinterdb/deploys/65b163d40cc3220008547b21

size_t shm_total_bytes; // Total size of shared segment allocated initially.
size_t shm_free_bytes; // Free bytes of memory left (that can be allocated)
size_t shm_used_bytes; // Used bytes of memory left (that were allocated)
size_t shm_used_bytes_HWM; // High-water mark of memory used bytes
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This set of deleted stats-fields are now dropped into common shminfo_usage_stats{} struct; listed below.

free_bytes = (size - sizeof(shmem_heap));
shm->shm_free_bytes = free_bytes;
shm->shm_used_bytes = 0;
shm->shm_used_bytes_HWM = 0;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The chunk of deleted inline code is now nicely replaced by a single call to platform_save_usage_stats() on L475, below.

nlarge_frags_inuse_HWM,
large_frags_found_in_use,
used_by_large_frags_bytes,
size_str(used_by_large_frags_bytes));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This messy chunk of deleted code is now all handled cleanly inside expanded fn platform_shm_print_usage_stats(), on L514 below.

@gapisback
Copy link
Collaborator Author

@rtjohnso - This is the last of the peel-off precursor change sets, paving the ground for reviewing PR #569.

I am assigning this to you, optimistically, expecting that the CI-jobs will all succeed. There is no logic change in core shared memory mgmt itself, so I expect CI-jobs will all succeed.

If you can take a quick look at this, I can merge this immediatelywhen CI passes.

And then will start revamping the large part-3 change-set to prepare PR #569 for review.

@gapisback gapisback requested a review from rtjohnso January 24, 2024 19:33
@gapisback
Copy link
Collaborator Author

Gentle reminder @rtjohnso -- in the hope that this ping via this UI will somehow get through to you.

The CI-jobs have succeeded ... and I'm all ready to check this in. Can you give this a quick look, please?

Copy link
Contributor

@rtjohnso rtjohnso left a comment

Choose a reason for hiding this comment

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

LGTM

@gapisback gapisback merged commit 23f5e4f into main Jan 25, 2024
14 checks passed
@gapisback gapisback deleted the agurajada/refactor-usage_stats branch January 25, 2024 06:12
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.

3 participants