You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by GitBox <gi...@apache.org> on 2021/05/01 07:21:16 UTC
[GitHub] [avro] unchuckable opened a new pull request #1206: AVRO-3048: Use SpecificRecord's MODEL$ in builder creation
unchuckable opened a new pull request #1206:
URL: https://github.com/apache/avro/pull/1206
This PR addresses AVRO-3048 (https://issues.apache.org/jira/projects/AVRO/issues/AVRO-3048), improving `newBuilder()` performance for newly generated `SpecificRecord`s by reusing the already existing `SpecificData` instance (`MODEL$`) in the specific records themselves.
No new unit tests should be required, for the current changes are already covered by existing unit tests. Reference files for code generation have been updated to reflect the minor changes to the record and errror templates.
Further measures might be required to alleviate the performance regression of AVRO-3048 for existing code that cannot be re-generated easily.
If this PR is found feasible, I would suggest cherry-picking it for 1.9 and 1.10 branches, too, to allow for existing applications to benefit from the changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] Fokko commented on a change in pull request #1206: AVRO-3048: Use SpecificRecord's MODEL$ in builder creation
Posted by GitBox <gi...@apache.org>.
Fokko commented on a change in pull request #1206:
URL: https://github.com/apache/avro/pull/1206#discussion_r625371128
##########
File path: lang/java/compiler/src/main/velocity/org/apache/avro/compiler/specific/templates/java/classic/record.vm
##########
@@ -338,7 +338,7 @@ static {
*/
private Builder(#if ($schema.getNamespace())$this.mangle($schema.getNamespace()).#end${this.mangle($schema.getName())} other) {
#if ($schema.isError()) super(other)#else
- super(SCHEMA$)#end;
+ super(SCHEMA$,MODEL$)#end;
Review comment:
Thanks @RyanSkraba for the review
```suggestion
super(SCHEMA$, MODEL$)#end;
```
@unchuckable Don't forget this one :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] Fokko commented on a change in pull request #1206: AVRO-3048: Use SpecificRecord's MODEL$ in builder creation
Posted by GitBox <gi...@apache.org>.
Fokko commented on a change in pull request #1206:
URL: https://github.com/apache/avro/pull/1206#discussion_r625371128
##########
File path: lang/java/compiler/src/main/velocity/org/apache/avro/compiler/specific/templates/java/classic/record.vm
##########
@@ -338,7 +338,7 @@ static {
*/
private Builder(#if ($schema.getNamespace())$this.mangle($schema.getNamespace()).#end${this.mangle($schema.getName())} other) {
#if ($schema.isError()) super(other)#else
- super(SCHEMA$)#end;
+ super(SCHEMA$,MODEL$)#end;
Review comment:
Thanks @RyanSkraba for the review
```suggestion
super(SCHEMA$, MODEL$)#end;
```
@unchuckable Don't forget this one :) and more below!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] RyanSkraba commented on pull request #1206: AVRO-3048: Use SpecificRecord's MODEL$ in builder creation
Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on pull request #1206:
URL: https://github.com/apache/avro/pull/1206#issuecomment-831274101
Even if this looks fine for addressing the original issue specifically raised in the JIRA, I asked another question there based on one of your comments. Can you take a look before we merge this?
> A quick question though, I can't see how your PR reduces any calls to newInstance(...) (which is also an expensive call). Am I mistaken?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] unchuckable commented on a change in pull request #1206: AVRO-3048: Use SpecificRecord's MODEL$ in builder creation
Posted by GitBox <gi...@apache.org>.
unchuckable commented on a change in pull request #1206:
URL: https://github.com/apache/avro/pull/1206#discussion_r625373620
##########
File path: lang/java/compiler/src/main/velocity/org/apache/avro/compiler/specific/templates/java/classic/record.vm
##########
@@ -338,7 +338,7 @@ static {
*/
private Builder(#if ($schema.getNamespace())$this.mangle($schema.getNamespace()).#end${this.mangle($schema.getName())} other) {
#if ($schema.isError()) super(other)#else
- super(SCHEMA$)#end;
+ super(SCHEMA$,MODEL$)#end;
Review comment:
Dang. Hereby I propose a feature: run google formatter over generated code 😉
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] RyanSkraba merged pull request #1206: AVRO-3048: Use SpecificRecord's MODEL$ in builder creation
Posted by GitBox <gi...@apache.org>.
RyanSkraba merged pull request #1206:
URL: https://github.com/apache/avro/pull/1206
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] unchuckable commented on a change in pull request #1206: AVRO-3048: Use SpecificRecord's MODEL$ in builder creation
Posted by GitBox <gi...@apache.org>.
unchuckable commented on a change in pull request #1206:
URL: https://github.com/apache/avro/pull/1206#discussion_r625188547
##########
File path: lang/java/compiler/src/main/velocity/org/apache/avro/compiler/specific/templates/java/classic/record.vm
##########
@@ -310,7 +310,7 @@ static {
/** Creates a new Builder */
private Builder() {
- super(SCHEMA$);
+ super(SCHEMA$,MODEL$);
Review comment:
Not nit-picky at all, but just shows that you're doing code reviews the way they're supposed to be done. I entirely missed that one (or rather, spotless:apply didn't catch it. Will fix that :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] unchuckable commented on pull request #1206: AVRO-3048: Use SpecificRecord's MODEL$ in builder creation
Posted by GitBox <gi...@apache.org>.
unchuckable commented on pull request #1206:
URL: https://github.com/apache/avro/pull/1206#issuecomment-831546041
>
>
> Even if this looks fine for addressing the original issue specifically raised in the JIRA, I asked another question there based on one of your comments. Can you take a look before we merge this?
>
> > A quick question though, I can't see how your PR reduces any calls to newInstance(...) (which is also an expensive call). Am I mistaken?
Good point, addressed this on JIRA.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org