Reduce allocations during conversion, enable new UnsafeConvertToVersion path #25018
Conversation
smarterclayton
added
the
sig/scalability
|
@wojtek-t @kubernetes/sig-api-machinery this is a bunch of prep refactors to enable the big performance gains on encoding / decoding by reusing types that are identical |
k8s-github-robot
added
kind/new-api
size/L
smarterclayton
force-pushed
the
fast_conversion
branch
from
ba5e337
to
9cfbdb5
Compare
smarterclayton
added
release-note-none
k8s-github-robot
added
the
needs-rebase
smarterclayton
force-pushed
the
fast_conversion
branch
from
9cfbdb5
to
007c556
Compare
k8s-github-robot
removed
the
needs-rebase
smarterclayton
force-pushed
the
fast_conversion
branch
from
007c556
to
f04a177
Compare
k8s-github-robot
added
size/XL
smarterclayton
force-pushed
the
fast_conversion
branch
from
f04a177
to
ca669c5
Compare
|
@wojtek-t in case this dropped off your radar |
| @@ -69,7 +69,7 @@ func NewCodecForScheme( | |||
| encodeVersion []unversioned.GroupVersion, | |||
| decodeVersion []unversioned.GroupVersion, | |||
| ) runtime.Codec { | |||
| return NewCodec(encoder, decoder, scheme, scheme, scheme, runtime.ObjectTyperToTyper(scheme), encodeVersion, decodeVersion) | |||
| return NewCodec(encoder, decoder, runtime.UnsafeObjectConvertor(scheme), scheme, scheme, runtime.ObjectTyperToTyper(scheme), encodeVersion, decodeVersion) | |||
There was a problem hiding this comment.
So are we assuming this safe by default since during a conversion step every sane thing I can think of throws away the original or is there some reason I'm not seeing that makes this always safe?
There was a problem hiding this comment.
If you are using a versioning codec, conversion is always "create
intermediate and then throw it away". If you don't run conversion, then we
guarantee not to mutate your object, unless you give us an object that
mutates itself during serialization, in which case we hate you and you
deserve what you get.
On Fri, May 6, 2016 at 4:05 PM, David Eads notifications@github.com wrote:
In pkg/runtime/serializer/versioning/versioning.go
#25018 (comment)
:@@ -69,7 +69,7 @@ func NewCodecForScheme(
encodeVersion []unversioned.GroupVersion,
decodeVersion []unversioned.GroupVersion,
) runtime.Codec {
- return NewCodec(encoder, decoder, scheme, scheme, scheme, runtime.ObjectTyperToTyper(scheme), encodeVersion, decodeVersion)
- return NewCodec(encoder, decoder, runtime.UnsafeObjectConvertor(scheme), scheme, scheme, runtime.ObjectTyperToTyper(scheme), encodeVersion, decodeVersion)
So are we assuming this safe by default since during a conversion step
every sane thing I can think of throws away the original or is there some
reason I'm not seeing that makes this always safe?—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/25018/files/ca669c5de0eba299d027ecb1c9d32155ef058038#r62383689
|
@smarterclayton - sorry I completely missed this PR. I will take a look on Monday. |
| return nil, fmt.Errorf("%v is not a registered type and cannot be converted into version %q", t, outVersion) | ||
| } | ||
|
|
||
| // if the Go type is also registered to the destination kind, no conversion is necessary |
There was a problem hiding this comment.
hmm - I don't understand it. Can you please clarify what are you doing here?
There was a problem hiding this comment.
Do I understand correctly that it basically mean that exactly the same type is basically registered in few different group versions?
There was a problem hiding this comment.
|
Some minor comments - feel free to self-apply lgtm label after addressing/answering them. |
smarterclayton
force-pushed
the
fast_conversion
branch
from
ca669c5
to
c7725e2
Compare
|
Comment applied, rebased, reapplying LGTM |
smarterclayton
added
lgtm
|
Bumping to P2 since this blocks the new "fast conversion" work. |
smarterclayton
force-pushed
the
fast_conversion
branch
from
c7725e2
to
95edab4
Compare
k8s-github-robot
removed
the
lgtm
wojtek-t
added
the
lgtm
|
LGTM |
We will probably readd these as an opaque object passed down to conversions that lets the caller get access to more info (like a negotiated serializer).
We don't need to pass a pointer into SetGroupKindVersion() - a struct works just as well.
Long delayed refactor, avoids a few more allocations.
Only encode/decode will call this path, to allow us to optimize for unsafe operations.
smarterclayton
force-pushed
the
fast_conversion
branch
from
95edab4
to
ea7e7a1
Compare
k8s-github-robot
removed
the
lgtm
smarterclayton
added
the
lgtm
|
Trivial rebase |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e build/test passed for commit ea7e7a1. |
|
Automatic merge from submit-queue |
Cleans up the conversion path to avoid a few unnecessary allocations, then creates a new UnsafeConvertToVersion path that will allow encode/decode to bypass copying the object for performance. In that subsequent PR, ConvertToVersion will start to call Copy() and we will refactor conversions to reuse as much of the existing object as possible.
Also changes the unversioned.ObjectKind signature to not require allocations - speeds up a few common paths.