Bug 2092473: libovsdb perf backports #1119
Conversation
If NativeToOVS was successful, then simply assign the value and break out of the switch statement, but continue with the loop. Prior to this change, only a single pointer value was assigned, and then regardless of success or failure, ApplyModifications returned with the error code, even if it was nil. That skipped other valid Modifications if several of them assigned a NativeToOvs pointer value. Signed-off-by: Andreas Karis <ak.karis@gmail.com> ovn-org/libovsdb#307
…ation tCache.Row() clones the model to get 'existing'. The another explicit Clone() is done to get 'modified' which is passed to ApplyModifications() and updated with the ovsdb row update. If 'modified' and 'existing' are different, then 'modified' is passed to Update() which does two more clones. First for the original row to (presumably?) nothing in the old row changes underneath Update() while it's processing the update. Finally Update() clones 'modified' and assigns that as the new row in the cache. So, we clone the cache row into 'existing', clone that into 'modified', update 'modified', clone 'modified' again to update the cache, and then throw it away. Ideally we could assign 'modified' to the cache instead of clone it again, but we have to pass it to the event handlers and we have to ensure it won't be changed underneath the event handler after we've released the row cache lock. Since Populate2() owns 'modified' (at least until it hands it off to the event handler) we can change a few things around and get rid of the first clone from 'existing' to 'modified'. 'existing'-s only purpose was to enable the model.Equal() comparison to know if we should actually update the cache, but since ApplyModifications() makes the changes it knows when 'modified' is changed. Use that instead of model.Equal(). Lastly, the event handler also used 'existing' but since we're replacing the original model in the row cache with 'modified' we can return the original model for the event handler in place of 'existing' since 'existing' was (previous to this patch) cloned from the original model anyway. All this avoids a Clone() in a hotpath. Testcases and benchmarks added. ovn-org/libovsdb#308
Lift the new value access out of the hot loop and save a bunch of time iterating over large sets. ovn-org/libovsdb#311
…s in hotpaths Arguments to log functions still get evaluated by Go even though the Log() function will never be called due to the V(5). So stop burning a ton of time constructing debug logging strings that will never get printed anywhere, unless we've actually got debug logging enabled. ovn-org/libovsdb#312
openshift-ci
bot
added
the
approved
|
/lgtm |
openshift-ci
bot
added
bugzilla/severity-high
|
@dcbw: This pull request references Bugzilla bug 2092473, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/retest is a CRIO error, not an OVN one.... |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dcbw, flavio-fernandes, tssurya The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/override ci/prow/e2e-aws-ovn-windows until the kube versioning check is figured out |
|
@dcbw: Overrode contexts on behalf of dcbw: ci/prow/e2e-aws-ovn-windows In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/retest-required |
|
@dcbw: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
@dcbw: All pull requests linked via external trackers have merged: Bugzilla bug 2092473 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/cherry-pick release-4.10 |
|
@dcbw: new pull request created: #1124 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Carry a couple upstream libovsdb performance/scale patches
ovn-org/libovsdb#312
ovn-org/libovsdb#311
ovn-org/libovsdb#308
And one bug fix that makes 308 apply cleanly:
ovn-org/libovsdb#307