8000 Add `SYSTEM_PEAK_BUFFER_MANAGER_MEMORY` and `SYSTEM_PEAK_TEMP_DIRECTORY_SIZE` to profiler by lnkuiper · Pull Request #17164 · duckdb/duckdb · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add SYSTEM_PEAK_BUFFER_MANAGER_MEMORY and SYSTEM_PEAK_TEMP_DIRECTORY_SIZE to profiler #17164

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
Merged
merged 7 commits into from
Apr 22, 2025
Merged

Add SYSTEM_PEAK_BUFFER_MANAGER_MEMORY and SYSTEM_PEAK_TEMP_DIRECTORY_SIZE to profiler #17164

merged 7 commits into from
Apr 22, 2025

Conversation

lnkuiper
Copy link
Contributor

Measures the peak buffer manager memory usage and temp directory size of the entire system during the query. We could consider expanding this to more query-specific metrics in the future, but these numbers are already easily available; this just exposes them.

They are not enabled by default, but maybe they should be? I would be happy to receive feedback on this.

@lnkuiper lnkuiper requested a review from taniabogatsch April 17, 2025 11:22
taniabogatsch
taniabogatsch reviewed Apr 17, 2025
View reviewed changes
Copy link
Contributor
@taniabogatsch taniabogatsch left a comment

Choose a reason for hiding this comment

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

PR looks great, I really like the changes that make some code parts (global metrics and aggregating metrics) more generic! Just left a few nits. :)

src/include/duckdb/main/query_profiler.hpp Outdated
Comment on lines 45 to 49
double time;
idx_t elements_returned;
idx_t result_set_size;
idx_t system_peak_buffer_manager_memory;
idx_t system_peak_temp_directory_size;
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that we keep likely keep expanding these, and since this PR also adds two additional parameters to the constructor: should we maybe move these into a struct? Something like OperatorInfoMetrics. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's already struct OperatorInformation, but I agree that this constructor is getting out of hand. I checked, and nowhere in the code base are we using the constructor; we always use the defaults. So instead, I've removed the arguments from the constructor, and made it so the values are initialized within the structure, which I think is a lot cleaner.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, great! 👍

@duckdb-draftbot duckdb-draftbot marked this pull request as draft April 22, 2025 08:50
@lnkuiper lnkuiper marked this pull request as ready for review April 22, 2025 08:51
@lnkuiper
Copy link
Contributor Author

Thanks for the feedback @taniabogatsch! I've implemented it, and the code is looking cleaner now :)

taniabogatsch
taniabogatsch approved these changes Apr 22, 2025
View reviewed changes
Copy link
Contributor
@taniabogatsch taniabogatsch left a comment

Choose a reason for hiding this comment

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

Looks good to me now!

@Mytherin Mytherin merged commit 8a17f33 into duckdb:main Apr 22, 2025
51 checks passed
@Mytherin
Copy link
Collaborator

Thanks!

@lnkuiper lnkuiper mentioned this pull request May 8, 2025
Merged
Mytherin added a commit that referenced this pull request May 12, 2025
@Mytherin
Enable SYSTEM_PEAK_BUFFER_MEMORY and SYSTEM_PEAK_TEMP_DIR_SIZE pr…
fda0ba6 
…ofiling by default (#17407)

This PR implements always collecting these statistics (introduced in
#17164) when profiling is enabled.
krlmlr added a commit to duckdb/duckdb-r that referenced this pull request May 18, 2025
@krlmlr
vendor: Update vendored sources to duckdb/duckdb@8a17f33
1cc2f18 
Add `SYSTEM_PEAK_BUFFER_MANAGER_MEMORY` and `SYSTEM_PEAK_TEMP_DIRECTORY_SIZE` to profiler (duckdb/duckdb#17164)
krlmlr added a commit to duckdb/duckdb-r that referenced this pull request May 18, 2025
@krlmlr
vendor: Update vendored sources to duckdb/duckdb@8a17f33
883b559 
Add `SYSTEM_PEAK_BUFFER_MANAGER_MEMORY` and `SYSTEM_PEAK_TEMP_DIRECTORY_SIZE` to profiler (duckdb/duckdb#17164)
krlmlr added a commit to duckdb/duckdb-r that referenced this pull request May 19, 2025
@krlmlr
vendor: Update vendored sources to duckdb/duckdb@8a17f33
a2da06b 
Add `SYSTEM_PEAK_BUFFER_MANAGER_MEMORY` and `SYSTEM_PEAK_TEMP_DIRECTORY_SIZE` to profiler (duckdb/duckdb#17164)
@taniabogatsch taniabogatsch added the Needs Documentation Use for issues or PRs that require changes in the documentation label May 22, 2025
@duckdblabs-bot duckdblabs-bot mentioned this pull request May 22, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@taniabogatsch taniabogatsch taniabogatsch approved these changes

Assignees
No one assigned
Labels
Needs Documentation Use for issues or PRs that require changes in the documentation Ready To Merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lnkuiper @Mytherin @taniabogatsch
0