[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:
Related
related to SERVER-17223 Disable updateWithDamages support for... Closed
is related to SERVER-15794 deleteRecord should take the record t... Closed
Tested
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:

  1. Changing APIs to pass more data around than is needed can be problematic as ensuring consistency of the extra data becomes an extra responsibility for all callers, or a burden for the callee.
  2. Ideally, a command like db.coll.update({_id:1}, { x : 1}) would never even need to read record `1`. So, rather than passing around the old document more, we may want to pass it around less.
  3. It would seem that the storage engine's `recoveryUnit` could just hold on to the pointer or the length itself when `dataFor` is called if that is an optimization that helps the storage engine.

-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
I can make a pull request from it at any time, just let me know when.

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

  5 indexed & 5 un-indexed updates per transaction 25 indexed updates per transaction
Vanilla RocksDB 255.94 103.68
Patched RocksDB 332.08 138.44

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
https://blog.connectifier.com/how-we-patched-mongodb-rocksdb-to-boost-performance/

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 ]

chengas123,

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.

Generated at Thu Feb 08 03:39:00 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.