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

@ChainOverridable on Properties #45

Closed
tavoda opened this Issue Jul 23, 2013 · 15 comments

Comments

Projects
None yet
3 participants
@tavoda
Member

tavoda commented Jul 23, 2013

I'm working now on extending Properties.xtend, however annotation @ChainOverridable generate wrong code for 'boolean' return value on statement 'return null'. We have to handle somehow primitive types return value.

@ghost ghost assigned tavoda Jul 23, 2013

@tavoda

This comment has been minimized.

Show comment
Hide comment
@tavoda

tavoda Jul 23, 2013

Member

Ron, I was checking new annotation processor. Don't understand this 'return null' section. _chain_XXX should ALWAYS be a fallback.

Member

tavoda commented Jul 23, 2013

Ron, I was checking new annotation processor. Don't understand this 'return null' section. _chain_XXX should ALWAYS be a fallback.

@rcodesmith

This comment has been minimized.

Show comment
Hide comment
@rcodesmith

rcodesmith Jul 23, 2013

Member

That part of the next_* method was a branch that realistically we never should get to. The next_* methods are explicitly called by template code to delegate to the next object in the chain. In the case of that branch, there is no next object in the chain. The only way this can happen is if the code in the ChainOverrideable-annotated class calls the next_* method, which makes no sense because that class is always the last in the chain and it doesn't make sense for it to be trying to delegate to anything else.

I could see we could either try to return some default return value, or throw an unchecked exception.

I don't see a sensible default for the return types (e.g. if it's an int or boolean), and plus it doesn't make sense the method was getting called in this scenario anyways, so I implemented the latter approach, throwing the unchecked exception.

Member

rcodesmith commented Jul 23, 2013

That part of the next_* method was a branch that realistically we never should get to. The next_* methods are explicitly called by template code to delegate to the next object in the chain. In the case of that branch, there is no next object in the chain. The only way this can happen is if the code in the ChainOverrideable-annotated class calls the next_* method, which makes no sense because that class is always the last in the chain and it doesn't make sense for it to be trying to delegate to anything else.

I could see we could either try to return some default return value, or throw an unchecked exception.

I don't see a sensible default for the return types (e.g. if it's an int or boolean), and plus it doesn't make sense the method was getting called in this scenario anyways, so I implemented the latter approach, throwing the unchecked exception.

@tavoda

This comment has been minimized.

Show comment
Hide comment
@tavoda

tavoda Jul 24, 2013

Member

Can you please try to add ChainOverridable annotation on sculptor-generator/sculptor-generator-core/src/main/java/org/sculptor/generator/ext/Properties.xtend and compile?

I'm still getting errors. Less than before but still some.

Thanks

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.0:compile (default-compile) on project sculptor-generator-core: Compilation failure: Compilation failure:
[ERROR] /home/tavoda/Work/sculptor/sculptor/sculptor-generator/sculptor-generator-core/src/main/xtend-gen/org/sculptor/generator/ext/Properties.java:[5152,72] incompatible types
[ERROR] required: boolean
[ERROR] found: java.lang.Object
[ERROR] /home/tavoda/Work/sculptor/sculptor/sculptor-generator/sculptor-generator-core/src/main/xtend-gen/org/sculptor/generator/ext/Properties.java:[5169,56] incompatible types
[ERROR] required: boolean
[ERROR] found: java.lang.Object
[ERROR] /home/tavoda/Work/sculptor/sculptor/sculptor-generator/sculptor-generator-core/src/main/xtend-gen/org/sculptor/generator/ext/Properties.java:[5183,63] incompatible types
[ERROR] required: java.lang.String
[ERROR] found: java.lang.Object
[ERROR] /home/tavoda/Work/sculptor/sculptor/sculptor-generator/sculptor-generator-core/src/main/xtend-gen/org/sculptor/generator/ext/Properties.java:[5200,47] incompatible types
[ERROR] required: java.lang.String
[ERROR] found: java.lang.Object
[ERROR] /home/tavoda/Work/sculptor/sculptor/sculptor-generator/sculptor-generator-core/src/main/xtend-gen/org/sculptor/generator/ext/Properties.java:[5214,74] incompatible types
[ERROR] required: java.lang.String
[ERROR] found: java.lang.Object
[ERROR] /home/tavoda/Work/sculptor/sculptor/sculptor-generator/sculptor-generator-core/src/main/xtend-gen/org/sculptor/generator/ext/Properties.java:[5231,58] incompatible types
[ERROR] required: java.lang.String
[ERROR] found: java.lang.Object

Member

tavoda commented Jul 24, 2013

Can you please try to add ChainOverridable annotation on sculptor-generator/sculptor-generator-core/src/main/java/org/sculptor/generator/ext/Properties.xtend and compile?

I'm still getting errors. Less than before but still some.

Thanks

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.0:compile (default-compile) on project sculptor-generator-core: Compilation failure: Compilation failure:
[ERROR] /home/tavoda/Work/sculptor/sculptor/sculptor-generator/sculptor-generator-core/src/main/xtend-gen/org/sculptor/generator/ext/Properties.java:[5152,72] incompatible types
[ERROR] required: boolean
[ERROR] found: java.lang.Object
[ERROR] /home/tavoda/Work/sculptor/sculptor/sculptor-generator/sculptor-generator-core/src/main/xtend-gen/org/sculptor/generator/ext/Properties.java:[5169,56] incompatible types
[ERROR] required: boolean
[ERROR] found: java.lang.Object
[ERROR] /home/tavoda/Work/sculptor/sculptor/sculptor-generator/sculptor-generator-core/src/main/xtend-gen/org/sculptor/generator/ext/Properties.java:[5183,63] incompatible types
[ERROR] required: java.lang.String
[ERROR] found: java.lang.Object
[ERROR] /home/tavoda/Work/sculptor/sculptor/sculptor-generator/sculptor-generator-core/src/main/xtend-gen/org/sculptor/generator/ext/Properties.java:[5200,47] incompatible types
[ERROR] required: java.lang.String
[ERROR] found: java.lang.Object
[ERROR] /home/tavoda/Work/sculptor/sculptor/sculptor-generator/sculptor-generator-core/src/main/xtend-gen/org/sculptor/generator/ext/Properties.java:[5214,74] incompatible types
[ERROR] required: java.lang.String
[ERROR] found: java.lang.Object
[ERROR] /home/tavoda/Work/sculptor/sculptor/sculptor-generator/sculptor-generator-core/src/main/xtend-gen/org/sculptor/generator/ext/Properties.java:[5231,58] incompatible types
[ERROR] required: java.lang.String
[ERROR] found: java.lang.Object

@tavoda tavoda reopened this Jul 24, 2013

@rcodesmith

This comment has been minimized.

Show comment
Hide comment
@rcodesmith

rcodesmith Jul 25, 2013

Member

The problem is with dispatch methods. XTend's processing of dispatch methods is clashing with our overriding transformations.

XTend renames these methods itself and substitutes an outer method that does the multiple dispatch. The problem is the return type in our _chained* method ends up being Object and the outer method ends up being the real return type.

I'm not sure right now about how/if dispatch methods should be supported for overriding - think about scenarios where one of several dispatch methods gets overridden in several extensions. And if we wanted to skip processing of dispatch methods, right now I don't see a way to detect a method is a dispatch method using MutableMethodDeclaration.

Member

rcodesmith commented Jul 25, 2013

The problem is with dispatch methods. XTend's processing of dispatch methods is clashing with our overriding transformations.

XTend renames these methods itself and substitutes an outer method that does the multiple dispatch. The problem is the return type in our _chained* method ends up being Object and the outer method ends up being the real return type.

I'm not sure right now about how/if dispatch methods should be supported for overriding - think about scenarios where one of several dispatch methods gets overridden in several extensions. And if we wanted to skip processing of dispatch methods, right now I don't see a way to detect a method is a dispatch method using MutableMethodDeclaration.

@tavoda

This comment has been minimized.

Show comment
Hide comment
@tavoda

tavoda Jul 25, 2013

Member

Will it help if we will change return value instead of primitive type to
Object type like Boolean?
Is dispatching processed before or after active annotation?

Can be dispatching method detected like:

  1. process all methods and remember method name and all parameter types
  2. if you find more than one such method don't touch it

I will ask on XTend forum about possibility to add 'dispatch' flag on
MutableMethodDeclaration.

Thanks

Pavel

On Thu, Jul 25, 2013 at 2:37 AM, Ron Smith notifications@github.com wrote:

The problem is with dispatch methods. XTend's processing of dispatch
methods is clashing with our overriding transformations.

XTend renames these methods itself and substitutes an outer method that
does the multiple dispatch. The problem is the return type in our _chained*
method ends up being Object and the outer method ends up being the real
return type.

I'm not sure right now about how/if dispatch methods should be supported
for overriding - think about scenarios where one of several dispatch
methods gets overridden in several extensions. And if we wanted to skip
processing of dispatch methods, right now I don't see a way to detect a
method is a dispatch method using MutableMethodDeclaration.


Reply to this email directly or view it on GitHubhttps://github.com//issues/45#issuecomment-21526441
.

Member

tavoda commented Jul 25, 2013

Will it help if we will change return value instead of primitive type to
Object type like Boolean?
Is dispatching processed before or after active annotation?

Can be dispatching method detected like:

  1. process all methods and remember method name and all parameter types
  2. if you find more than one such method don't touch it

I will ask on XTend forum about possibility to add 'dispatch' flag on
MutableMethodDeclaration.

Thanks

Pavel

On Thu, Jul 25, 2013 at 2:37 AM, Ron Smith notifications@github.com wrote:

The problem is with dispatch methods. XTend's processing of dispatch
methods is clashing with our overriding transformations.

XTend renames these methods itself and substitutes an outer method that
does the multiple dispatch. The problem is the return type in our _chained*
method ends up being Object and the outer method ends up being the real
return type.

I'm not sure right now about how/if dispatch methods should be supported
for overriding - think about scenarios where one of several dispatch
methods gets overridden in several extensions. And if we wanted to skip
processing of dispatch methods, right now I don't see a way to detect a
method is a dispatch method using MutableMethodDeclaration.


Reply to this email directly or view it on GitHubhttps://github.com//issues/45#issuecomment-21526441
.

@tavoda

This comment has been minimized.

Show comment
Hide comment
@tavoda

tavoda Jul 25, 2013

Member

I did little bit of investigation about this problem. Now I can detect 'dispatched' methods. However I'm curious why do we need such complicated implementation of Chain?
What was problem with previous implementation? Do we need separate XxxMethodIndex class?
Later on we will have @ChainOverridable annotation nearly on all templates and extension.

Member

tavoda commented Jul 25, 2013

I did little bit of investigation about this problem. Now I can detect 'dispatched' methods. However I'm curious why do we need such complicated implementation of Chain?
What was problem with previous implementation? Do we need separate XxxMethodIndex class?
Later on we will have @ChainOverridable annotation nearly on all templates and extension.

@rcodesmith

This comment has been minimized.

Show comment
Hide comment
@rcodesmith

rcodesmith Jul 25, 2013

Member

The issue before was that if an override/extension class overrode one method, it wouldn't get called unless the class overrode every method in the class. To fix that, when any overrideable method gets called however deep in the chain (for instance the original template), it must dispatch to the head of the chain, in case the head of the chain or some earlier class overrode the method.

The added complexity of having two arrays of objects (head and next) and the *MethodIndex constants are to avoid chaining through empty methods for every class on the chain. Since we now have to dispatch to the head of the chain for every method call, it'll be many extra function calls to find the class that actually implements a method. Worse, if your template methods are iterating over elements, calling sub functions for each, having possibly multiple levels of iteration, if each of those calls has to traverse the chain, it can get expensive. Especially considering in many cases those methods will probably be empty and there to simply chain to the next.

What the two arrays of objects allow us to do is only dispatch to those classes that actually overrode a particular method. So if in a chain of 5 classes, only the original template overrode method b(), calling b() will dispatch directly to the original template override method rather than going through the other 4 classes first.

Member

rcodesmith commented Jul 25, 2013

The issue before was that if an override/extension class overrode one method, it wouldn't get called unless the class overrode every method in the class. To fix that, when any overrideable method gets called however deep in the chain (for instance the original template), it must dispatch to the head of the chain, in case the head of the chain or some earlier class overrode the method.

The added complexity of having two arrays of objects (head and next) and the *MethodIndex constants are to avoid chaining through empty methods for every class on the chain. Since we now have to dispatch to the head of the chain for every method call, it'll be many extra function calls to find the class that actually implements a method. Worse, if your template methods are iterating over elements, calling sub functions for each, having possibly multiple levels of iteration, if each of those calls has to traverse the chain, it can get expensive. Especially considering in many cases those methods will probably be empty and there to simply chain to the next.

What the two arrays of objects allow us to do is only dispatch to those classes that actually overrode a particular method. So if in a chain of 5 classes, only the original template overrode method b(), calling b() will dispatch directly to the original template override method rather than going through the other 4 classes first.

@tavoda

This comment has been minimized.

Show comment
Hide comment
@tavoda

tavoda Jul 26, 2013

Member

Original implementation was very simple. Every template had 'next' pointer and we created for injection full chain. Here wasn't necessary to find anything.
Normally it will be overridden by 1 user template not big chaining with 5 overrides. Cartridges overrides usually top RootTmpl which is called just once.
Do we have some measurement which speed up this bring, I think none. Calling empty method like 'next.doSomething()' even 5 times cost nothing.
If we will stick with this implementation anyway we have to solve problem with next_something(). This is against any object principle I know. Little bit over what my object soul can accept.
Yesterday with some fixes I was able to enhance Properties. I still have to look and test this implementation.

Member

tavoda commented Jul 26, 2013

Original implementation was very simple. Every template had 'next' pointer and we created for injection full chain. Here wasn't necessary to find anything.
Normally it will be overridden by 1 user template not big chaining with 5 overrides. Cartridges overrides usually top RootTmpl which is called just once.
Do we have some measurement which speed up this bring, I think none. Calling empty method like 'next.doSomething()' even 5 times cost nothing.
If we will stick with this implementation anyway we have to solve problem with next_something(). This is against any object principle I know. Little bit over what my object soul can accept.
Yesterday with some fixes I was able to enhance Properties. I still have to look and test this implementation.

@rcodesmith

This comment has been minimized.

Show comment
Hide comment
@rcodesmith

rcodesmith Jul 26, 2013

Member

I agree the user override class wouldn't be a problem since it was just one. What gave me some concern was the cartridges. Cartridges will probably override RootTmpl, but I think it won't be uncommon to override other templates also. The MongoDB cartridge will have to override RepositoryTmpl, DomainObjectReferenceTmpl, DomainObjectTmpl, etc since the specialized code is embedded into these classes.

We don't have performance measurements, but the number of extra method calls seemed excessive since we have to always go to the head of the chain now. e.g. if in evaluating a template there's 100 method calls in the template normally, 2 other classes in the chain besides the template (e.g. 1 cartridge & 1 override), you have up to 3 extra calls per invocation (1 to dispatch to head of chain, 2 classes in chain) so 300 extra method calls in addition to the 100 normal ones.

If you'd rather go with the object-level chaining rather than the per-method chaining, I can put that back and we can do some profiling later.

I'm not fond of the next_something() solution, but what we had before (calling super.something() or next.something() ) won't work because something() needs to dispatch to the head of the chain rather than the next object in the chain. That was the fundamental problem, regardless of whether we do object-level or method level chaining. I'll see if I can think of a solution that allows us to get back to a more OO-syntax. Any ideas?

Member

rcodesmith commented Jul 26, 2013

I agree the user override class wouldn't be a problem since it was just one. What gave me some concern was the cartridges. Cartridges will probably override RootTmpl, but I think it won't be uncommon to override other templates also. The MongoDB cartridge will have to override RepositoryTmpl, DomainObjectReferenceTmpl, DomainObjectTmpl, etc since the specialized code is embedded into these classes.

We don't have performance measurements, but the number of extra method calls seemed excessive since we have to always go to the head of the chain now. e.g. if in evaluating a template there's 100 method calls in the template normally, 2 other classes in the chain besides the template (e.g. 1 cartridge & 1 override), you have up to 3 extra calls per invocation (1 to dispatch to head of chain, 2 classes in chain) so 300 extra method calls in addition to the 100 normal ones.

If you'd rather go with the object-level chaining rather than the per-method chaining, I can put that back and we can do some profiling later.

I'm not fond of the next_something() solution, but what we had before (calling super.something() or next.something() ) won't work because something() needs to dispatch to the head of the chain rather than the next object in the chain. That was the fundamental problem, regardless of whether we do object-level or method level chaining. I'll see if I can think of a solution that allows us to get back to a more OO-syntax. Any ideas?

@rcodesmith

This comment has been minimized.

Show comment
Hide comment
@rcodesmith

rcodesmith Aug 9, 2013

Member

Pavel - you mentioned you found a way to detect dispatched methods. What was the technique? I'd like to at least temporarily skip them.

Member

rcodesmith commented Aug 9, 2013

Pavel - you mentioned you found a way to detect dispatched methods. What was the technique? I'd like to at least temporarily skip them.

@rcodesmith rcodesmith closed this Aug 9, 2013

@rcodesmith rcodesmith reopened this Aug 9, 2013

@tavoda

This comment has been minimized.

Show comment
Hide comment
@tavoda

tavoda Aug 12, 2013

Member

Hi Ron, I just returned from holidays. I will answer about Wednesday.

Member

tavoda commented Aug 12, 2013

Hi Ron, I just returned from holidays. I will answer about Wednesday.

@rcodesmith

This comment has been minimized.

Show comment
Hide comment
@rcodesmith

rcodesmith Aug 13, 2013

Member

Ok. I'm just about done with moving the next_* methods into a separate class so it'll be cleaner (call next method via nextObj->methodName(...)).

Member

rcodesmith commented Aug 13, 2013

Ok. I'm just about done with moving the next_* methods into a separate class so it'll be cleaner (call next method via nextObj->methodName(...)).

@tavoda

This comment has been minimized.

Show comment
Hide comment
@tavoda

tavoda Aug 22, 2013

Member

Dispatch methods are skipped now (fixed in be9f5d1). For now Properties are working. However we need mechanism how to override also dispatch methods.

Member

tavoda commented Aug 22, 2013

Dispatch methods are skipped now (fixed in be9f5d1). For now Properties are working. However we need mechanism how to override also dispatch methods.

@rcodesmith

This comment has been minimized.

Show comment
Hide comment
@rcodesmith

rcodesmith May 8, 2014

Member

There may be a solution for dispatch methods:
https://groups.google.com/forum/#!topic/xtend-lang/Id8S9bbRsOc

I'll try it out soon.

Member

rcodesmith commented May 8, 2014

There may be a solution for dispatch methods:
https://groups.google.com/forum/#!topic/xtend-lang/Id8S9bbRsOc

I'll try it out soon.

@rcodesmith rcodesmith assigned rcodesmith and unassigned tavoda May 15, 2014

rcodesmith added a commit that referenced this issue May 15, 2014

#45: Overriding of dispatch methods now working. This should take car…
…e of the Properties class, which was the original reason for this issue. Not sure if dispatch

create methods supported.  This solution depends on how XTend generates code for dispatch methods - the original methods have a '_' prepended to them.  I updated scu
lptor-shipping-generator with an example of a dispatch method being overridden.
@rcodesmith

This comment has been minimized.

Show comment
Hide comment
@rcodesmith

rcodesmith May 15, 2014

Member

This should be working now based on the last commit. I'll keep the issue open for now as there may be a bit of cleanup to do.

Member

rcodesmith commented May 15, 2014

This should be working now based on the last commit. I'll keep the issue open for now as there may be a bit of cleanup to do.

@tjuerge tjuerge added 3 - Done and removed 2 - Working labels May 26, 2014

@tjuerge tjuerge added this to the 3.0.4 milestone May 26, 2014

@tjuerge tjuerge closed this May 26, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment