-
Notifications
You must be signed in to change notification settings - Fork 7
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
Avoid unnecessary copies in generated VisitorTransforms #75
Conversation
a63e33c
to
3c09dd9
Compare
Previously, the generated VisitorTransform classes *always* made a deep copy of the input tree. With this commit, only those nodes which are actually changed will be copied. In the transform*() functions of the VisitorTransform super classes, we now check to see if all of the new child nodes are the same instance as the original child nodes. If so, there is no need to copy the original node. Note that this does not apply to cross-domain visitor transforms since the type of the nodes is changing, it is still neccessary to perform the deep copy.
3c09dd9
to
8468d05
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only had a couple typos and a question about an old TODO. Rest LGTM.
I'm curious if there's any benchmarking we can use to compare performance with versus without this change.
open fun transformAnyElement(node: AnyElement): AnyElement { | ||
val newMetas = transformMetas(node.metas) | ||
return if(node.metas !== newMetas) { | ||
// TODO: remove .asAnyElement() below when https://github.com/amzn/ion-element-kotlin/issues/36 is fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a side note, is this TODO still relevant? The mentioned issue was closed via:
Actually, this is not possible since the narrower IonElement implementations already has a covariant return type which is narrowed.
@@ -133,6 +135,47 @@ class VisitorTransformSmokeTests { | |||
assertEquals(expectedAst, output) | |||
} | |||
|
|||
@Test | |||
fun doesNotMakeUneccessaryCopiesWithWithLongPrimitives() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: Uneccessary -> Unnecessary
} | ||
|
||
@Test | ||
fun doesNotMakeUneccessaryCopiesWithWithSymbolPrimitives() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: Uneccessary -> Unnecessary
I'm not sure why this didn't occur to me earlier, but previously, the generated
VisitorTransform
classes always made a deep copy of the input tree. With this commit, only those nodes which are actually changed will be copied.In the
transform*()
functions of theVisitorTransform
super classes, we now check to see if all of the new child nodes are the same instance as the original child nodes. If so, there is no need to copy the original node.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.