Skip to content
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

Merged
merged 3 commits into from
Jun 13, 2021

Conversation

dlurton
Copy link
Member

@dlurton dlurton commented May 30, 2021

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 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.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@dlurton dlurton force-pushed the avoid-unnecessary-copies branch 3 times, most recently from a63e33c to 3c09dd9 Compare May 30, 2021 20:28
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.
@dlurton dlurton force-pushed the avoid-unnecessary-copies branch from 3c09dd9 to 8468d05 Compare May 30, 2021 20:32
Copy link
Member

@alancai98 alancai98 left a 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.
Copy link
Member

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() {
Copy link
Member

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() {
Copy link
Member

Choose a reason for hiding this comment

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

typo: Uneccessary -> Unnecessary

@dlurton dlurton changed the title Avoid uneccessary copies in generated VisitorTransforms Avoid unnecessary copies in generated VisitorTransforms Jun 8, 2021
@dlurton dlurton merged commit 3e11da6 into master Jun 13, 2021
@dlurton dlurton deleted the avoid-unnecessary-copies branch June 13, 2021 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants