Skip to main content
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [epsilon-dev] IPropertyGetters/Setters in Models 
Hi Sina,

> I’m wondering whether it would make sense to derive this from context.getExecutorFactory().getActiveModuleElement()?

Sounds like a good idea. If that doesn't work for whatever reason, in
the two places we call property setters (AssignmentSatement and
TypeInitialiser) we could catch any exceptions produced by property
setters, set their ASTs, and rethrow them.

Cheers,
Dimitris

On Wed, 29 Apr 2020 at 16:58, Sina Madani <sinadoom@xxxxxxxxxxxxxx> wrote:
>
> Hi Alfonso,
>
>
>
> Thank you for your thoughts. Yes you’re right, the API should be clearer. Infact I think the methods shouldn’t be deprecated: either clearly documented or removed. My experience when doing the refactoring is that in the vast majority of cases, calling invoke without the ModuleElement and IEolContext is the “correct” usage (i.e. ModuleElement and IEolContext are both null parameters). The only time when the full method (i.e. with the ModuleElement and IEolContext parameters) is useful from the caller’s perspective is from EOL.
>
>
>
> I’m going to undeprecate the methods and emphasize that implementers should override the “full” method, leaving the default implementation to invoke that one with (null, null) additional parameters, as well as adding the caveat that the one with the context parameter should be preferred where possible.
>
>
>
> That said, the only reason we have the ModuleElement parameter (as far as I can tell) is for populating the stack trace when throwing exceptions. I’m wondering whether it would make sense to derive this from context.getExecutorFactory().getActiveModuleElement()? That way we could get rid of it and simplify the API.
>
>
>
> Thanks,
>
> Sina
>
>
>
> From: Alfonso de la Vega
> Sent: 29 April 2020 16:40
> To: Epislon Project developer discussions
> Subject: Re: [epsilon-dev] IPropertyGetters/Setters in Models
>
>
>
> Hello SIna,
>
>
>
> Thank you for that big refactoring work.
>
>
>
> In my case, as Pinset uses the previous IPropertyGetter implementation, I encounter the following: an error when setting the context of a getter object, and deprecated warnings when I call to the invoke method without the context as an extra parameter.
>
>
>
> I think the current documentation of the now deprecated methods (such as invoke(object,property)) might be wrongly interpreted as if such deprecated methods still work, although obviously they won't if a context is required. So, maybe adding to that documentation an extra statement between the existing two saying something like "This method will not work properly if an IEolContext is required" might make it clearer, if not already evident.
>
>
>
> Cheers,
>
> Alfonso
>
>
>
>
>
> 29 Apr 2020, 15:09 by sinadoom@xxxxxxxxxxxxxx:
>
> Hi everyone,
>
>
>
> Dimitris and I have discussed this and concluded that a stateless design is a sensible way forward, since the former design was from the old homogenous DOM (AST) days. I’ve refactored IPropertyGetter and IPropertySetter to be stateless (as well as IReflectivePropertySetter), which was quite a big task as it breaks backwards compatibility, but I’ve updated all known implementations. Since we’ll be releasing a major version, now is probably the best time to make such a change.
>
> Please let me know if you have any suggestions for improving this or preserving backwards compatibility.
>
>
>
> Thanks,
>
> Sina
>
>
>
> From: Sina Madani
>
> Sent: 27 April 2020 20:43
>
> To: Sina Madani
>
> Subject: RE: RE: [epsilon-dev] IPropertyGetters/Setters in Models
>
>
>
> Hi again,
>
>
>
> I propose that we rethink this design (or at least refactor it) slightly in a backwards-compatible way. I’ve noticed that there is inconsistent usage of IPropertyGetters/Setters across models, even though they all extend the Abstract base class. The main issue with IPGs is that they have a setAst method. This implies  only one property access may occur at any given point. Of course this isn’t enforced, but if an exception occurs then presumably only one of them will be reported. It appears that the ModuleElement field (the Ast) is only used for populating the exception. As for IPS, the issue there is the object value to be set is part of the object, not a method parameter. This means recycling is absolutely forbidden: at least under parallel execution.
>
>
>
> By contrast, the design for EOL’s AbstractOperation (which is the superclass of FirstOrderOperation) is stateless. No fields involved there. We should ideally make IModel’s IPG/IPS follow the same design so that we can re-use them in their respective IModels. This is probably possible in a backwards-compatible manner: we add default methods which take the object value to be set, ModuleElement and IEolContext as additional parameters to invoke and hasProperty. We can then rework the known implementations and usages accordingly.
>
>
>
> I’ll set up a branch to investigate this further. Please let me know if you have any thoughts on this. I should also point out that this isn’t purely a micro-optimisation, but for a more consistent and above all “correct” design (i.e. behaving as expected).
>
>
>
> Thanks,
>
> Sina
>
>
>
> From: Sina Madani
>
> Sent: 27 April 2020 18:20
>
> To: Epislon Project developer discussions
>
> Subject: RE: [epsilon-dev] IPropertyGetters/Setters in Models
>
>
>
> This turned out to be a classic case of premature optimisation. I assumed that because some models were caching this, it would be the right thing to do, and when I changed it all tests were passing. However I had missed the PropertySetter in EmfModel, and when I changed this two tests failed and I realised there is state involved. I’ll carefully look at this and revert the changes, and see if there is a better way.
>
>
>
> From: Sina Madani
>
> Sent: 27 April 2020 11:01
>
> To: Epislon Project developer discussions
>
> Subject: RE: Re: [epsilon-dev] IPropertyGetters/Setters in Models
>
>
>
> Hi Dimitris,
>
>
>
> Thank you for the clarification. It appears this was mainly an issue in JavaModel, PlainXmlModel, XmlModel and AbstractEmfModel. The others were all using a propertyGetter/propertySetter field, so I have moved this to the Model class and updated extensions accordingly so that the appropriate type is assigned in the constructor.
>
>
>
> Thanks,
>
> Sina
>
>
>
> From: Dimitris Kolovos
>
> Sent: 27 April 2020 08:30
>
> To: Epislon Project developer discussions
>
> Subject: Re: [epsilon-dev] IPropertyGetters/Setters in Models
>
>
>
> Hi Sina,
>
>
>
> This looks like an oversight. I can't see any reason why we shouldn't
>
> be reusing stateless property getters/setters.
>
>
>
> Cheers,
>
> Dimitris
>
>
>
> On Mon, 27 Apr 2020 at 00:20, Sina Madani <sinadoom@xxxxxxxxxxxxxx> wrote:
>
> >
>
> > Hi everyone,
>
> >
>
> >
>
> >
>
> > I noticed that the getPropertyGetter and getPropertySetter implementations in a lot of models (including PlainXml and Emf) return a new instance every time. Since property accesses are frequent (could be tens of millions of times per program), this results in a lot of unnecessary objects being created and GC’d. I was wondering if this is a bug in the implementation (perhaps a programming mistake), or part of the design? Since judging by the implementations of the getters/setters, there isn’t any state stored – these are effectively pure functions. If so I propose to cache all property getters and setters, which should in theory drastically reduce memory allocations. Intuitively it seems like a costly oversight in the design (in terms of performance) if this is the case, so please let me know if I’m missing something.
>
> >
>
> >
>
> >
>
> > Thanks,
>
> >
>
> > Sina
>
> >
>
> > _______________________________________________
>
> > epsilon-dev mailing list
>
> > epsilon-dev@xxxxxxxxxxx
>
> > To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/epsilon-dev
>
>
>
>
>
>
>
> --
>
> Dimitris Kolovos
>
> Professor of Software Engineering
>
> Department of Computer Science
>
> University of York
>
> http://www.cs.york.ac.uk/~dkolovos
>
>
>
> EMAIL DISCLAIMER http://www.york.ac.uk/docs/disclaimer/email.htm
>
> _______________________________________________
>
> epsilon-dev mailing list
>
> epsilon-dev@xxxxxxxxxxx
>
> To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/epsilon-dev
>
>
>
>
>
>
>
>
>
>
>
>
>
> _______________________________________________
> epsilon-dev mailing list
> epsilon-dev@xxxxxxxxxxx
> To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/epsilon-dev



-- 
Dimitris Kolovos
Professor of Software Engineering
Department of Computer Science
University of York
http://www.cs.york.ac.uk/~dkolovos

EMAIL DISCLAIMER http://www.york.ac.uk/docs/disclaimer/email.htm

Back to the top