WT-3149 Have eviction choose a random point when walking a tree. #3285

agorrod · 2017-02-09T05:21:25Z

Only choose a random point when there is no saved walk point.

michaelcahill · 2017-02-09T05:24:19Z

src/btree/row_srch.c

@@ -785,7 +785,7 @@ __wt_row_random_leaf(WT_SESSION_IMPL *session, WT_CURSOR_BTREE *cbt)
 *	Find a random leaf page in a row-store tree.
 */
 int
-__wt_row_random_descent(WT_SESSION_IMPL *session, WT_CURSOR_BTREE *cbt)


I'm not seeing much here that is row-store specific. If we change the page->type check to use WT_PAGE_IS_INTERNAL(page), can we use this on any tree?

I agree with this, and I've pushed that change in 7666562.

I left the function itself in row_srch.c because I can't think of a better place for it.

agorrod · 2017-02-09T05:24:28Z

@keithbostic Could you take a look at this change and let me know what you think?

There are two things I don't like stylistically about this change:

The eviction server is aware of row storeness.
The extra flags to the search feel a bit clunky.

A more invasive change could fix that by adding a general random search function that returns ENOTSUP for non row-stores. I'm happy to go down that path if you think it'd be better?

I'm also a bit apprehensive about how long the search could take in a sparse tree, but can't think of a good way to check that (and it should be rare that we don't have an existing walk point.

agorrod · 2017-02-09T05:29:31Z

src/evict/evict_lru.c

@@ -1662,6 +1662,21 @@ __evict_walk_file(WT_SESSION_IMPL *session,
 		FLD_SET(walk_flags, WT_READ_PREV);


@keithbostic I think we probably also want to revisit evict_walk_reverse - I suspect we only want to set a walk direction when setting from a NULL evict_ref.

@agorrod

I think we probably also want to revisit evict_walk_reverse - I suspect we only want to set a walk direction when setting from a NULL evict_ref.

If by "revisit", you mean "remove", I think that's a good idea.

Frankly, the less reverse traversal of the tree we do, the happier I am.

work for any tree type.

keithbostic · 2017-02-09T16:05:39Z

@agorrod

The eviction server is aware of row storeness.

Updated per @michaelcahill's suggestion.

The extra flags to the search feel a bit clunky.

I assume you mean the mem_only flag to __wt_row_random_descent(). Yes, I agree it's a little clunky, but I can't think of anything better.

There is a retry in here (retry 100 times if we don't find a valid leaf page), that may not be right for eviction, depending on how often this function is called?

If there are other changes to this function to support eviction, we could create an eviction-specific version: it's only about 80 lines of code.

A more invasive change could fix that by adding a general random search function that returns ENOTSUP for non row-stores.

I think that question is resolved with @michaelcahill's suggested change. The WT_CURSOR.next_random call stack doesn't call into this function for anything other than row-stores (it checks both when setting the cursor's next_random method, and again when the underlying next_random function is called).

I'm also a bit apprehensive about how long the search could take in a sparse tree, but can't think of a good way to check that (and it should be rare that we don't have an existing walk point).

If this function is only called rarely, I don't think sparse trees will be an issue (including any questions about the retry loop in the code).

…ion.

walking it sequentially weren't the same. Changed that, which caused the test_compact02 test to fail. There's an underlying bug in this code, if we return WT_NOTFOUND, we can lose a hazard pointer on the page of the tree we unsucessfully searched. Add a page-release in the case of returning not-found. Simplify the termination conditions as well: there are bunch of ref states I'd just as soon not worry about, for example, WT_REF_SPLIT. I think they'd all work, except split, but as I said, I'd just as soon not worry about them. Limit our valid returns to in-memory if we're running on behalf of eviction, disk and in-memory for everybody else.

keithbostic · 2017-02-09T20:04:37Z

@agorrod
A couple of fixes chasing the test suite failures: b9dd358 and ea9f063.

point should be a more effective solution.

agorrod

@keithbostic I'm pretty happy with this change now, but it needs perf testing (and your agreement) before merging

agorrod · 2017-02-10T04:30:35Z

src/btree/row_srch.c

 */
 int
-__wt_row_random_descent(WT_SESSION_IMPL *session, WT_CURSOR_BTREE *cbt)
+__wt_random_descent(WT_SESSION_IMPL *session, WT_REF **refp, bool eviction)


My vote would be for this to be in a different location. Either a new file; maybe src/btree/btree_random.c or maybe into bt_cursor.c

In response to the other question - I'm inclined to keep the single function for now. The function isn't that long, but I can easily imaging bug fixes coming here, and I don't see a way to share the common code paths.

michaelcahill · 2017-02-10T05:23:29Z

I'd like to talk more about the walk direction when walks start at a random point. Can we have that discussion early next week?

keithbostic · 2017-02-10T15:08:08Z

@agorrod, lgtm.

I agree with you & @michaelcahill, it would be good to move this somewhere else.

I don't want this code in bt_cursor.c, that already has too much stuff.

We could create bt_random.c, and put __wt_btcur_next_random(), __wt_random_descent() and __wt_row_random_leaf() there?

(If we do that, let's merge PR #3289 first, it changes __wt_btcur_next_random().)

michaelcahill · 2017-02-13T00:00:23Z

@keithbostic, agreed and done.

UPDATE: sorry, I lost track of branches and didn't merge #3289 first. I think git will handle this just fine, I'll sort it out tomorrow assuming #3289 is ready to go.

This is designed to keep even coverage of the tree (if we always did forward walks, the left-most pages in the tree would rarely be visited).

michaelcahill · 2017-02-13T01:01:47Z

@agorrod, I resurrected the code to reverse the direction of eviction walks and make it reverse each time we start a new random walk. Let's talk about whether we want to go this way: in my local testing of evict-btree.wtperf, I don't measure any performance impact.

agorrod · 2017-02-14T05:37:02Z

Thanks @keithbostic and @michaelcahill these changes look good to me. I'm going to merge and watch the perf jobs over night.

WT-3149 Have eviction choose a random point when walking a tree.

dfe707b

Only choose a random point when there is no saved walk point.

michaelcahill reviewed Feb 9, 2017

View reviewed changes

agorrod commented Feb 9, 2017

View reviewed changes

keithbostic added 2 commits February 9, 2017 10:41

Merge branch 'develop' into wt-3149-evict_rand_walk

f44b1f5

Rename __wt_row_random_descent() to be __wt_random_descent(), it can

7666562

work for any tree type.

keithbostic approved these changes Feb 9, 2017

View reviewed changes

keithbostic added 2 commits February 9, 2017 14:09

We'll hang if we tap the eviction thread walking the tree to do evict…

b9dd358

…ion.

Stop switching eviction walk direction, choosing a random starting

9ab9f40

point should be a more effective solution.

agorrod commented Feb 10, 2017

View reviewed changes

michaelcahill added 2 commits February 13, 2017 10:53

Merge branch 'develop' into wt-3149-evict_rand_walk

afd6149

Move random lookup code into its own source file.

1251635

michaelcahill added 2 commits February 13, 2017 11:26

whitespace

63d2904

Reverse eviction walks each time we start at a new random point.

75d0a19

This is designed to keep even coverage of the tree (if we always did forward walks, the left-most pages in the tree would rarely be visited).

Merge branch 'develop' into wt-3149-evict_rand_walk

01bb77e

agorrod merged commit df64d27 into develop Feb 14, 2017

agorrod deleted the wt-3149-evict_rand_walk branch February 14, 2017 05:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WT-3149 Have eviction choose a random point when walking a tree. #3285

WT-3149 Have eviction choose a random point when walking a tree. #3285

agorrod commented Feb 9, 2017

michaelcahill Feb 9, 2017

keithbostic Feb 9, 2017

agorrod commented Feb 9, 2017

agorrod Feb 9, 2017

keithbostic Feb 9, 2017

keithbostic commented Feb 9, 2017

keithbostic commented Feb 9, 2017

agorrod left a comment

agorrod Feb 10, 2017

agorrod Feb 10, 2017

michaelcahill commented Feb 10, 2017

keithbostic commented Feb 10, 2017

michaelcahill commented Feb 13, 2017 •

edited

michaelcahill commented Feb 13, 2017

agorrod commented Feb 14, 2017

		@@ -1662,6 +1662,21 @@ __evict_walk_file(WT_SESSION_IMPL *session,
		FLD_SET(walk_flags, WT_READ_PREV);

WT-3149 Have eviction choose a random point when walking a tree. #3285

WT-3149 Have eviction choose a random point when walking a tree. #3285

Conversation

agorrod commented Feb 9, 2017

michaelcahill Feb 9, 2017

Choose a reason for hiding this comment

keithbostic Feb 9, 2017

Choose a reason for hiding this comment

agorrod commented Feb 9, 2017

agorrod Feb 9, 2017

Choose a reason for hiding this comment

keithbostic Feb 9, 2017

Choose a reason for hiding this comment

keithbostic commented Feb 9, 2017

keithbostic commented Feb 9, 2017

agorrod left a comment

Choose a reason for hiding this comment

agorrod Feb 10, 2017

Choose a reason for hiding this comment

agorrod Feb 10, 2017

Choose a reason for hiding this comment

michaelcahill commented Feb 10, 2017

keithbostic commented Feb 10, 2017

michaelcahill commented Feb 13, 2017 • edited

michaelcahill commented Feb 13, 2017

agorrod commented Feb 14, 2017

michaelcahill commented Feb 13, 2017 •

edited