WT-13990 Make handle-related stats private in the checkpoint module #11402

etienneptl · 2025-01-10T04:05:09Z

This PR creates APIs in order to make the existing struct WT_CKPT_HANDLE_STATS private.

…ndle_stats

github-actions · 2025-01-10T04:05:22Z

Thanks for creating a pull request! Please answer the questions below by editing this comment.

Type of change made in this PR

Functional change
Test-only change
Refactor-only change
Other non-functional change

What makes this change safe?

Answering this question helps the reviewers understand where they should focus their attention. Please consider these prompts:

How risky is this change? Why?
Low risk, creating new APIs to refactor the code.
What tests are you adding, changing or relying on? Why?
Existing ones.
What, if anything, are you concerned about that you'd like the reviewer to focus on?
Please double check the APIs are correct and if they make sense, they might seem overkill just to get to a private module.

References:

Checklist before requesting a review

I have performed a self-review of my code.
I have made corresponding changes to the documentation, or no documentation change is needed.
I have tests that show my change is correct, or have described above why functional test changes are not required.

wiredtiger-prbot · 2025-01-10T04:21:06Z

Test coverage is ok, please refer to the Code change/coverage report links below and try to improve it if feasible.

Metric (for added/changed code)	Coverage
Line coverage	100% (27/27)
Branch coverage	76% (35/46)

ershov

Thanks Etienne, I like the change!
Just a couple of comments inside.

ershov · 2025-01-10T04:51:51Z

src/conn/conn_dhandle.c

+        if (F_ISSET(S2BT(session), WT_BTREE_SKIP_CKPT))
+            __wt_checkpoint_skip_handle_stats(session, &conn->ckpt, time_diff);
+        else
+            __wt_checkpoint_apply_handle_stats(session, &conn->ckpt, time_diff);


Did you think to move this "if" into the function and merge the "skip" and "apply" functions into one? Then here it'd be just one function call and this logic would move inside the function.

Yes, I pointed out that it was a bit overkill in the description above. I am glad you think the same.
The issue with this approach is that we need something from the caller to tell us if we should be in the if or else statement.

I thought, the function could be literally like:

__wt_checkpoint_apply_handle_stats(...) { if (F_ISSET(S2BT(session), WT_BTREE_SKIP_CKPT)) { ... } else { ... }

But it's a nitpick.

I did not want to add more btree stuff in the checkpoint but I think it should be fine, what do you think about it @sueloverso?
Updated in 9575d88 but happy to revert if that was the wrong move.

ershov · 2025-01-10T04:52:10Z

src/checkpoint/checkpoint_stats.c

+
+/*
+ * __wt_checkpoint_set_handle_stats --
+ *     Reset handle-related stats.


Comment copy-pasted?

Nice catch! Fixed in c11788f

ershov

I have some readability suggestions but otherwise I'm happy with the PR!

ershov · 2025-01-10T06:07:01Z

src/checkpoint/checkpoint_stats.c

+ *     Update the skip handle-related stats.
+ */
+void
+__wt_checkpoint_skip_handle_stats(


If you decide to keep these functions separate, then because this function is very similar to __wt_checkpoint_apply_handle_stats, I think it'd be more clear to put them next to each other.

ershov · 2025-01-10T06:09:16Z

src/conn/conn_dhandle.c

+        if (F_ISSET(S2BT(session), WT_BTREE_SKIP_CKPT))
+            __wt_checkpoint_skip_handle_stats(session, &conn->ckpt, time_diff);
+        else
+            __wt_checkpoint_apply_handle_stats(session, &conn->ckpt, time_diff);


I thought, the function could be literally like:

__wt_checkpoint_apply_handle_stats(...) { if (F_ISSET(S2BT(session), WT_BTREE_SKIP_CKPT)) { ... } else { ... }

But it's a nitpick.

Sean04 · 2025-01-13T02:34:33Z

src/checkpoint/checkpoint_private.h

+struct __wti_ckpt_handle_stats {
+    uint64_t apply;           /* handles applied */
+    uint64_t apply_time;      /* applied handles gather time */
+    uint64_t drop;            /* handles dropped */


While we're here could you please improve this comment. I've been mislead by this implying dropping handles but it's actually dropping checkpoints for that handle. So I would replace it with:
handle checkpoints dropped.

Does it mean we need to update the other fields too?

No, updating the comment here where we define it is okay. If you encounter it elsewhere I expect the comment where it's defined to be what clarifies it.

Done in 738caf6

Sean04

LGTM, nice change

…dle_stats

Siddhartha8899 · 2025-01-13T06:11:38Z

src/conn/conn_dhandle.c

@@ -664,13 +664,7 @@ __conn_btree_apply_internal(WT_SESSION_IMPL *session, WT_DATA_HANDLE *dhandle,
    if (time_start != 0) {
        time_stop = __wt_clock(session);
        time_diff = WT_CLOCKDIFF_US(time_stop, time_start);
-        if (F_ISSET(S2BT(session), WT_BTREE_SKIP_CKPT)) {


is it possible to have them incremented within the file_func?
Then there is no need to call a seperate function and the logic would become easier to understand.

Does it make it easier to understand though? You have to trace back what "file_func" is to know what it does.
Did you want to use file_func in __checkpoint_apply_operation?

Discussed with @Siddhartha8899 online, we won't do it. This PR improves encapsulation but, unfortunately, not coupling. It would be great to revisit the code flow and make some effort towards making the coupling less complex.

sueloverso

LGTM

etienneptl added 6 commits January 10, 2025 13:03

Create checkpoint_stats.c

46b9d3e

Create __wt_checkpoint_reset_handle_stats

59c1815

Create __wt_checkpoint_set_handle_stats

473c8dd

Create __wt_checkpoint_apply_handle_stats and __wt_checkpoint_skip_ha…

fc251bb

…ndle_stats

Make __wt_ckpt_handle_stats private

bf4600f

s_all

c05bbd2

etienneptl requested review from ershov, sueloverso, Sean04 and Siddhartha8899 January 10, 2025 04:05

etienneptl self-assigned this Jan 10, 2025

ershov reviewed Jan 10, 2025

View reviewed changes

Fix typo

c11788f

ershov approved these changes Jan 10, 2025

View reviewed changes

Sean04 reviewed Jan 13, 2025

View reviewed changes

Sean04 approved these changes Jan 13, 2025

View reviewed changes

Merge __wt_checkpoint_skip_handle_stats and __wt_checkpoint_apply_han…

9575d88

…dle_stats

Siddhartha8899 reviewed Jan 13, 2025

View reviewed changes

sueloverso approved these changes Jan 13, 2025

View reviewed changes

Improve comment

738caf6

etienneptl added this pull request to the merge queue Jan 13, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 13, 2025

etienneptl added this pull request to the merge queue Jan 14, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 14, 2025

etienneptl added this pull request to the merge queue Jan 14, 2025

Merged via the queue into develop with commit d48adcd Jan 14, 2025
10 checks passed

etienneptl deleted the WT-13990-ckpt-handle-stats-api branch January 14, 2025 01:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WT-13990 Make handle-related stats private in the checkpoint module #11402

WT-13990 Make handle-related stats private in the checkpoint module #11402

etienneptl commented Jan 10, 2025

github-actions bot commented Jan 10, 2025 •

edited by etienneptl

Loading

wiredtiger-prbot bot commented Jan 10, 2025 •

edited

Loading

ershov left a comment

ershov Jan 10, 2025

etienneptl Jan 10, 2025

ershov Jan 10, 2025

etienneptl Jan 13, 2025 •

edited

Loading

ershov Jan 10, 2025

etienneptl Jan 10, 2025

ershov left a comment

ershov Jan 10, 2025

ershov Jan 10, 2025

Sean04 Jan 13, 2025 •

edited

Loading

etienneptl Jan 13, 2025

Sean04 Jan 13, 2025

etienneptl Jan 13, 2025

Sean04 left a comment

Siddhartha8899 Jan 13, 2025

etienneptl Jan 13, 2025

etienneptl Jan 13, 2025

sueloverso left a comment

WT-13990 Make handle-related stats private in the checkpoint module #11402

WT-13990 Make handle-related stats private in the checkpoint module #11402

Conversation

etienneptl commented Jan 10, 2025

github-actions bot commented Jan 10, 2025 • edited by etienneptl Loading

Type of change made in this PR

What makes this change safe?

Checklist before requesting a review

wiredtiger-prbot bot commented Jan 10, 2025 • edited Loading

ershov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etienneptl Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ershov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sean04 Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sean04 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sueloverso left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 10, 2025 •

edited by etienneptl

Loading

wiredtiger-prbot bot commented Jan 10, 2025 •

edited

Loading

etienneptl Jan 13, 2025 •

edited

Loading

Sean04 Jan 13, 2025 •

edited

Loading