Skip to content

Optimize benchmarks; permit serialization #88

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jul 19, 2019

Conversation

lavalamp
Copy link
Contributor

@lavalamp lavalamp commented Jul 16, 2019

Prior to these changes:

BenchmarkDeducedSimple-12                                   3000            472665 ns/op          139180 B/op       1810 allocs/op
BenchmarkDeducedNested-12                                   1000           1370363 ns/op          401776 B/op       5146 allocs/op
BenchmarkDeducedNestedAcrossVersion-12                      1000           1435743 ns/op          434426 B/op       5527 allocs/op
BenchmarkLeafConflictAcrossVersion-12                       3000            492026 ns/op          147113 B/op       1848 allocs/op
BenchmarkMultipleApplierRecursiveRealConversion-12          1000           2585529 ns/op          805147 B/op       8457 allocs/op

After these changes:

BenchmarkDeducedSimple-12                                   5000            432763 ns/op          127173 B/op       1703 allocs/op
BenchmarkDeducedNested-12                                   1000           1298198 ns/op          356460 B/op       4678 allocs/op
BenchmarkDeducedNestedAcrossVersion-12                      1000           1359040 ns/op          383408 B/op       5022 allocs/op
BenchmarkLeafConflictAcrossVersion-12                       3000            437574 ns/op          134185 B/op       1783 allocs/op
BenchmarkMultipleApplierRecursiveRealConversion-12           500           2382065 ns/op          732700 B/op       7978 allocs/op

Mostly I was looking at allocations but this is a modest improvement for everything.

This will permit a straightforward serialization. The current map with string key form can't be deserialized in a single step.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 16, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lavalamp

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 16, 2019
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jul 16, 2019
@lavalamp lavalamp changed the title WIP: Optimize benchmarks WIP: Optimize benchmarks; permit serialization Jul 17, 2019
@lavalamp lavalamp changed the title WIP: Optimize benchmarks; permit serialization Optimize benchmarks; permit serialization Jul 17, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 17, 2019
@lavalamp
Copy link
Contributor Author

Antoine is OOO. I think this should be ready to go. I haven't tried this on the main repo but this is definitely not worse than what we have now unless the benchmarks are completely unrepresentative.

/assign @jennybuckley @kwiesmueller

@apelisse
Copy link
Contributor

Benchmarks in this repo use significantly smaller sets than real-life sets. I doubt this is going to be equally good with real-life size objects :-/

@lavalamp
Copy link
Contributor Author

Benchmark comparison:

benchmark                                              old ns/op     new ns/op     delta
BenchmarkDeducedSimple-12                              472665        432763        -8.44%
BenchmarkDeducedNested-12                              1370363       1298198       -5.27%
BenchmarkDeducedNestedAcrossVersion-12                 1435743       1359040       -5.34%
BenchmarkLeafConflictAcrossVersion-12                  492026        437574        -11.07%
BenchmarkMultipleApplierRecursiveRealConversion-12     2585529       2382065       -7.87%

benchmark                                              old allocs     new allocs     delta
BenchmarkDeducedSimple-12                              1810           1703           -5.91%
BenchmarkDeducedNested-12                              5146           4678           -9.09%
BenchmarkDeducedNestedAcrossVersion-12                 5527           5022           -9.14%
BenchmarkLeafConflictAcrossVersion-12                  1848           1783           -3.52%
BenchmarkMultipleApplierRecursiveRealConversion-12     8457           7978           -5.66%

benchmark                                              old bytes     new bytes     delta
BenchmarkDeducedSimple-12                              139180        127173        -8.63%
BenchmarkDeducedNested-12                              401776        356460        -11.28%
BenchmarkDeducedNestedAcrossVersion-12                 434426        383408        -11.74%
BenchmarkLeafConflictAcrossVersion-12                  147113        134185        -8.79%
BenchmarkMultipleApplierRecursiveRealConversion-12     805147        732700        -9.00%

@lavalamp
Copy link
Contributor Author

I'll test in the main repo, then. :'(

@apelisse
Copy link
Contributor

Wouldn't it be better to improve these tests? Not sure what is the best way to make them more real-life size.

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 17, 2019
@lavalamp
Copy link
Contributor Author

The benchmark code here is running a bunch of test assertions, diluting the signal here. After making it not do that, the results are not changed much, maybe slightly worse. Time seems pretty noisy, but I'm looking at allocations.

I started to make the items in the benchmarks bigger but that is a harder task than expected, plus I seem to have hit a bug where field names are not handled right, e.g. letter followed by number has the number stripped.

benchmark                                              old ns/op     new ns/op     delta
BenchmarkDeducedSimple-12                              453044        458392        +1.18%
BenchmarkDeducedNested-12                              1382705       1266794       -8.38%
BenchmarkDeducedNestedAcrossVersion-12                 1501691       1388808       -7.52%
BenchmarkLeafConflictAcrossVersion-12                  465940        454865        -2.38%
BenchmarkMultipleApplierRecursiveRealConversion-12     2498657       2328747       -6.80%

benchmark                                              old allocs     new allocs     delta
BenchmarkDeducedSimple-12                              1810           1703           -5.91%
BenchmarkDeducedNested-12                              5146           4678           -9.09%
BenchmarkDeducedNestedAcrossVersion-12                 5527           5022           -9.14%
BenchmarkLeafConflictAcrossVersion-12                  1848           1783           -3.52%
BenchmarkMultipleApplierRecursiveRealConversion-12     8149           7694           -5.58%

benchmark                                              old bytes     new bytes     delta
BenchmarkDeducedSimple-12                              139179        127172        -8.63%
BenchmarkDeducedNested-12                              401784        356427        -11.29%
BenchmarkDeducedNestedAcrossVersion-12                 434410        383419        -11.74%
BenchmarkLeafConflictAcrossVersion-12                  147112        134184        -8.79%
BenchmarkMultipleApplierRecursiveRealConversion-12     784542        711478        -9.31%

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 17, 2019
@lavalamp
Copy link
Contributor Author

I've added a microbenchmark for set.

benchmark                                  old ns/op     new ns/op     delta
BenchmarkFieldSet/insert-10-12             11498         6663          -42.05%
BenchmarkFieldSet/has-10-12                910           748           -17.80%
BenchmarkFieldSet/union-10-12              4195          4132          -1.50%
BenchmarkFieldSet/intersection-10-12       1903          2790          +46.61%
BenchmarkFieldSet/difference-10-12         1893          3152          +66.51%
BenchmarkFieldSet/insert-20-12             58147         26062         -55.18%
BenchmarkFieldSet/has-20-12                1710          1037          -39.36%
BenchmarkFieldSet/union-20-12              41174         15651         -61.99%
BenchmarkFieldSet/intersection-20-12       8791          7893          -10.21%
BenchmarkFieldSet/difference-20-12         28758         12580         -56.26%
BenchmarkFieldSet/insert-50-12             186330        89396         -52.02%
BenchmarkFieldSet/has-50-12                1978          1354          -31.55%
BenchmarkFieldSet/union-50-12              131180        40162         -69.38%
BenchmarkFieldSet/intersection-50-12       21577         17659         -18.16%
BenchmarkFieldSet/difference-50-12         93593         30367         -67.55%
BenchmarkFieldSet/insert-100-12            676984        278113        -58.92%
BenchmarkFieldSet/has-100-12               2794          2016          -27.85%
BenchmarkFieldSet/union-100-12             262925        94525         -64.05%
BenchmarkFieldSet/intersection-100-12      31648         43703         +38.09%
BenchmarkFieldSet/difference-100-12        162427        74614         -54.06%
BenchmarkFieldSet/insert-500-12            3433305       1595039       -53.54%
BenchmarkFieldSet/has-500-12               3517          2639          -24.96%
BenchmarkFieldSet/union-500-12             1247269       392640        -68.52%
BenchmarkFieldSet/intersection-500-12      214700        183495        -14.53%
BenchmarkFieldSet/difference-500-12        873070        294497        -66.27%
BenchmarkFieldSet/insert-1000-12           8943251       3711783       -58.50%
BenchmarkFieldSet/has-1000-12              4074          2888          -29.11%
BenchmarkFieldSet/union-1000-12            2432131       714658        -70.62%
BenchmarkFieldSet/intersection-1000-12     461434        342822        -25.71%
BenchmarkFieldSet/difference-1000-12       1699348       586396        -65.49%

benchmark                                  old allocs     new allocs     delta
BenchmarkFieldSet/insert-10-12             50             31             -38.00%
BenchmarkFieldSet/has-10-12                4              5              +25.00%
BenchmarkFieldSet/union-10-12              6              21             +250.00%
BenchmarkFieldSet/intersection-10-12       5              19             +280.00%
BenchmarkFieldSet/difference-10-12         4              19             +375.00%
BenchmarkFieldSet/insert-20-12             231            134            -41.99%
BenchmarkFieldSet/has-20-12                8              6              -25.00%
BenchmarkFieldSet/union-20-12              127            85             -33.07%
BenchmarkFieldSet/intersection-20-12       45             61             +35.56%
BenchmarkFieldSet/difference-20-12         102            74             -27.45%
BenchmarkFieldSet/insert-50-12             743            446            -39.97%
BenchmarkFieldSet/has-50-12                9              7              -22.22%
BenchmarkFieldSet/union-50-12              416            196            -52.88%
BenchmarkFieldSet/intersection-50-12       93             126            +35.48%
BenchmarkFieldSet/difference-50-12         302            163            -46.03%
BenchmarkFieldSet/insert-100-12            2462           1338           -45.65%
BenchmarkFieldSet/has-100-12               11             9              -18.18%
BenchmarkFieldSet/union-100-12             1241           435            -64.95%
BenchmarkFieldSet/intersection-100-12      205            305            +48.78%
BenchmarkFieldSet/difference-100-12        818            388            -52.57%
BenchmarkFieldSet/insert-500-12            13424          8013           -40.31%
BenchmarkFieldSet/has-500-12               13             12             -7.69%
BenchmarkFieldSet/union-500-12             6027           2341           -61.16%
BenchmarkFieldSet/intersection-500-12      1215           1604           +32.02%
BenchmarkFieldSet/difference-500-12        4240           1998           -52.88%
BenchmarkFieldSet/insert-1000-12           29646          17782          -40.02%
BenchmarkFieldSet/has-1000-12              14             13             -7.14%
BenchmarkFieldSet/union-1000-12            11738          4413           -62.40%
BenchmarkFieldSet/intersection-1000-12     2198           2848           +29.57%
BenchmarkFieldSet/difference-1000-12       8040           3756           -53.28%

As you can see, it's a massive improvement with a few exceptions, mostly intersection.

Copy link
Contributor

@apelisse apelisse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised with the very high variance (15-25%) across consecutive runs. I looked carefully and I don't see any obvious reasons for it. I'll investigate a little bit more. (I'm a little surprised by some of your old/new benchmark being sometimes better and sometimes worse in a non-linear way).

@@ -598,7 +598,13 @@ func BenchmarkDeducedSimple(b *testing.B) {
},
}

// Make sure this passes...
if err := test.Test(typed.DeducedParseableType); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You kept the same test below in the loop, is that on purpose? (same for other benches below)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah that explains why the numbers didn't change much, haha

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

benchmark                                              old ns/op     new ns/op     delta
BenchmarkDeducedSimple-12                              422922        415645        -1.72%
BenchmarkDeducedNested-12                              1252984       1179542       -5.86%
BenchmarkDeducedNestedAcrossVersion-12                 1395454       1293546       -7.30%
BenchmarkLeafConflictAcrossVersion-12                  412367        410950        -0.34%
BenchmarkMultipleApplierRecursiveRealConversion-12     2481784       2413984       -2.73%

benchmark                                              old allocs     new allocs     delta
BenchmarkDeducedSimple-12                              1634           1533           -6.18%
BenchmarkDeducedNested-12                              4757           4289           -9.84%
BenchmarkDeducedNestedAcrossVersion-12                 5138           4633           -9.83%
BenchmarkLeafConflictAcrossVersion-12                  1672           1613           -3.53%
BenchmarkMultipleApplierRecursiveRealConversion-12     8154           7696           -5.62%

benchmark                                              old bytes     new bytes     delta
BenchmarkDeducedSimple-12                              126676        114318        -9.76%
BenchmarkDeducedNested-12                              368885        323195        -12.39%
BenchmarkDeducedNestedAcrossVersion-12                 401554        350212        -12.79%
BenchmarkLeafConflictAcrossVersion-12                  134616        121344        -9.86%
BenchmarkMultipleApplierRecursiveRealConversion-12     784880        711578        -9.34%

After fixing these (not pushed yet).

randOperand := func() *Set { return operands[rand.Intn(len(operands))] }
b.Run(fmt.Sprintf("union-%v", here.size), func(b *testing.B) {
b.ResetTimer()
b.ReportAllocs()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious if these two are needed

@apelisse
Copy link
Contributor

apelisse commented Jul 18, 2019

I just had this in two consecutive runs:

BenchmarkFieldSet/difference-50-8                 11911         26110         +119.21%

@lavalamp
Copy link
Contributor Author

I tweaked the benchmarks a bit. I did three runs of old and new, which were all very consistent with each other. Here's a random old / new comparison:

benchmark                                  old ns/op     new ns/op     delta
BenchmarkFieldSet/insert-20-12             54252         24868         -54.16%
BenchmarkFieldSet/has-20-12                1597          1306          -18.22%
BenchmarkFieldSet/union-20-12              36040         17464         -51.54%
BenchmarkFieldSet/intersection-20-12       7631          8752          +14.69%
BenchmarkFieldSet/difference-20-12         25865         13741         -46.87%
BenchmarkFieldSet/insert-50-12             149605        90214         -39.70%
BenchmarkFieldSet/has-50-12                1776          1438          -19.03%
BenchmarkFieldSet/union-50-12              95960         37428         -61.00%
BenchmarkFieldSet/intersection-50-12       16027         15331         -4.34%
BenchmarkFieldSet/difference-50-12         67187         28200         -58.03%
BenchmarkFieldSet/insert-100-12            498853        233598        -53.17%
BenchmarkFieldSet/has-100-12               2357          1864          -20.92%
BenchmarkFieldSet/union-100-12             246561        73341         -70.25%
BenchmarkFieldSet/intersection-100-12      34281         33725         -1.62%
BenchmarkFieldSet/difference-100-12        170985        63803         -62.69%
BenchmarkFieldSet/insert-500-12            2797068       1407020       -49.70%
BenchmarkFieldSet/has-500-12               2607          2375          -8.90%
BenchmarkFieldSet/union-500-12             1095431       383595        -64.98%
BenchmarkFieldSet/intersection-500-12      219792        205134        -6.67%
BenchmarkFieldSet/difference-500-12        778033        352579        -54.68%
BenchmarkFieldSet/insert-1000-12           5288591       2782317       -47.39%
BenchmarkFieldSet/has-1000-12              2800          2298          -17.93%
BenchmarkFieldSet/union-1000-12            2039674       692459        -66.05%
BenchmarkFieldSet/intersection-1000-12     436493        352916        -19.15%
BenchmarkFieldSet/difference-1000-12       1457956       738868        -49.32%

benchmark                                  old allocs     new allocs     delta
BenchmarkFieldSet/insert-20-12             227            130            -42.73%
BenchmarkFieldSet/has-20-12                7              6              -14.29%
BenchmarkFieldSet/union-20-12              129            84             -34.88%
BenchmarkFieldSet/intersection-20-12       43             60             +39.53%
BenchmarkFieldSet/difference-20-12         105            73             -30.48%
BenchmarkFieldSet/insert-50-12             729            435            -40.33%
BenchmarkFieldSet/has-50-12                8              7              -12.50%
BenchmarkFieldSet/union-50-12              424            190            -55.19%
BenchmarkFieldSet/intersection-50-12       89             117            +31.46%
BenchmarkFieldSet/difference-50-12         310            157            -49.35%
BenchmarkFieldSet/insert-100-12            2413           1309           -45.75%
BenchmarkFieldSet/has-100-12               10             9              -10.00%
BenchmarkFieldSet/union-100-12             1203           401            -66.67%
BenchmarkFieldSet/intersection-100-12      169            268            +58.58%
BenchmarkFieldSet/difference-100-12        801            357            -55.43%
BenchmarkFieldSet/insert-500-12            13179          7754           -41.16%
BenchmarkFieldSet/has-500-12               12             12             +0.00%
BenchmarkFieldSet/union-500-12             5862           2198           -62.50%
BenchmarkFieldSet/intersection-500-12      1013           1469           +45.01%
BenchmarkFieldSet/difference-500-12        4169           1886           -54.76%
BenchmarkFieldSet/insert-1000-12           29000          17208          -40.66%
BenchmarkFieldSet/has-1000-12              13             13             +0.00%
BenchmarkFieldSet/union-1000-12            11506          4118           -64.21%
BenchmarkFieldSet/intersection-1000-12     1841           2585           +40.41%
BenchmarkFieldSet/difference-1000-12       7994           3492           -56.32%

Even though intersection does more allocations, it's still somehow faster.

@lavalamp
Copy link
Contributor Author

I made another change to permit it to cache the order for PathElement.Keys. It was apparently a huge deal.

benchmark                                  old ns/op     new ns/op     delta
BenchmarkFieldSet/insert-20-12             54252         19315         -64.40%
BenchmarkFieldSet/has-20-12                1597          704           -55.92%
BenchmarkFieldSet/union-20-12              36040         11495         -68.10%
BenchmarkFieldSet/intersection-20-12       7631          5153          -32.47%
BenchmarkFieldSet/difference-20-12         25865         8763          -66.12%
BenchmarkFieldSet/insert-50-12             149605        62701         -58.09%
BenchmarkFieldSet/has-50-12                1776          873           -50.84%
BenchmarkFieldSet/union-50-12              95960         29569         -69.19%
BenchmarkFieldSet/intersection-50-12       16027         10213         -36.28%
BenchmarkFieldSet/difference-50-12         67187         20901         -68.89%
BenchmarkFieldSet/insert-100-12            498853        165516        -66.82%
BenchmarkFieldSet/has-100-12               2357          1287          -45.40%
BenchmarkFieldSet/union-100-12             246561        62777         -74.54%
BenchmarkFieldSet/intersection-100-12      34281         20617         -39.86%
BenchmarkFieldSet/difference-100-12        170985        46939         -72.55%
BenchmarkFieldSet/insert-500-12            2797068       770229        -72.46%
BenchmarkFieldSet/has-500-12               2607          1459          -44.04%
BenchmarkFieldSet/union-500-12             1095431       283355        -74.13%
BenchmarkFieldSet/intersection-500-12      219792        126864        -42.28%
BenchmarkFieldSet/difference-500-12        778033        260655        -66.50%
BenchmarkFieldSet/insert-1000-12           5288591       2120121       -59.91%
BenchmarkFieldSet/has-1000-12              2800          1360          -51.43%
BenchmarkFieldSet/union-1000-12            2039674       540864        -73.48%
BenchmarkFieldSet/intersection-1000-12     436493        213266        -51.14%
BenchmarkFieldSet/difference-1000-12       1457956       394617        -72.93%

benchmark                                  old allocs     new allocs     delta
BenchmarkFieldSet/insert-20-12             227            78             -65.64%
BenchmarkFieldSet/has-20-12                7              2              -71.43%
BenchmarkFieldSet/union-20-12              129            54             -58.14%
BenchmarkFieldSet/intersection-20-12       43             29             -32.56%
BenchmarkFieldSet/difference-20-12         105            43             -59.05%
BenchmarkFieldSet/insert-50-12             729            247            -66.12%
BenchmarkFieldSet/has-50-12                8              2              -75.00%
BenchmarkFieldSet/union-50-12              424            133            -68.63%
BenchmarkFieldSet/intersection-50-12       89             61             -31.46%
BenchmarkFieldSet/difference-50-12         310            101            -67.42%
BenchmarkFieldSet/insert-100-12            2413           813            -66.31%
BenchmarkFieldSet/has-100-12               10             3              -70.00%
BenchmarkFieldSet/union-100-12             1203           257            -78.64%
BenchmarkFieldSet/intersection-100-12      169            124            -26.63%
BenchmarkFieldSet/difference-100-12        801            214            -73.28%
BenchmarkFieldSet/insert-500-12            13179          4141           -68.58%
BenchmarkFieldSet/has-500-12               12             3              -75.00%
BenchmarkFieldSet/union-500-12             5862           1468           -74.96%
BenchmarkFieldSet/intersection-500-12      1013           739            -27.05%
BenchmarkFieldSet/difference-500-12        4169           1162           -72.13%
BenchmarkFieldSet/insert-1000-12           29000          8981           -69.03%
BenchmarkFieldSet/has-1000-12              13             3              -76.92%
BenchmarkFieldSet/union-1000-12            11506          2842           -75.30%
BenchmarkFieldSet/intersection-1000-12     1841           1330           -27.76%
BenchmarkFieldSet/difference-1000-12       7994           2238           -72.00%

@lavalamp
Copy link
Contributor Author

I also looked at a mem & cpu profile while running the benchmarks and I don't think there's anything left that's easy to improve now. The latest change improved the original benchmarks, too:

benchmark                                              old ns/op     new ns/op     delta
BenchmarkDeducedSimple-12                              422922        404477        -4.36%
BenchmarkDeducedNested-12                              1252984       1054765       -15.82%
BenchmarkDeducedNestedAcrossVersion-12                 1395454       1129654       -19.05%
BenchmarkLeafConflictAcrossVersion-12                  412367        393057        -4.68%
BenchmarkMultipleApplierRecursiveRealConversion-12     2481784       2242044       -9.66%

benchmark                                              old allocs     new allocs     delta
BenchmarkDeducedSimple-12                              1634           1525           -6.67%
BenchmarkDeducedNested-12                              4757           4180           -12.13%
BenchmarkDeducedNestedAcrossVersion-12                 5138           4524           -11.95%
BenchmarkLeafConflictAcrossVersion-12                  1672           1605           -4.01%
BenchmarkMultipleApplierRecursiveRealConversion-12     8154           7692           -5.67%

I'd like to squash and merge this, can someone review it first?

lavalamp added 4 commits July 18, 2019 13:33
… on all benchmarks; needs more testing of the lexical operator
This is technically a breaking change, but most people won't construct
this directly so it should be low-impact.
@lavalamp
Copy link
Contributor Author

I squashed this into some sensible commits. @jennybuckley can you LGTM?

Copy link
Contributor

@apelisse apelisse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

if s.members[loc].Equals(pe) {
return
}
n := len(s.members) - 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I meant to switch this to the thing they have listed as the "copy" variant; the other variant does an unnecessary allocation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#89

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 19, 2019
@k8s-ci-robot k8s-ci-robot merged commit f7547e8 into kubernetes-sigs:master Jul 19, 2019
@apelisse
Copy link
Contributor

The final benchmarks are a little underwhelming, compared to the improvements to set operations.

@lavalamp
Copy link
Contributor Author

The final benchmarks are a little underwhelming, compared to the improvements to set operations.

I think the string / yaml conversion is probably dominating the performance in those benchmarks.

@lavalamp
Copy link
Contributor Author

lavalamp commented Jul 19, 2019

I managed to run this in the main repo, will send an update PR there I guess if I can figure out the dependency stuff, but here's the results:

$ benchcmp orig.txt new.txt
benchmark                       old ns/op     new ns/op     delta
BenchmarkApplyNewObject-12      2230683       2129184       -4.55%
BenchmarkUpdateNewObject-12     2624417       2379121       -9.35%
BenchmarkRepeatedUpdate-12      1031653       981264        -4.88%
BenchmarkSetToFields-12         24778         23686         -4.41%
BenchmarkFieldsToSet-12         33876         17971         -46.95%
BenchmarkYAMLToTyped-12         68377         64312         -5.94%

benchmark                       old allocs     new allocs     delta
BenchmarkApplyNewObject-12      8632           7168           -16.96%
BenchmarkUpdateNewObject-12     11210          8396           -25.10%
BenchmarkRepeatedUpdate-12      3057           2887           -5.56%
BenchmarkSetToFields-12         124            124            +0.00%
BenchmarkFieldsToSet-12         147            93             -36.73%
BenchmarkYAMLToTyped-12         150            152            +1.33%

benchmark                       old bytes     new bytes     delta
BenchmarkApplyNewObject-12      555847        500704        -9.92%
BenchmarkUpdateNewObject-12     659671        560262        -15.07%
BenchmarkRepeatedUpdate-12      255150        221120        -13.34%
BenchmarkSetToFields-12         8974          8369          -6.74%
BenchmarkFieldsToSet-12         12897         4688          -63.65%
BenchmarkYAMLToTyped-12         27625         27384         -0.87%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants