[SERVER-15793] Storage engine's updateRecord function should take oldRec Created: 24/Oct/14 Updated: 17/Apr/15 Resolved: 23/Jan/15 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Storage |
| Affects Version/s: | 2.7.8 |
| Fix Version/s: | None |
| Type: | Improvement | Priority: | Minor - P4 |
| Reporter: | John Esmet | Assignee: | Unassigned |
| Resolution: | Done | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||||||
| Participants: | |||||||||||||||||
| Description |
|
For storage engines that do transactional size checking, the old record is necessary to know the data size delta for the running update. Re-reading it from storage is not very cheap so the API should just include the oldRec, since it is readily available to the caller. Related to https://jira.mongodb.org/browse/SERVER-15701 |
| Comments |
| Comment by Ben McCann [ 17/Apr/15 ] | |||||||||
|
Thanks for the review and feedback on this, Geert. Igor, what do you think of this suggestion? You're much more familiar with this code, so I don't have a sense of which would be more preferable or feasible for the storage engines as end users of the interface. I filed an issue with the suggestion over on your GitHub, so that we don't lose track of it (https://github.com/mongodb-partners/mongo-rocks/issues/2) | |||||||||
| Comment by Geert Bosch [ 10/Apr/15 ] | |||||||||
|
I'm not sure about this approach. My reasons are:
-Geert | |||||||||
| Comment by Igor Canadi [ 10/Mar/15 ] | |||||||||
|
It would be great to get this in. My recent testing on Parse workload shows that there are periods where mongo+rocks is very CPU-bound (lots of cached queries coming in). Any CPU that we can save is meaningful. | |||||||||
| Comment by Ben McCann [ 10/Mar/15 ] | |||||||||
|
@Benety Goh think we could reopen this for consideration now that the 3.0 release is out? | |||||||||
| Comment by Benety Goh [ 20/Feb/15 ] | |||||||||
|
chengas123, let's discuss after 3.0 is released. | |||||||||
| Comment by Ben McCann [ 19/Feb/15 ] | |||||||||
|
@Benety, do you think it'd be an appropriate time to consider the patch now with the benchmarks available and the 3.0 branch cut? | |||||||||
| Comment by Gevorg Voskanyan [ 11/Feb/15 ] | |||||||||
|
The new branch for this is here: https://github.com/gevorgvoskanyan/mongo/tree/server-15793_1 | |||||||||
| Comment by Igor Canadi [ 08/Feb/15 ] | |||||||||
|
@Ben I'm assuming the performance improvement is in the case when the entire DB is cached. We shouldn't be paying too much cost on I/O-bound workloads since that block is likely to be present in cache (as it was recently read). However, CPU improvement should be substantial and I'm excited about your results. It would be great to get this in 3.0, but I understand Mongo's decision to postpone the decision after 3.0, since they are only applying finishing touches. | |||||||||
| Comment by Ben McCann [ 07/Feb/15 ] | |||||||||
|
This made a pretty big difference. 29% performance improvement in one test I ran and 33% performance improvement in the other test I ran. Numbers are in sysbench transactions per second
I found some benchmarking tools on the WiredTiger wiki that I used to run these tests. I created a write up in this blog post describing how I ran them I noticed that there's a branch for 3.0 now and the changes going into master seem to be for 3.1. Perhaps we would be able to reopen the PR at this point? | |||||||||
| Comment by Ben McCann [ 30/Jan/15 ] | |||||||||
|
Thanks for clarifying. I'll check back in after 3.0. I haven't gotten to do any performance testing yet. | |||||||||
| Comment by Benety Goh [ 30/Jan/15 ] | |||||||||
|
Let's discuss re-opening the PR after 3.0. You alluded to doing some performance testing in the PR - do you have any results to share with us on the impact of these changes? We haven't abandoned those PRs - just busy with 3.0 at the moment. | |||||||||
| Comment by Ben McCann [ 29/Jan/15 ] | |||||||||
|
Could we re-open this ticket? I'd really like to see Gevorg's change to update RocksDB make it's way in after 3.0 comes out. It's such a clean, isolated change and has such a huge advantage of hitting the disk far less often I understand waiting until 3.0 is released to make any changes, but I think it'd be nice to leave the PR open until then. There are plenty of tickets and even some PRs that are a couple years old and completely abandoned that are still open, so I don't see why we'd rush to close this | |||||||||
| Comment by Benety Goh [ 23/Jan/15 ] | |||||||||
|
We are closing this ticket for now because we are unable to accept the pull requests #837 and #912 in their current form. We can always reopen this ticket in the future if we learn more about the potential benefits of the proposed change. |