| Re: [epsilon-dev] IPropertyGetters/Setters in Models |
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 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 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 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 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 |