Do not use Compare for Equality checks #237
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
approved
Contributor
Author
|
We should take a quick look at how that |
Contributor
Author
|
Fortunately the ordering code is correct because it's tested individually in the typed package. |
apelisse
commented
Jun 23, 2023
| - a | ||
| - aprime | ||
| - b | ||
| - aprime |
Contributor
Author
There was a problem hiding this comment.
I've very carefully inspected these two tests and the change is correct. Ordering isn't broken.
Comment on lines
207
to
209
| if compare.IsSame() { | ||
| newObject = nil | ||
| } |
Member
There was a problem hiding this comment.
I think this is the nil you are talking about?
The ManagedFieldsUpdater is the last in the FieldManager chain. It seems responsible for handling the returned nil:
Member
There was a problem hiding this comment.
So it doesnt seem like at least for k8s usage this is that risky. The nil is bubbled through the field manager chain until it is actualized into the old value.
Contributor
Author
There was a problem hiding this comment.
So object is always going to be non-nil, so the timestamp will always be updated, should we compare the value and see if they changed?
Contributor
Author
There was a problem hiding this comment.
Instead of doing a compare here, I could do a value.Equals and keep the exact same behavior, which would be a little less risky and easier to backport.
Member
There was a problem hiding this comment.
I dont think it matters to perform the comparison. The nil only goes through StipMetaManager and ManagedFieldsUpdater before becoming non-nil, and neither of those care about it being the same object.
Member
There was a problem hiding this comment.
I see the timestamp update is conditional on the object returned being nil, but I think that can be made unconditional due to the no-op modulo timestamp check inside the store.
| } | ||
| tvu := convertMapAnyToMapString(tv.AsValue().Unstructured()) | ||
| liveu := convertMapAnyToMapString(live.AsValue().Unstructured()) | ||
| return cmp.Diff(tvu, liveu), nil |
Member
There was a problem hiding this comment.
I'm sus about how cmp.Diff equality check works on the values here. I'd be more comfortable adding options to the existing Compare to get the desired behavior. Would also save us a conversion to unstructured for old/new on every write.
Contributor
Author
There was a problem hiding this comment.
well, this is a test here and cmp.Diff is convenient to see what's different. I can use a Comparer that uses Value.Equals if you think it's safer? Is that what you meant?
Member
There was a problem hiding this comment.
ah did not realize this was a test. as you were. resolved
apelisse
force-pushed
the
proper-comparison
branch
2 times, most recently
from
bec4274 to
255cc0b
Compare
Contributor
Author
|
Updated, PTAL |
merge/update.go
Outdated
| return nil, fieldpath.ManagedFields{}, err | ||
| } | ||
| if compare.IsSame() { | ||
| if value.Equals(liveObject.AsValue(), newObject.AsValue()) { |
Contributor
Author
There was a problem hiding this comment.
Yes, benchmarks at the top ... :-(
Contributor
Author
There was a problem hiding this comment.
Not sure why I'm not using the allocator here...
Member
|
I am not sure it is worth the performance cost especially if we suspect it may not be necessary (and the primary motivation is for testing) |
apelisse
force-pushed
the
proper-comparison
branch
from
255cc0b to
3f26607
Compare
apelisse
changed the title
Contributor
Author
|
Updated text and updated the benchmarks accordingly. |
Currently, we use the Compare function to compare objects, but this doesn't look at the actual differences between objects, it collects the fields from each object using smd logic and then compares that. This has a couple of drawbacks: 1. Order is not preserved, which means anything that depends on order is wrong! 2. When we decide to allow invalid duplicates, we won't be able to compare such objects. Use value.Equals to compare objects so that we have proper comparison, and cmp.Diff in tests to highlight the differences. As a consequence of 1, this has identified an interesting bug: we used the comparison to see if two objects (before and after) were equal to return nil as an output of `Apply`, but that logic doesn't check if the order has changed, which caused some tests to fail (since only the order had changed, and that change was ignored, the object wasn't updated). Using Equals instead of re-using the compare result in applying has an extra cost that can be noticed on ApplyTwice test-suite, as shown below: ``` name old time/op new time/op delta DeducedSimple-8 60.7µs ± 0% 62.1µs ± 0% +2.21% (p=0.008 n=5+5) DeducedNested-8 131µs ± 0% 131µs ± 0% -0.19% (p=0.008 n=5+5) DeducedNestedAcrossVersion-8 171µs ± 0% 174µs ± 0% +1.89% (p=0.008 n=5+5) LeafConflictAcrossVersion-8 83.2µs ± 0% 83.2µs ± 0% +0.11% (p=0.008 n=5+5) MultipleApplierRecursiveRealConversion-8 1.15ms ± 0% 1.17ms ± 0% +1.59% (p=0.008 n=5+5) Operations/Pod/Create-8 72.5µs ± 0% 70.9µs ± 0% -2.29% (p=0.008 n=5+5) Operations/Pod/Apply-8 184µs ± 0% 182µs ± 0% -0.81% (p=0.008 n=5+5) Operations/Pod/ApplyTwice-8 416µs ± 0% 457µs ± 0% +10.03% (p=0.008 n=5+5) Operations/Pod/Update-8 181µs ± 0% 178µs ± 0% -1.62% (p=0.008 n=5+5) Operations/Pod/UpdateVersion-8 259µs ± 0% 256µs ± 0% -0.86% (p=0.008 n=5+5) Operations/Node/Create-8 110µs ± 0% 111µs ± 0% +1.18% (p=0.008 n=5+5) Operations/Node/Apply-8 283µs ± 0% 275µs ± 0% -2.97% (p=0.008 n=5+5) Operations/Node/ApplyTwice-8 661µs ± 0% 685µs ± 0% +3.64% (p=0.008 n=5+5) Operations/Node/Update-8 344µs ± 0% 275µs ± 0% -20.20% (p=0.008 n=5+5) Operations/Node/UpdateVersion-8 383µs ± 0% 401µs ± 0% +4.60% (p=0.008 n=5+5) Operations/Endpoints/Create-8 7.46µs ± 0% 9.98µs ± 0% +33.75% (p=0.008 n=5+5) Operations/Endpoints/Apply-8 16.6µs ± 0% 19.0µs ± 0% +14.22% (p=0.008 n=5+5) Operations/Endpoints/ApplyTwice-8 930µs ± 0% 4300µs ± 0% +362.33% (p=0.008 n=5+5) Operations/Endpoints/Update-8 882µs ± 0% 1241µs ± 0% +40.70% (p=0.008 n=5+5) Operations/Endpoints/UpdateVersion-8 1.90ms ± 0% 2.24ms ± 0% +17.77% (p=0.008 n=5+5) Operations/Node100%override/Create-8 108µs ± 0% 130µs ± 0% +20.77% (p=0.008 n=5+5) Operations/Node100%override/Apply-8 273µs ± 0% 307µs ± 0% +12.56% (p=0.008 n=5+5) Operations/Node100%override/ApplyTwice-8 630µs ± 0% 688µs ± 0% +9.26% (p=0.008 n=5+5) Operations/Node100%override/Update-8 281µs ± 0% 290µs ± 0% +3.23% (p=0.008 n=5+5) Operations/Node100%override/UpdateVersion-8 402µs ± 0% 416µs ± 0% +3.40% (p=0.008 n=5+5) Operations/Node10%override/Create-8 110µs ± 0% 119µs ± 0% +7.80% (p=0.008 n=5+5) Operations/Node10%override/Apply-8 272µs ± 0% 307µs ± 0% +12.74% (p=0.008 n=5+5) Operations/Node10%override/ApplyTwice-8 640µs ± 0% 711µs ± 0% +11.19% (p=0.008 n=5+5) Operations/Node10%override/Update-8 272µs ± 0% 281µs ± 0% +3.12% (p=0.008 n=5+5) Operations/Node10%override/UpdateVersion-8 407µs ± 0% 472µs ± 0% +16.00% (p=0.008 n=5+5) Operations/Endpoints100%override/Create-8 7.66µs ± 0% 8.93µs ± 0% +16.56% (p=0.008 n=5+5) Operations/Endpoints100%override/Apply-8 16.4µs ± 0% 22.3µs ± 0% +35.78% (p=0.008 n=5+5) Operations/Endpoints100%override/ApplyTwice-8 889µs ± 0% 2445µs ± 0% +175.06% (p=0.008 n=5+5) Operations/Endpoints100%override/Update-8 872µs ± 0% 1009µs ± 0% +15.68% (p=0.008 n=5+5) Operations/Endpoints100%override/UpdateVersion-8 1.97ms ± 0% 2.28ms ± 0% +15.72% (p=0.008 n=5+5) Operations/Endpoints10%override/Create-8 7.62µs ± 0% 11.83µs ± 0% +55.25% (p=0.008 n=5+5) Operations/Endpoints10%override/Apply-8 16.5µs ± 0% 26.4µs ± 0% +59.77% (p=0.008 n=5+5) Operations/Endpoints10%override/ApplyTwice-8 901µs ± 0% 4497µs ± 0% +399.05% (p=0.008 n=5+5) Operations/Endpoints10%override/Update-8 879µs ± 0% 1034µs ± 0% +17.57% (p=0.008 n=5+5) Operations/Endpoints10%override/UpdateVersion-8 1.96ms ± 0% 2.48ms ± 0% +26.24% (p=0.008 n=5+5) Operations/PrometheusCRD/Create-8 646µs ± 0% 625µs ± 0% -3.21% (p=0.008 n=5+5) Operations/PrometheusCRD/Apply-8 1.54ms ± 0% 1.57ms ± 0% +2.12% (p=0.008 n=5+5) Operations/PrometheusCRD/ApplyTwice-8 3.61ms ± 0% 4.23ms ± 0% +16.93% (p=0.008 n=5+5) Operations/PrometheusCRD/Update-8 1.56ms ± 0% 1.68ms ± 0% +7.56% (p=0.008 n=5+5) Operations/PrometheusCRD/UpdateVersion-8 2.29ms ± 0% 2.48ms ± 0% +8.42% (p=0.008 n=5+5) Operations/apiresourceimport/Create-8 3.61ms ± 0% 3.05ms ± 0% -15.32% (p=0.008 n=5+5) Operations/apiresourceimport/Apply-8 8.12ms ± 0% 8.41ms ± 0% +3.64% (p=0.008 n=5+5) Operations/apiresourceimport/ApplyTwice-8 16.3ms ± 0% 17.1ms ± 0% +4.42% (p=0.008 n=5+5) Operations/apiresourceimport/Update-8 4.88ms ± 0% 5.00ms ± 0% +2.57% (p=0.008 n=5+5) Operations/apiresourceimport/UpdateVersion-8 6.50ms ± 0% 6.87ms ± 0% +5.80% (p=0.008 n=5+5) name old alloc/op new alloc/op delta DeducedSimple-8 29.7kB ± 0% 30.4kB ± 0% +2.35% (p=0.008 n=5+5) DeducedNested-8 58.3kB ± 0% 59.1kB ± 0% +1.39% (p=0.008 n=5+5) DeducedNestedAcrossVersion-8 78.2kB ± 0% 79.1kB ± 0% +1.16% (p=0.008 n=5+5) LeafConflictAcrossVersion-8 42.3kB ± 0% 43.0kB ± 0% +1.69% (p=0.008 n=5+5) MultipleApplierRecursiveRealConversion-8 714kB ± 0% 716kB ± 0% +0.29% (p=0.008 n=5+5) Operations/Pod/Create-8 21.3kB ± 0% 21.3kB ± 0% +0.02% (p=0.008 n=5+5) Operations/Pod/Apply-8 52.8kB ± 0% 53.1kB ± 0% +0.56% (p=0.008 n=5+5) Operations/Pod/ApplyTwice-8 107kB ± 0% 110kB ± 0% +3.60% (p=0.008 n=5+5) Operations/Pod/Update-8 40.0kB ± 0% 40.0kB ± 0% -0.01% (p=0.008 n=5+5) Operations/Pod/UpdateVersion-8 52.6kB ± 0% 52.6kB ± 0% +0.03% (p=0.008 n=5+5) Operations/Node/Create-8 30.2kB ± 0% 30.2kB ± 0% -0.01% (p=0.008 n=5+5) Operations/Node/Apply-8 73.8kB ± 0% 74.1kB ± 0% +0.38% (p=0.008 n=5+5) Operations/Node/ApplyTwice-8 149kB ± 0% 155kB ± 0% +4.24% (p=0.008 n=5+5) Operations/Node/Update-8 57.4kB ± 0% 57.4kB ± 0% -0.03% (p=0.008 n=5+5) Operations/Node/UpdateVersion-8 75.9kB ± 0% 75.9kB ± 0% +0.01% (p=0.008 n=5+5) Operations/Endpoints/Create-8 3.84kB ± 0% 3.84kB ± 0% ~ (all equal) Operations/Endpoints/Apply-8 6.70kB ± 0% 6.99kB ± 0% +4.28% (p=0.008 n=5+5) Operations/Endpoints/ApplyTwice-8 110kB ± 0% 208kB ± 0% +88.69% (p=0.008 n=5+5) Operations/Endpoints/Update-8 104kB ± 0% 104kB ± 0% +0.00% (p=0.008 n=5+5) Operations/Endpoints/UpdateVersion-8 203kB ± 0% 202kB ± 0% -0.05% (p=0.008 n=5+5) Operations/Node100%override/Create-8 30.2kB ± 0% 30.2kB ± 0% -0.01% (p=0.008 n=5+5) Operations/Node100%override/Apply-8 73.8kB ± 0% 74.1kB ± 0% +0.43% (p=0.008 n=5+5) Operations/Node100%override/ApplyTwice-8 149kB ± 0% 155kB ± 0% +4.19% (p=0.008 n=5+5) Operations/Node100%override/Update-8 57.4kB ± 0% 57.4kB ± 0% -0.05% (p=0.008 n=5+5) Operations/Node100%override/UpdateVersion-8 75.9kB ± 0% 75.9kB ± 0% -0.01% (p=0.008 n=5+5) Operations/Node10%override/Create-8 30.2kB ± 0% 30.2kB ± 0% -0.01% (p=0.008 n=5+5) Operations/Node10%override/Apply-8 73.8kB ± 0% 74.1kB ± 0% +0.36% (p=0.008 n=5+5) Operations/Node10%override/ApplyTwice-8 149kB ± 0% 155kB ± 0% +4.17% (p=0.008 n=5+5) Operations/Node10%override/Update-8 57.4kB ± 0% 57.4kB ± 0% -0.02% (p=0.008 n=5+5) Operations/Node10%override/UpdateVersion-8 75.9kB ± 0% 75.9kB ± 0% +0.01% (p=0.008 n=5+5) Operations/Endpoints100%override/Create-8 3.84kB ± 0% 3.84kB ± 0% ~ (all equal) Operations/Endpoints100%override/Apply-8 6.70kB ± 0% 6.99kB ± 0% +4.30% (p=0.008 n=5+5) Operations/Endpoints100%override/ApplyTwice-8 110kB ± 0% 208kB ± 0% +88.60% (p=0.008 n=5+5) Operations/Endpoints100%override/Update-8 104kB ± 0% 104kB ± 0% -0.01% (p=0.008 n=5+5) Operations/Endpoints100%override/UpdateVersion-8 203kB ± 0% 203kB ± 0% -0.03% (p=0.008 n=5+5) Operations/Endpoints10%override/Create-8 3.84kB ± 0% 3.84kB ± 0% +0.03% (p=0.008 n=5+5) Operations/Endpoints10%override/Apply-8 6.70kB ± 0% 6.99kB ± 0% +4.32% (p=0.008 n=5+5) Operations/Endpoints10%override/ApplyTwice-8 110kB ± 0% 208kB ± 0% +88.58% (p=0.008 n=5+5) Operations/Endpoints10%override/Update-8 104kB ± 0% 104kB ± 0% +0.01% (p=0.008 n=5+5) Operations/Endpoints10%override/UpdateVersion-8 203kB ± 0% 203kB ± 0% -0.02% (p=0.008 n=5+5) Operations/PrometheusCRD/Create-8 165kB ± 0% 165kB ± 0% ~ (all equal) Operations/PrometheusCRD/Apply-8 471kB ± 0% 471kB ± 0% +0.13% (p=0.008 n=5+5) Operations/PrometheusCRD/ApplyTwice-8 935kB ± 0% 983kB ± 0% +5.11% (p=0.008 n=5+5) Operations/PrometheusCRD/Update-8 318kB ± 0% 318kB ± 0% -0.01% (p=0.008 n=5+5) Operations/PrometheusCRD/UpdateVersion-8 425kB ± 0% 424kB ± 0% -0.11% (p=0.008 n=5+5) Operations/apiresourceimport/Create-8 679kB ± 0% 679kB ± 0% -0.00% (p=0.008 n=5+5) Operations/apiresourceimport/Apply-8 1.94MB ± 0% 1.94MB ± 0% +0.11% (p=0.008 n=5+5) Operations/apiresourceimport/ApplyTwice-8 3.56MB ± 0% 3.65MB ± 0% +2.42% (p=0.008 n=5+5) Operations/apiresourceimport/Update-8 1.02MB ± 0% 1.02MB ± 0% +0.00% (p=0.008 n=5+5) Operations/apiresourceimport/UpdateVersion-8 1.35MB ± 0% 1.35MB ± 0% +0.01% (p=0.008 n=5+5) name old allocs/op new allocs/op delta DeducedSimple-8 703 ± 0% 724 ± 0% +2.99% (p=0.008 n=5+5) DeducedNested-8 1.34k ± 0% 1.36k ± 0% +1.72% (p=0.008 n=5+5) DeducedNestedAcrossVersion-8 1.83k ± 0% 1.85k ± 0% +1.37% (p=0.008 n=5+5) LeafConflictAcrossVersion-8 943 ± 0% 964 ± 0% +2.23% (p=0.008 n=5+5) MultipleApplierRecursiveRealConversion-8 7.57k ± 0% 7.62k ± 0% +0.67% (p=0.008 n=5+5) Operations/Pod/Create-8 481 ± 0% 481 ± 0% ~ (all equal) Operations/Pod/Apply-8 1.26k ± 0% 1.27k ± 0% +0.63% (p=0.008 n=5+5) Operations/Pod/ApplyTwice-8 2.62k ± 0% 2.72k ± 0% +3.97% (p=0.008 n=5+5) Operations/Pod/Update-8 976 ± 0% 976 ± 0% ~ (all equal) Operations/Pod/UpdateVersion-8 1.40k ± 0% 1.40k ± 0% ~ (all equal) Operations/Node/Create-8 565 ± 0% 565 ± 0% ~ (all equal) Operations/Node/Apply-8 1.57k ± 0% 1.58k ± 0% +0.51% (p=0.008 n=5+5) Operations/Node/ApplyTwice-8 3.37k ± 0% 3.56k ± 0% +5.58% (p=0.008 n=5+5) Operations/Node/Update-8 1.26k ± 0% 1.26k ± 0% -0.08% (p=0.008 n=5+5) Operations/Node/UpdateVersion-8 1.87k ± 0% 1.87k ± 0% ~ (all equal) Operations/Endpoints/Create-8 84.0 ± 0% 84.0 ± 0% ~ (all equal) Operations/Endpoints/Apply-8 156 ± 0% 164 ± 0% +5.13% (p=0.008 n=5+5) Operations/Endpoints/ApplyTwice-8 2.35k ± 0% 4.40k ± 0% +87.52% (p=0.008 n=5+5) Operations/Endpoints/Update-8 2.20k ± 0% 2.20k ± 0% ~ (all equal) Operations/Endpoints/UpdateVersion-8 4.27k ± 0% 4.27k ± 0% -0.02% (p=0.008 n=5+5) Operations/Node100%override/Create-8 565 ± 0% 565 ± 0% ~ (all equal) Operations/Node100%override/Apply-8 1.57k ± 0% 1.58k ± 0% +0.57% (p=0.008 n=5+5) Operations/Node100%override/ApplyTwice-8 3.37k ± 0% 3.56k ± 0% +5.55% (p=0.008 n=5+5) Operations/Node100%override/Update-8 1.26k ± 0% 1.26k ± 0% -0.08% (p=0.008 n=5+5) Operations/Node100%override/UpdateVersion-8 1.87k ± 0% 1.87k ± 0% ~ (all equal) Operations/Node10%override/Create-8 565 ± 0% 565 ± 0% ~ (all equal) Operations/Node10%override/Apply-8 1.57k ± 0% 1.58k ± 0% +0.51% (p=0.008 n=5+5) Operations/Node10%override/ApplyTwice-8 3.37k ± 0% 3.56k ± 0% +5.52% (p=0.008 n=5+5) Operations/Node10%override/Update-8 1.26k ± 0% 1.26k ± 0% -0.08% (p=0.008 n=5+5) Operations/Node10%override/UpdateVersion-8 1.87k ± 0% 1.87k ± 0% +0.05% (p=0.008 n=5+5) Operations/Endpoints100%override/Create-8 84.0 ± 0% 84.0 ± 0% ~ (all equal) Operations/Endpoints100%override/Apply-8 156 ± 0% 164 ± 0% +5.13% (p=0.008 n=5+5) Operations/Endpoints100%override/ApplyTwice-8 2.35k ± 0% 4.40k ± 0% +87.44% (p=0.008 n=5+5) Operations/Endpoints100%override/Update-8 2.20k ± 0% 2.20k ± 0% ~ (all equal) Operations/Endpoints100%override/UpdateVersion-8 4.27k ± 0% 4.27k ± 0% ~ (all equal) Operations/Endpoints10%override/Create-8 84.0 ± 0% 84.0 ± 0% ~ (all equal) Operations/Endpoints10%override/Apply-8 156 ± 0% 164 ± 0% +5.13% (p=0.008 n=5+5) Operations/Endpoints10%override/ApplyTwice-8 2.35k ± 0% 4.40k ± 0% +87.47% (p=0.008 n=5+5) Operations/Endpoints10%override/Update-8 2.20k ± 0% 2.20k ± 0% ~ (all equal) Operations/Endpoints10%override/UpdateVersion-8 4.27k ± 0% 4.27k ± 0% ~ (all equal) Operations/PrometheusCRD/Create-8 3.68k ± 0% 3.68k ± 0% ~ (all equal) Operations/PrometheusCRD/Apply-8 10.2k ± 0% 10.2k ± 0% +0.10% (p=0.008 n=5+5) Operations/PrometheusCRD/ApplyTwice-8 20.0k ± 0% 21.1k ± 0% +5.56% (p=0.008 n=5+5) Operations/PrometheusCRD/Update-8 6.98k ± 0% 6.98k ± 0% ~ (all equal) Operations/PrometheusCRD/UpdateVersion-8 10.2k ± 0% 10.2k ± 0% -0.05% (p=0.008 n=5+5) Operations/apiresourceimport/Create-8 15.4k ± 0% 15.4k ± 0% ~ (all equal) Operations/apiresourceimport/Apply-8 43.0k ± 0% 43.0k ± 0% +0.06% (p=0.008 n=5+5) Operations/apiresourceimport/ApplyTwice-8 81.7k ± 0% 83.8k ± 0% +2.56% (p=0.008 n=5+5) Operations/apiresourceimport/Update-8 26.3k ± 0% 26.3k ± 0% ~ (all equal) Operations/apiresourceimport/UpdateVersion-8 37.3k ± 0% 37.3k ± 0% ~ (all equal) ```
apelisse
force-pushed
the
proper-comparison
branch
from
3f26607 to
6310a8a
Compare
k8s-ci-robot
added
size/L
Member
alexzielenski
left a comment
alexzielenski
left a comment
There was a problem hiding this comment.
/lgtm
/hold
in case you want to change some things
merge/update.go
Outdated
| // way of doing that. Create this flag to stop it. | ||
| // Comparing has become more expensive too now that we're not using | ||
| // `Compare` but `value.Equals` so this gives an option to avoid it. | ||
| StopComparingAfterApply atomic.Bool |
Member
There was a problem hiding this comment.
Hate to nit on naming but I think something like ApplyAllowUnchangedResult is more apt
Contributor
Author
There was a problem hiding this comment.
Thanks for nitting on this actually, I hate the name I was just trying to make you have an opinion, it worked :-)
ApplyAllowUnchangedResult is that accurate? Both allow unchanged result? ReturnNilOnApplyNoop? Also it has to be enabled by default so unfortunately we have to make it negative somehow.
merge/update.go
Outdated
| // way of doing that. Create this flag to stop it. | ||
| // Comparing has become more expensive too now that we're not using | ||
| // `Compare` but `value.Equals` so this gives an option to avoid it. | ||
| StopComparingAfterApply atomic.Bool |
Member
There was a problem hiding this comment.
why does this need to be atomic
Contributor
Author
There was a problem hiding this comment.
because it's not in the constructor so it seems like anyone can change it at anytime which is awful, so ... what do you suggest?
Member
There was a problem hiding this comment.
Might've suggesting adding a way to put it into constructor but that wouldnt be api compatible. Was just curious
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
apelisse
force-pushed
the
proper-comparison
branch
from
2c65cbb to
bb08934
Compare
k8s-ci-robot
removed
the
lgtm
Currently we have an expensive (especially since the parent commit) comparison after applying an object. This is mostly unuseful considering that we have a piece of code in kubernetes that removes entries that are no-ops. Provide a new flag so that the comparison can be disabled, especially since the comparison is done somewhere else, and the comparison is not good enough anyway. ``` BenchmarkOperations/Pod/ApplyTwice-8 267 437527 ns/op 110382 B/op 2722 allocs/op BenchmarkOperations/Pod/ApplyTwiceNoCompare-8 289 412029 ns/op 106516 B/op 2619 allocs/op BenchmarkOperations/Node/ApplyTwice-8 176 828565 ns/op 155376 B/op 3557 allocs/op BenchmarkOperations/Node/ApplyTwiceNoCompare-8 100 1179327 ns/op 149046 B/op 3370 allocs/op BenchmarkOperations/Endpoints/ApplyTwice-8 50 2268639 ns/op 208438 B/op 4402 allocs/op BenchmarkOperations/Endpoints/ApplyTwiceNoCompare-8 126 921992 ns/op 110419 B/op 2347 allocs/op BenchmarkOperations/Node100%override/ApplyTwice-8 174 669942 ns/op 155305 B/op 3556 allocs/op BenchmarkOperations/Node100%override/ApplyTwiceNoCompare-8 190 634832 ns/op 149025 B/op 3369 allocs/op BenchmarkOperations/Node10%override/ApplyTwice-8 157 691645 ns/op 155316 B/op 3557 allocs/op BenchmarkOperations/Node10%override/ApplyTwiceNoCompare-8 186 635949 ns/op 149036 B/op 3370 allocs/op BenchmarkOperations/Endpoints100%override/ApplyTwice-8 63 1811443 ns/op 208261 B/op 4400 allocs/op BenchmarkOperations/Endpoints100%override/ApplyTwiceNoCompare-8 127 932614 ns/op 110431 B/op 2347 allocs/op BenchmarkOperations/Endpoints10%override/ApplyTwice-8 58 1903112 ns/op 208359 B/op 4401 allocs/op BenchmarkOperations/Endpoints10%override/ApplyTwiceNoCompare-8 124 957186 ns/op 110426 B/op 2347 allocs/op BenchmarkOperations/PrometheusCRD/ApplyTwice-8 30 3843649 ns/op 983421 B/op 21105 allocs/op BenchmarkOperations/PrometheusCRD/ApplyTwiceNoCompare-8 34 3500118 ns/op 935164 B/op 19987 allocs/op BenchmarkOperations/apiresourceimport/ApplyTwice-8 7 15743401 ns/op 3650892 B/op 83842 allocs/op BenchmarkOperations/apiresourceimport/ApplyTwiceNoCompare-8 7 15123009 ns/op 3566088 B/op 81776 allocs/op ```
apelisse
force-pushed
the
proper-comparison
branch
from
bb08934 to
d1b78eb
Compare
k8s-ci-robot
added
the
lgtm
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexzielenski, apelisse The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Contributor
Author
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, we use the Compare function to compare objects, but this
doesn't look at the actual differences between objects, it collects the
fields from each object using smd logic and then compares that. This has
a couple of drawbacks:
wrong!
compare such objects.
Use value.Equals to compare objects so that we have proper comparison,
and cmp.Diff in tests to highlight the differences.
As a consequence of 1, this has identified an interesting bug: we used
the comparison to see if two objects (before and after) were equal to
return nil as an output of
Apply, but that logic doesn't check if theorder has changed, which caused some tests to fail (since only the order
had changed, and that change was ignored, the object wasn't updated).
Using Equals instead of re-using the compare result in applying has an extra
cost that can be noticed on ApplyTwice test-suite, as shown below: