set serialization #90

lavalamp · 2019-07-21T21:54:52Z

This is not ready to merge yet. I need to clean up / integrate and optimize the code I copied.

This should actually be a drop-in replacement for the current JSON serialization in the main repo...

Initial benchmark follows, I expect to significantly beat this shortly:

BenchmarkFieldSet/serialize-20-12                  30000             63898 ns/op           14104 B/op        188 allocs/op
BenchmarkFieldSet/deserialize-20-12                10000            112561 ns/op           21864 B/op        400 allocs/op
BenchmarkFieldSet/serialize-50-12                  10000            143026 ns/op           31207 B/op        513 allocs/op
BenchmarkFieldSet/deserialize-50-12                 5000            251731 ns/op           53827 B/op       1128 allocs/op
BenchmarkFieldSet/serialize-100-12                  5000            408637 ns/op          100048 B/op       1740 allocs/op
BenchmarkFieldSet/deserialize-100-12                2000            718344 ns/op          174620 B/op       3902 allocs/op
BenchmarkFieldSet/serialize-500-12                  1000           2086484 ns/op          503934 B/op       9237 allocs/op
BenchmarkFieldSet/deserialize-500-12                 500           3413733 ns/op          850048 B/op      19521 allocs/op
BenchmarkFieldSet/serialize-1000-12                  500           4234109 ns/op         1098302 B/op      20014 allocs/op
BenchmarkFieldSet/deserialize-1000-12                200           6758817 ns/op         1851899 B/op      42475 allocs/op

k8s-ci-robot · 2019-07-21T21:54:57Z

[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:

~~OWNERS~~ [lavalamp]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

lavalamp · 2019-07-22T16:37:35Z

I'm pretty sure I can still cut down on the allocations a ton. Still WIP.

benchmark                                 old ns/op     new ns/op     delta
BenchmarkFieldSet/serialize-20-12         63898         26510         -58.51%
BenchmarkFieldSet/deserialize-20-12       112561        91106         -19.06%
BenchmarkFieldSet/serialize-50-12         143026        57427         -59.85%
BenchmarkFieldSet/deserialize-50-12       251731        204589        -18.73%
BenchmarkFieldSet/serialize-100-12        408637        169586        -58.50%
BenchmarkFieldSet/deserialize-100-12      718344        581129        -19.10%
BenchmarkFieldSet/serialize-500-12        2086484       905256        -56.61%
BenchmarkFieldSet/deserialize-500-12      3413733       2712487       -20.54%
BenchmarkFieldSet/serialize-1000-12       4234109       1959082       -53.73%
BenchmarkFieldSet/deserialize-1000-12     6758817       5710877       -15.50%

benchmark                                 old allocs     new allocs     delta
BenchmarkFieldSet/serialize-20-12         188            111            -40.96%
BenchmarkFieldSet/deserialize-20-12       400            349            -12.75%
BenchmarkFieldSet/serialize-50-12         513            301            -41.33%
BenchmarkFieldSet/deserialize-50-12       1128           974            -13.65%
BenchmarkFieldSet/serialize-100-12        1740           1029           -40.86%
BenchmarkFieldSet/deserialize-100-12      3902           3416           -12.46%
BenchmarkFieldSet/serialize-500-12        9237           5696           -38.33%
BenchmarkFieldSet/deserialize-500-12      19521          17103          -12.39%
BenchmarkFieldSet/serialize-1000-12       20014          12371          -38.19%
BenchmarkFieldSet/deserialize-1000-12     42475          37159          -12.52%

lavalamp · 2019-07-22T23:02:47Z

This is still doing way more allocations than it should. Now:

benchmark                                 old ns/op     new ns/op     delta
BenchmarkFieldSet/serialize-20-12         63898         27979         -56.21%
BenchmarkFieldSet/deserialize-20-12       112561        41165         -63.43%
BenchmarkFieldSet/serialize-50-12         143026        60013         -58.04%
BenchmarkFieldSet/deserialize-50-12       251731        94645         -62.40%
BenchmarkFieldSet/serialize-100-12        408637        169823        -58.44%
BenchmarkFieldSet/deserialize-100-12      718344        257874        -64.10%
BenchmarkFieldSet/serialize-500-12        2086484       928576        -55.50%
BenchmarkFieldSet/deserialize-500-12      3413733       1199376       -64.87%
BenchmarkFieldSet/serialize-1000-12       4234109       1852152       -56.26%
BenchmarkFieldSet/deserialize-1000-12     6758817       2360565       -65.07%

benchmark                                 old allocs     new allocs     delta
BenchmarkFieldSet/serialize-20-12         188            111            -40.96%
BenchmarkFieldSet/deserialize-20-12       400            239            -40.25%
BenchmarkFieldSet/serialize-50-12         513            301            -41.33%
BenchmarkFieldSet/deserialize-50-12       1128           673            -40.34%
BenchmarkFieldSet/serialize-100-12        1740           1029           -40.86%
BenchmarkFieldSet/deserialize-100-12      3902           2401           -38.47%
BenchmarkFieldSet/serialize-500-12        9237           5698           -38.31%
BenchmarkFieldSet/deserialize-500-12      19521          12006          -38.50%
BenchmarkFieldSet/serialize-1000-12       20014          12362          -38.23%
BenchmarkFieldSet/deserialize-1000-12     42475          26186          -38.35%

lavalamp · 2019-07-23T00:57:30Z

It'd be easier to get the allocations down further if we changed the format a bit. Right now encoding things to strings (and back) means an allocation for each key. Fixing is possible but probably needs some surgery to jsoniter.

It's a bit weird, but we could do that by leveraging JSON arrays instead of objects. Instead of objects that are lists of (key, subobject) pairs, we could do a list of lists-- ({key type: key}, subobject) pairs.

[
  [{"k": {"port":443,"protocol":"tcp"}}, "."],
  [{"f": "spec"}, [
    [{"f": "replicas"}, "."]
  ]
]

@apelisse @jennybuckley what do you think?

lavalamp · 2019-07-23T04:49:40Z

OK I have reasonable names and tests now. Does anyone want to look before I squash?

lavalamp · 2019-07-23T05:08:40Z

In the main repo, doing nothing but switching in the path element serialization functions here, we get

benchmark                       old ns/op     new ns/op     delta
BenchmarkApplyNewObject-12      2065260       2130809       +3.17%
BenchmarkUpdateNewObject-12     2415312       2348243       -2.78%
BenchmarkRepeatedUpdate-12      967089        945741        -2.21%
BenchmarkSetToFields-12         22058         19687         -10.75%
BenchmarkFieldsToSet-12         15648         8933          -42.91%
BenchmarkYAMLToTyped-12         65407         66945         +2.35%

benchmark                       old allocs     new allocs     delta
BenchmarkApplyNewObject-12      7168           7264           +1.34%
BenchmarkUpdateNewObject-12     8396           8510           +1.36%
BenchmarkRepeatedUpdate-12      2887           3024           +4.75%
BenchmarkSetToFields-12         130            130            +0.00%
BenchmarkFieldsToSet-12         91             70             -23.08%
BenchmarkYAMLToTyped-12         152            152            +0.00%

apelisse · 2019-07-23T08:23:21Z

It's a bit weird, but we could do that by leveraging JSON arrays instead of objects. Instead of objects that are lists of (key, subobject) pairs, we could do a list of lists-- ({key type: key}, subobject) pairs.

When we created the initial API, we did disregard these kind of APIs because they force you to have an interface{} somewhere and we were trying to avoid that (though I maybe see a workaround now).

WRT benchmarks, I'm somewhat surprised by these results. The first 3 benchmarks, which are supposed to be the closest to real-life scenarios are now showing more allocations. The 2-3% difference in CPU time are probably irrelevant. And on the other hand, FieldToSet is dropping almost 50% again (same as last PR)... I don't know what to think.

apelisse · 2019-07-23T08:27:31Z

I think I read all of it, so feel free to squash.

lavalamp · 2019-07-23T16:48:39Z

WRT benchmarks, I'm somewhat surprised by these results. The first 3 benchmarks, which are supposed to be the closest to real-life scenarios are now showing more allocations. The 2-3% difference in CPU time are probably irrelevant. And on the other hand, FieldToSet is dropping almost 50% again (same as last PR)... I don't know what to think.

To make it weirder, when I hook up everything (using a RawExtension), the opposite happens-- 10-15% improvements except for fieldtoset which gets worse... maybe I compared the wrong things.

Anyway, this is squashed and ready to merge. We can improve it more later, it should be in the right shape at least.

lavalamp · 2019-07-24T04:13:15Z

Pushed missing boilerplate and another optimization.

benchmark                                 old ns/op     new ns/op     delta
BenchmarkFieldSet/serialize-20-12         63898         27360         -57.18%
BenchmarkFieldSet/deserialize-20-12       112561        35718         -68.27%
BenchmarkFieldSet/serialize-50-12         143026        57494         -59.80%
BenchmarkFieldSet/deserialize-50-12       251731        86494         -65.64%
BenchmarkFieldSet/serialize-100-12        408637        170144        -58.36%
BenchmarkFieldSet/deserialize-100-12      718344        223283        -68.92%
BenchmarkFieldSet/serialize-500-12        2086484       919350        -55.94%
BenchmarkFieldSet/deserialize-500-12      3413733       1084716       -68.22%
BenchmarkFieldSet/serialize-1000-12       4234109       1786525       -57.81%
BenchmarkFieldSet/deserialize-1000-12     6758817       1950813       -71.14%

benchmark                                 old allocs     new allocs     delta
BenchmarkFieldSet/serialize-20-12         188            110            -41.49%
BenchmarkFieldSet/deserialize-20-12       400            216            -46.00%
BenchmarkFieldSet/serialize-50-12         513            300            -41.52%
BenchmarkFieldSet/deserialize-50-12       1128           616            -45.39%
BenchmarkFieldSet/serialize-100-12        1740           1032           -40.69%
BenchmarkFieldSet/deserialize-100-12      3902           2167           -44.46%
BenchmarkFieldSet/serialize-500-12        9237           5701           -38.28%
BenchmarkFieldSet/deserialize-500-12      19521          10770          -44.83%
BenchmarkFieldSet/serialize-1000-12       20014          12365          -38.22%
BenchmarkFieldSet/deserialize-1000-12     42475          23523          -44.62%

lavalamp · 2019-07-24T04:17:14Z

And

benchmark                       old ns/op     new ns/op     delta
BenchmarkApplyNewObject-12      2230683       1918830       -13.98%
BenchmarkUpdateNewObject-12     2624417       2175278       -17.11%
BenchmarkRepeatedUpdate-12      1031653       774939        -24.88%
BenchmarkSetToFields-12         24778         8860          -64.24%
BenchmarkFieldsToSet-12         33876         10001         -70.48%
BenchmarkYAMLToTyped-12         68377         64342         -5.90%

benchmark                       old allocs     new allocs     delta
BenchmarkApplyNewObject-12      8632           6861           -20.52%
BenchmarkUpdateNewObject-12     11210          7947           -29.11%
BenchmarkRepeatedUpdate-12      3057           2332           -23.72%
BenchmarkSetToFields-12         124            42             -66.13%
BenchmarkFieldsToSet-12         147            82             -44.22%
BenchmarkYAMLToTyped-12         150            152            +1.33%

apelisse · 2019-07-24T19:40:45Z

fieldpath/serialize-pe.go

+	case peValueSepBytes[0]:
+		iter := readPool.BorrowIterator(b)
+		defer readPool.ReturnIterator(iter)
+		v, err := value.ReadJSONIter(iter)


Could you call FromJSONFast directly?

apelisse · 2019-07-24T19:40:52Z

fieldpath/serialize-pe.go

+		}
+		return PathElement{Value: &v}, nil
+	case peKeySepBytes[0]:
+		iter := readPool.BorrowIterator(b)


Same as above

apelisse · 2019-07-24T19:42:19Z

fieldpath/serialize-pe.go

+
+// DeserializePathElement parses a serialized path element
+func DeserializePathElement(s string) (PathElement, error) {
+	b := []byte(s)


I believe this is copying the full buffer, is it worth optimizing?

apelisse · 2019-07-24T19:49:54Z

/lgtm

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 21, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 21, 2019

k8s-ci-robot requested review from apelisse and jennybuckley July 21, 2019 21:54

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 21, 2019

lavalamp mentioned this pull request Jul 22, 2019

Add cached lazy unmarshalling for field sets kubernetes/kubernetes#80438

Closed

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 23, 2019

lavalamp force-pushed the optimize branch from 9cfa9b6 to 924a1ab Compare July 23, 2019 00:59

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 23, 2019

lavalamp changed the title ~~WIP: set serialization~~ set serialization Jul 23, 2019

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 23, 2019

lavalamp force-pushed the optimize branch 2 times, most recently from 132762f to 208bc23 Compare July 23, 2019 16:46

lavalamp mentioned this pull request Jul 23, 2019

WIP: custom serialization for field set kubernetes/kubernetes#80497

Closed

Serialize Set and Value.

978ad1e

lavalamp force-pushed the optimize branch from 208bc23 to a990ceb Compare July 24, 2019 04:05

reduce allocations for small maps

8f1be3a

Vendor dependencies.

b48d312

lavalamp force-pushed the optimize branch from a990ceb to b48d312 Compare July 24, 2019 04:15

apelisse reviewed Jul 24, 2019

View reviewed changes

k8s-ci-robot assigned apelisse Jul 24, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 24, 2019

k8s-ci-robot merged commit 0eb2853 into kubernetes-sigs:master Jul 24, 2019

jennybuckley mentioned this pull request Jul 29, 2019

Use raw bytes instead of nested map in metav1.Fields kubernetes/kubernetes#80730

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

set serialization #90

set serialization #90

lavalamp commented Jul 21, 2019 •

edited

Loading

k8s-ci-robot commented Jul 21, 2019

lavalamp commented Jul 22, 2019

lavalamp commented Jul 22, 2019

lavalamp commented Jul 23, 2019 •

edited

Loading

lavalamp commented Jul 23, 2019

lavalamp commented Jul 23, 2019

apelisse commented Jul 23, 2019

apelisse commented Jul 23, 2019

lavalamp commented Jul 23, 2019

lavalamp commented Jul 24, 2019

lavalamp commented Jul 24, 2019

apelisse Jul 24, 2019

apelisse Jul 24, 2019

apelisse Jul 24, 2019

apelisse commented Jul 24, 2019

set serialization #90

set serialization #90

Conversation

lavalamp commented Jul 21, 2019 • edited Loading

k8s-ci-robot commented Jul 21, 2019

lavalamp commented Jul 22, 2019

lavalamp commented Jul 22, 2019

lavalamp commented Jul 23, 2019 • edited Loading

lavalamp commented Jul 23, 2019

lavalamp commented Jul 23, 2019

apelisse commented Jul 23, 2019

apelisse commented Jul 23, 2019

lavalamp commented Jul 23, 2019

lavalamp commented Jul 24, 2019

lavalamp commented Jul 24, 2019

apelisse Jul 24, 2019

Choose a reason for hiding this comment

apelisse Jul 24, 2019

Choose a reason for hiding this comment

apelisse Jul 24, 2019

Choose a reason for hiding this comment

apelisse commented Jul 24, 2019

lavalamp commented Jul 21, 2019 •

edited

Loading

lavalamp commented Jul 23, 2019 •

edited

Loading