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

feature: Detection of model change #1168

Closed
pvojtechovsky opened this issue Feb 4, 2017 · 10 comments
Closed

feature: Detection of model change #1168

pvojtechovsky opened this issue Feb 4, 2017 · 10 comments
Labels

Comments

@pvojtechovsky
Copy link
Collaborator

The model change detection is needed when:

  • caching some derived information. The cache should be invalidated when model element which is source for the information in cache changes.
  • model of higher abstraction level is based on spoon model. Here it needs to detect changes to be able to keep model up to date.

Requirements

R1 detects change of model element

For example if CtClass X changes name, then it should be possible to detect that on X

R2 detects change of any child model element

For example if an statement is added/removed/modified in body of an method then it should be possible to detect that change on all parents: CtBlock, CtMethod, CtClass, CtPackage, CtModel.

R3 detects change of model element referred by defined physical or computed relation

For example if method is added/removed/modified in CtClass A, then it should be possible to detect that on all classes, which directly or indirectly inherits/extends class A.

Solutions

Let's collect here the ideas how to solve that.

Solution 1 - Change events

We might fire event (call an method) on each change. The clients which needs to detect the change would listen for these events (using registered listener).

Features

The clients is informed immediately when node is changed

Problems

it can become performance problem, when it is used heavily and each change would fire many events ...
it can need a lot of memory to keep collections of listeners

Solution 2 - CtElement#modelVersion attribute

We might have this code

class CtElement {
   long modelVersion = 0;
  //returns true if this model or any child element was changed after time represented by modelVersionSnapshot
   boolean wasChangedAfter(long modelVersionSnapshot) {
      return modelVersionSnapshot<modelVersion;
  }
  public long getModelVersion() {
     return modelVersion;
  }
}
class CtModel {
  long version = 0;
  public long incrementVersion() {
    version++
    return version
  }
/*
* increments version of model and applies that version to modified element(s) and all parents
* @param e - the element whose content was changed
*/
  public long onElementChanged(CtElement... e) {
     long version = incrementVersion();
     e.modelVersion=version;
     e.forEachParent(parent->parent.modelVersion = version);
     return version;
  }
  public long getModelVersionSnapshot() {
     return version;
  }
}

Features

The clients have to ask whether element was changed. The client is not informed immediately.

We have one model version counter.

  • It allows to apply new version to model elements using complex algorithms, which visits not only parents of changed element, but also related elements. Thanks to one version number, it is easy to detect that visiting algorithm already applied new version to the element and we can easily avoid infinite loops.
  • It allows clients to store only one number - as snapshot - and detect if model was changed using that snapshot.
  • It can be implemented thread safe - when another thread is modifying the model too, then higher version number is kept.

Problems

Each model element has one long attribute.

@msteinbeck
Copy link
Contributor

In order to change the model, a client must actively add, remove, and modify nodes. Consequently, clients should always know about changes and should be able to handle them accordingly, shouldn't they?

@pvojtechovsky
Copy link
Collaborator Author

I agree, therefore I think that CtModel#onElementChanged will be probably called by client (or by spoon refactoring utilities, which we will have in future) and not by spoon model itself.
But then client might welcome a change detection support on spoon model side, which will help to clear appropriate caches.
I do not know yet, whether spoon really needs this change detection feature, but I came to this idea when I was solving #1164. In this PR whenever the reference to Lambda is needed, the spoon has to search all super classes/interfaces (there is usually none) and search for all methods (there is usually 1) ... (so no big problem) ... but may be that result might be cached - if it would be nearly free and automatically working behind the scene). There are many other things, which might be cached in spoon model. But all these caches must be able to detect that they should invalidate itself. So let's discuss this issue and see the result.

@monperrus
Copy link
Collaborator

I like this feature proposal. I have a slight preference for solution 2 which seems simpler than maintaining an Observer pattern. I'd say that the cost of one long per AST node is only a little overhead compared to the memory footprint of the whole AST, but to be measured.

The tests of this feature will be quite easy by piggybacking on IntercessionTest, ParentTest, ReplacementTest and the like (the parametrized tests).

@tdurieux
Copy link
Collaborator

tdurieux commented Feb 11, 2017

several month ago @GerardPaligot implements a stack AST change (in this branch): https://github.com/GerardPaligot/spoon/tree/feat_stack
It is not exactly what you want but it is close.

the model of the stack
photo_20161118_152007

@pvojtechovsky
Copy link
Collaborator Author

Thanks for that link! It is very interesting idea. Yes, it is not what I want, but it shows challenges of this issue. The nearly each method of spoon model has to be changed to implement such feature. And it is visible that it means extra effort to maintain or to understand such enhanced model.
The solutions might be (just fast ideas):

  • to detect calls of model setters using AspectJ
  • to generate Spoon model interfaces and classes from an metamodel
  • to have spoon model immutable and to provide builders to create new immutable model for each possible change. The new immutable model might share most of the model elements from the origin model.

@msteinbeck
Copy link
Contributor

AspectJ is a quite complex library. Do you thing it is worth adding it to the Spoon eco system?

@pvojtechovsky
Copy link
Collaborator Author

I am trying what spoon comunity likes. ;-) I have an experience with AspectJ and I know, that it would be an solution. But in case of spoon, I preffer spoon based solution. Something like

  1. define spoon metamodel using java interfaces, classes and annotations
  2. parse these sources using spoon and make spoon model of spoon metamodel
  3. (optionally) create java instance of SpoonMetaModel and fill it from spoon model of spoon metamodel. SpoonMetaModel is set of new java classes, which expresses the spoon metamodel.
  4. generate implementations of spoon model classes (e.g. CtTypeImpl), including spread change detection code, from spoon metamodel using spoon templates.

I know that you are already doing that (but without step 3)... so it is probably nothing new for you.

@tdurieux
Copy link
Collaborator

Is #1417 enough for your need?

@pvojtechovsky
Copy link
Collaborator Author

I have not checked your latest changes, but few days ago it was exactly what I need. I guess it will fit now too. And if not then it will be only minor changes needed, which can be done by another PR.

@tdurieux
Copy link
Collaborator

Ok, perfect.

In the last version we change the listener hierarchy.
Now we have an FineModelChangeListener and an ActionBasedChangeListenerImpl.
Where ActionBasedChangeListenerImpl extends FineModelChangeListener and add the action based listener. Which is easier to understand and to use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants