[SERVER-31845] WT performance regression with write retryability enabled Created: 06/Nov/17 Updated: 30/Oct/23 Resolved: 29/Nov/17 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | None |
| Affects Version/s: | 3.6.0-rc2 |
| Fix Version/s: | 3.6.1, 3.7.1 |
| Type: | Bug | Priority: | Major - P3 |
| Reporter: | Randolph Tan | Assignee: | Randolph Tan |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Attachments: |
|
||||||||||||||||||||||||||||
| Issue Links: |
|
||||||||||||||||||||||||||||
| Backwards Compatibility: | Fully Compatible | ||||||||||||||||||||||||||||
| Backport Requested: |
v3.6
|
||||||||||||||||||||||||||||
| Sprint: | Sharding 2017-12-04 | ||||||||||||||||||||||||||||
| Participants: | |||||||||||||||||||||||||||||
| Description |
OverviewFor insert_vector workload, there was around 22% less throughput with retryability for single thread and around 17% less throughput for 16 threads. For mixed_insert workload, it was around 20% less throughput at 512 threads. Here's the raw data from sysperf running insert_vector.js and mix.js (please ignore mix delete data since it is returning an error because multi remove is not supported with retryability) as of 18007ba100: Baseline
With Retryability
baseline mix.js
mix.js with retryability
|
| Comments |
| Comment by Githook User [ 30/Nov/17 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Randolph Tan', 'username': 'renctan', 'email': 'randolph@10gen.com'}Message: (cherry picked from commit ece7d8eee78d77a1463302cad68120853af10a2a) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 29/Nov/17 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Randolph Tan', 'username': 'renctan', 'email': 'randolph@10gen.com'}Message: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Kaloian Manassiev [ 16/Nov/17 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Sounds good to me. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Andy Schwerin [ 16/Nov/17 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
That's promising. I propose we review and commit this patch, and then look for mutex contention issues at higher thread counts, using linux perf. Does that seem reasonable renctan and kaloian.manassiev? | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Randolph Tan [ 15/Nov/17 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Using schwerin's suggestion, I tried a patch where I go straight to the index and collection object when updating the transaction table. I see a lot of gains already: for single thread, 9.33% degradation vs 33%. The gains gets smaller as the #of threads increase: at 16 threads, it's 20.6% vs 23.9%. With mixed insert load at 512 threads, the degradation was 32.6% vs 49.3%. Here's the full sysperf data based on this commit: No retry:
Retryability on, updates to transaction table completely bypassing query planning/execution engine:
Retryability on, updates to transaction table using query planning/execution engine:
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by David Storch [ 09/Nov/17 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
The difference is that we already maintain stageDebug for testing. Also, right now its interface is to specify the plan as BSON rather than to use types like IndexScan and FetchStage directly. That layering of a public interface on top of the query execution stages will likely make future maintenance by the query team easier. I'd guess that if we wanted to turn stageDebug into a real thing, however, we might want to use some new PlanBuilder concept as opposed to forcing callers to specify the plan as BSON. Though this might be too much work to make it for 3.6. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Randolph Tan [ 08/Nov/17 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Wouldn't be using stageDebug be the same as constructing the plan manually in that someone has to maintain the special code path? Currently, stageDebug doesn't appear to support updateStage so we also need to implement that if we want to go that route. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by David Storch [ 08/Nov/17 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
The only thought that comes to mind is that the query engine could expose a way for internal local clients to use some sort of direct execution language to specify the desired plan. You know at the call site that you want a particular IXSCAN => FETCH => UPDATE plan, but the query API requires you to specify the logical request rather than the physical plan. Looking at the call graph, it looks like only about 1/3 of the time in updateSessionEntry() is spent setting up the PlanExecutor, so this wouldn't buy you back everything but it could be a significant win. Of course, we have no stable direct execution API today. That leaves us with a few options:
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by David Storch [ 07/Nov/17 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
kaloian.manassiev renctan, do you have profile data you can share showing where we spend time processing the update? Presumably you were hoping to use IDHACK because much of the time is being spent in the planner? | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Randolph Tan [ 06/Nov/17 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
kaloian.manassievdavid.storch Mathias suggested using replacement update instead of using $set, I have seen some improvements, but not a lot. I'll post an update of what the numbers are. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Randolph Tan [ 06/Nov/17 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
FYI: Here's the mmap data, which drops to around 50% less throughput on mix_insert at 512 threads. Most likely caused by collection exclusive lock while modifying the config.transactions table. insert_vector baseline:
insert_vector with retryability:
mix baseline:
mix with retryability:
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Kaloian Manassiev [ 06/Nov/17 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
david.storch, what options do we have for speeding-up the config.transactions update codepath here? The query, which we are constructing is here and the reason we have fields other than the _id is more for "compare-and-swap" type of update rather than an actual lookup. That is, we are using the extra predicates to ensure that we are moving the session's persisted state from one consistent state to another. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Randolph Tan [ 06/Nov/17 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Based on profiling data from PERF-1090, it looks like the update to the config.transacitons table dominates the work, which is understandable, since it takes more work to do an update compared to insert. It also looked like the amount of time to generate the update plan is roughly the same amount of time to execute it. An attempt to short circuit the planner to use idHack didn't work since idHack requires the query predicate to be exclusively _id, which is not the case here. |