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

WT-12218 cache_eviction_target_strategy Statistics bug fix and Optimized macro definition #10061

Conversation

y123456yz
Copy link
Contributor

cache_eviction_target_strategy Statistics bug fix and Optimized macro definition

michaelcahill and others added 30 commits December 7, 2017 10:48
Record how many pages we want and how many pages we have queued so far
in a tree, then resume the walk next iteration.  This avoids a single
tree with a target larger than the queue size being walked completely
before the eviction server moves on to the next tree.
(cherry picked from commit fca6a8d)
There's trickiness in the page-pinned check. By definition a remove
operation leaves a cursor positioned if it's initially positioned.
However, if every item on the page is deleted and we unpin the page,
eviction might delete the page and our search will re-instantiate an
empty page for us. Cursor remove returns not-found whether or not
that eviction/deletion happens and it's OK unless cursor-overwrite
is configured (which means we return success even if there's no item
to delete). In that case, we'll fail when we try to point the cursor
at the key on the page to satisfy the positioned requirement. It's
arguably safe to simply leave the key initialized in the cursor (as
that's all a positioned cursor implies), but it's probably safer to
avoid page eviction entirely in the positioned case.

(cherry picked from commit 2ac616e)
Copy link

Hi @y123456yz, thank you for your submission!
Please make sure to sign our Contributor Agreement (if you haven't already) and provide us with editor permissions on your branch. Instructions on how do that can be found here.

@y123456yz
Copy link
Contributor Author

WT-12218

@korteland korteland changed the title cache_eviction_target_strategy Statistics bug fix and Optimized macro definition WT-12218 cache_eviction_target_strategy Statistics bug fix and Optimized macro definition Jan 2, 2024
@Siddhartha8899 Siddhartha8899 self-requested a review January 2, 2024 00:58
} else
}

if (F_ISSET(cache, WT_CACHE_EVICT_DIRTY) && F_ISSET(cache, WT_CACHE_EVICT_UPDATES))
WT_STAT_CONN_INCR(session, cache_eviction_target_strategy_both_clean_and_dirty);
Copy link
Contributor

Choose a reason for hiding this comment

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

@y123456yz thanks for this PR.

Could you help me understand the reasoning behind these changes?

@Siddhartha8899
Copy link
Contributor

@y123456yz let me know your thoughts on #10061 (comment)

@y123456yz
Copy link
Contributor Author

y123456yz commented Jan 12, 2024

@y123456yz let me know your thoughts on #10061 (comment)
@Siddhartha8899
I'm sorry, I didn't catch you earlier, but this is actually as following, I have changed:

    if (F_ISSET(cache, WT_CACHE_EVICT_CLEAN) && F_ISSET(cache, WT_CACHE_EVICT_DIRTY))
        WT_STAT_CONN_INCR(session, cache_eviction_target_strategy_both_clean_and_dirty);

https://github.com/y123456yz/wiredtiger/blob/develop_cache_eviction_target_strategy_bug_fix/src/evict/evict_lru.c#L1848-L1849

@Siddhartha8899
Copy link
Contributor

@y123456yz thanks, but I do not see the current check as a bug. Could you please explain the reasoning behind this change.

@y123456yz
Copy link
Contributor Author

y123456yz commented Jan 12, 2024

@y123456yz thanks, but I do not see the current check as a bug. Could you please explain the reasoning behind this change.

hi, @Siddhartha8899
old code:

    if (!F_ISSET(cache, WT_CACHE_EVICT_DIRTY | WT_CACHE_EVICT_UPDATES))
        //only clean
    else if (!F_ISSET(cache, WT_CACHE_EVICT_CLEAN)) {
        //There are 3 cases of this branch: [dirty, update, update+dirty]
    } else
       //There are 3 cases of this branch: [clean+update, clean+diry, clean+update+dirty]

Since dirty contains updates, it can be simplified to:

    if (!F_ISSET(cache, WT_CACHE_EVICT_DIRTY | WT_CACHE_EVICT_UPDATES))
        //only clean
    else if (!F_ISSET(cache, WT_CACHE_EVICT_CLEAN)) {
        //There are 2 cases of this branch: [dirty, update]
    } else
       //There are 2 cases of this branch: [clean+update, clean+diry]

Our disagreement should be whether to consider WT_CACHE_EVICT_UPDATES?

@Siddhartha8899
Copy link
Contributor

Hi @y123456yz we are currently reviewing this specific code base internally and would like to hold off making any changes until we have determined how to proceed. So we would like to close this PR without merging at this time. There is currently no deadline for the current code review, but we will endeavor to update the corresponding Jira ticket when appropriate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants