Presentation Model Garbage Collection #63

Merged
merged 15 commits into from May 18, 2016

Projects

None yet

4 participants

@hendrikebbers
Member

A first version of a garbage collection for presentation models.
The new package that contains the gc isn't referenced in the sources and this request is done for a public review of the gc.

This package contains a first garbage collection implementation for the Dolphin Platform model. As defined by @DolphinModel and @DolphinBean each Dolphin Platform model is a hierarchy of several Dolphin Platform beans. All the beans will automatically be synchronized between server and client. To do so the remoting layer holds a representation of each Dolphin Platform Bean.
When the server controller removes a bean out of the hierarchy the garbage collection should notice this and automatically remove the bean representation from the remoting layer. By doing so the bean will automatically be deleted on the client.
How a bean can be created and removed is defined by the BeanManager. By using the garbage collection an application developer doesn't need to call BeanManager.remove(...) anymore.
The current version of the garbage collections don't allow cycles in references.

Some information how the current implementation is working:
The GarbageCollection class is the central point of this implementation. The class provides some methods that must called by the platform whenever a change in the beans has been done:

  • onBeanCreated(...) must be called for each new bean
  • onPropertyValueChanged(...) must be called whenever the internal value of a Property changes
  • onAddedToList(...)must be called whenever a new item was added to an ObservableList
  • onRemovedFromList(...)must be called whenever a item was removed from an ObservableList

This methods can simply be called be the BeanManager, the PropertyImpl and the ObservableArrayList. Whenever any of the methods is called the gc checks if the modified bean is still referenced by a root bean (the model of a MVC group). This is internally done by reference counting. If the bean isn't referenced anymore it will be added to a "toRemove" list. It will be removed from this list if it will be referenced again.

Whenever the gc() method of the garbage collection is called all the presentation models that are in the "toRemove" list will be removed. The plan is to call this method at the end of each server cycle. This means that whenever a command was handled on the server the server will remove unreferenced beans from the presentation model.

Review on Reviewable

hendrikebbers added some commits Jan 20, 2016
@hendrikebbers hendrikebbers Basic GC fd4003e
@hendrikebbers hendrikebbers field caching 17ee442
@hendrikebbers hendrikebbers Check Reference to root bean ca15025
@hendrikebbers hendrikebbers additional unit test 27aaee8
@hendrikebbers hendrikebbers additional tests 233d479
@hendrikebbers hendrikebbers JavaDoc for GC
0b2dc45
@hendrikebbers hendrikebbers added this to the 0.9.0 milestone Jan 26, 2016
@aalmiray
Contributor

Reviewed 19 of 19 files at r1.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


java-client-javafx/src/test/java/com/canoo/dolphin/client/javafx/FXBinderTest.java, line 3 [r1] (raw file):
why is this import needed in this code change?


java-core/src/main/java/com/canoo/dolphin/impl/IdentitySet.java, line 1 [r1] (raw file):
file header. sigh #108


java-server/src/main/java/com/canoo/dolphin/server/impl/gc/GarbageCollection.java, line 33 [r1] (raw file):
s/MVG/MVC/


java-server/src/main/java/com/canoo/dolphin/server/impl/gc/GarbageCollection.java, line 50 [r1] (raw file):
s/onRemove/onRemoveCallback/ or some other name. calling onRemove.onRemove(arg) looks weird


java-server/src/main/java/com/canoo/dolphin/server/impl/gc/GarbageCollection.java, line 76 [r1] (raw file):
use a more specific exception


Comments from the review on Reviewable.io

@hendrikebbers
Member

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


java-client-javafx/src/test/java/com/canoo/dolphin/client/javafx/FXBinderTest.java, line 3 [r1] (raw file):
I moved the MockedProperty class since it's now used on server and client


java-core/src/main/java/com/canoo/dolphin/impl/IdentitySet.java, line 1 [r1] (raw file):
Done, and yes I know :( Hope to find some time for this general infrastructure issues in future. But based on the projects that depend on DP other issues currently have a bigger importance.


java-server/src/main/java/com/canoo/dolphin/server/impl/gc/GarbageCollection.java, line 33 [r1] (raw file):
done


java-server/src/main/java/com/canoo/dolphin/server/impl/gc/GarbageCollection.java, line 50 [r1] (raw file):
done


java-server/src/main/java/com/canoo/dolphin/server/impl/gc/GarbageCollection.java, line 76 [r1] (raw file):
done


Comments from the review on Reviewable.io

@hendrikebbers hendrikebbers Changes based on review
9f9d0c0
@hendrikebbers
Member

@netopyr maybe you find some time for a review. I would love to add this basic API in near future. Based on this we can discuss the general integration.

@moschkom

Are Listeners on DolphinProperties automatically unsubscribed when the Presentation Model is removed?

@hendrikebbers
Member

I need to check / think about this. Maybe there are some constellations that currently don't work.

@hendrikebbers
Member

Normally you don't need to unsubscribe the listeners since you don't use the property anymore. Based on this the property instance will be GC by the JVM and since the property should be the only place that has a reference to the listener it should be removed (by GC), too.

@hendrikebbers hendrikebbers modified the milestone: 0.8.5, 0.9.0 May 2, 2016
@hendrikebbers hendrikebbers Merge branch 'master' into pm_gc
# Conflicts:
#	java-client-javafx/src/test/java/com/canoo/dolphin/client/javafx/MockedProperty.java
#	java-core/src/main/java/com/canoo/dolphin/impl/MockedProperty.java
#	platform/dolphin-platform-client-javafx/src/test/java/com/canoo/dolphin/client/javafx/FXBinderTest.java
#	platform/dolphin-platform-core/src/main/java/com/canoo/dolphin/impl/MockedProperty.java
#	platform/dolphin-platform-server/src/main/java/com/canoo/dolphin/server/context/DolphinContext.java
312a156
@coveralls

Coverage Status

Coverage increased (+4.1%) to 48.395% when pulling 312a156 on pm_gc into 46a211c on master.

@hendrikebbers hendrikebbers documentation
408d4fb
@hendrikebbers
Member

I updated this branch based on the new structure (merged master) and already added the GC to DP. I added a feature flag that defines if GC should be used. By default GC is not used but it can be simply activated in the dolphin.properties file. I think we should review and merge this branch for the next release. In that case we can test the GC internally.

@coveralls

Coverage Status

Coverage increased (+4.1%) to 48.395% when pulling 408d4fb on pm_gc into 46a211c on master.

@hendrikebbers hendrikebbers license headers
6f4253d
@coveralls

Coverage Status

Coverage increased (+4.1%) to 48.395% when pulling 6f4253d on pm_gc into 46a211c on master.

@aalmiray
Contributor

Reviewed 40 of 65 files at r3, 2 of 3 files at r4, 20 of 20 files at r5.
Review status: all files reviewed at latest revision, 11 unresolved discussions.


platform/dolphin-platform-client/src/main/java/com/canoo/dolphin/client/impl/ClientBeanBuilderImpl.java, line 53 [r5] (raw file):

    protected Property create(final Attribute attribute, final PropertyInfo propertyInfo) {
        return new PropertyImpl<>(attribute, propertyInfo);

would it make sense to obtain this instance using a factory?


platform/dolphin-platform-core/src/main/java/com/canoo/dolphin/impl/PlatformConstants.java, line 38 [r5] (raw file):

    String DESTROY_CONTEXT_COMMAND_NAME = DOLPHIN_PLATFORM_PREFIX + "disconnectClientContext";

    String GARBAGE_COLLECTION_COMMAND_NAME = DOLPHIN_PLATFORM_PREFIX + "garbage-collection";

this name has a hyphen - whereas the others use camel case.


platform/dolphin-platform-core/src/main/java/com/canoo/dolphin/impl/PropertyImpl.java, line 106 [r5] (raw file):

    }

    protected void notifyInternalListeners(ValueChangeEvent<T> event) {

is this method needed at all?


platform/dolphin-platform-server/src/main/java/com/canoo/dolphin/server/context/DolphinContext.java, line 99 [r5] (raw file):

    private final DolphinSession dolphinSession;

    private final GarbageCollection garbageCollection;

perhaps it's me but I find the name of this class a bit strange. The act/operation is "garbage collection" performed by a "garbage collector"


platform/dolphin-platform-server/src/main/java/com/canoo/dolphin/server/impl/ServerBeanBuilder.java, line 25 [r5] (raw file):

public interface ServerBeanBuilder extends BeanBuilder {

   <T> T createRootModel(Class<T> beanClass);

missing javadoc


platform/dolphin-platform-server/src/main/java/com/canoo/dolphin/server/impl/ServerBeanRepository.java, line 25 [r5] (raw file):

public interface ServerBeanRepository extends BeanRepository {

    <T> void deleteByGC(T bean);

missing javadoc. needs a better name perhaps


platform/dolphin-platform-server/src/main/java/com/canoo/dolphin/server/impl/gc/GarbageCollection.java, line 64 [r5] (raw file):

    public GarbageCollection(GarbageCollectionCallback onRemoveCallback) {
        this.onRemoveCallback = Assert.requireNonNull(onRemoveCallback, "onRemoveCallback");
        removeOnGC = new IdentityHashMap<>();

assignment could be made at the same time as field definition


platform/dolphin-platform-server/src/main/java/com/canoo/dolphin/server/mbean/beans/DolphinSessionInfo.java, line 41 [r5] (raw file):

        DolphinSession session = dolphinSessionRef.get();
        if(session == null) {
            throw new RuntimeException("Session == null");

throw NPE, ISE, IAE, or a DP exception instead. Or use Assert.requireNonNulltoo


platform/dolphin-platform-server/src/main/java/com/canoo/dolphin/server/mbean/beans/DolphinSessionInfo.java, line 49 [r5] (raw file):

        GarbageCollection garbageCollection = garbageCollectionRef.get();
        if(garbageCollection == null) {
            throw new RuntimeException("GarbageCollection == null");

throw NPE, ISE, IAE, or a DP exception instead. Or use Assert.requireNonNulltoo


platform/dolphin-platform-server/src/test/java/com/canoo/dolphin/server/impl/gc/TestGarbageCollection.java, line 54 [r5] (raw file):

        assertEquals(garbageCollection.getManagedInstancesCount(), 1);
        garbageCollection.gc();
        assertThat(removedObjects, Matchers.hasSize(0));

the code reads better if you use static imports in the case of assertions such as this one


platform/dolphin-platform-server/src/test/java/com/canoo/dolphin/server/impl/gc/TestGarbageCollection.java, line 849 [r5] (raw file):

    private GarbageCollection createGarbageCollection(GarbageCollectionCallback gcConsumer) {
        UnstableFeatureFlags.setUseGc(true);
        return new GarbageCollection(gcConsumer);

does the test turns this feature off at some point?


Comments from Reviewable

@hendrikebbers
Member

Review status: all files reviewed at latest revision, 11 unresolved discussions.


platform/dolphin-platform-core/src/main/java/com/canoo/dolphin/impl/PlatformConstants.java, line 38 [r5] (raw file):

Previously, aalmiray (Andres Almiray) wrote…

this name has a hyphen - whereas the others use camel case.


done


platform/dolphin-platform-core/src/main/java/com/canoo/dolphin/impl/PropertyImpl.java, line 106 [r5] (raw file):

Previously, aalmiray (Andres Almiray) wrote…

is this method needed at all?


yes, it's overwritten when a new property gets created. Same mechanism is already used by the observable lists


platform/dolphin-platform-server/src/main/java/com/canoo/dolphin/server/context/DolphinContext.java, line 99 [r5] (raw file):

Previously, aalmiray (Andres Almiray) wrote…

perhaps it's me but I find the name of this class a bit strange. The act/operation is "garbage collection" performed by a "garbage collector"


done


platform/dolphin-platform-server/src/main/java/com/canoo/dolphin/server/impl/ServerBeanBuilder.java, line 25 [r5] (raw file):

Previously, aalmiray (Andres Almiray) wrote…

missing javadoc


done


platform/dolphin-platform-server/src/main/java/com/canoo/dolphin/server/impl/ServerBeanRepository.java, line 25 [r5] (raw file):

Previously, aalmiray (Andres Almiray) wrote…

missing javadoc. needs a better name perhaps


done


platform/dolphin-platform-server/src/main/java/com/canoo/dolphin/server/mbean/beans/DolphinSessionInfo.java, line 41 [r5] (raw file):

Previously, aalmiray (Andres Almiray) wrote…

throw NPE, ISE, IAE, or a DP exception instead. Or use Assert.requireNonNulltoo


done


platform/dolphin-platform-server/src/main/java/com/canoo/dolphin/server/mbean/beans/DolphinSessionInfo.java, line 49 [r5] (raw file):

Previously, aalmiray (Andres Almiray) wrote…

throw NPE, ISE, IAE, or a DP exception instead. Or use Assert.requireNonNulltoo


done


Comments from Reviewable

@hendrikebbers hendrikebbers based on review
3667e3c
@hendrikebbers
Member

Review status: 27 of 45 files reviewed at latest revision, 11 unresolved discussions.


platform/dolphin-platform-client/src/main/java/com/canoo/dolphin/client/impl/ClientBeanBuilderImpl.java, line 53 [r5] (raw file):

Previously, aalmiray (Andres Almiray) wrote…

would it make sense to obtain this instance using a factory?


I don't think so. This class defines does nothing than create the concrete property / list instances. Currently there is no other way than creating this objects. It would make sense if there is another remoting lib next to OD.


platform/dolphin-platform-server/src/main/java/com/canoo/dolphin/server/impl/gc/GarbageCollector.java, line 64 [r5] (raw file):

Previously, aalmiray (Andres Almiray) wrote…

assignment could be made at the same time as field definition


done


Comments from Reviewable

@hendrikebbers hendrikebbers Changes based on review
a3eda33
@coveralls

Coverage Status

Coverage increased (+21.4%) to 65.64% when pulling a3eda33 on pm_gc into 46a211c on master.

@hendrikebbers hendrikebbers based on review
61395b2
@hendrikebbers
Member

Review status: 27 of 45 files reviewed at latest revision, 11 unresolved discussions.


platform/dolphin-platform-server/src/test/java/com/canoo/dolphin/server/impl/gc/TestGarbageCollection.java, line 54 [r5] (raw file):

Previously, aalmiray (Andres Almiray) wrote…

the code reads better if you use static imports in the case of assertions such as this one


done


platform/dolphin-platform-server/src/test/java/com/canoo/dolphin/server/impl/gc/TestGarbageCollection.java, line 849 [r5] (raw file):

Previously, aalmiray (Andres Almiray) wrote…

does the test turns this feature off at some point?


done


Comments from Reviewable

@hendrikebbers
Member

refactored the branch based on @aalmiray review. I think the request is ready to merge. Don't know if we should do this before or after the OD merge? @aalmiray ?

@coveralls

Coverage Status

Coverage increased (+21.3%) to 65.534% when pulling 61395b2 on pm_gc into 46a211c on master.

@aalmiray
Contributor

it should be merged after the OD PR is taken in

@hendrikebbers hendrikebbers Merge branch 'master' into pm_gc
176b467
@coveralls

Coverage Status

Coverage increased (+1.1%) to 65.256% when pulling 176b467 on pm_gc into d2503bd on master.

@hendrikebbers hendrikebbers GC logging
344aca0
@coveralls

Coverage Status

Coverage increased (+1.2%) to 65.307% when pulling 344aca0 on pm_gc into d2503bd on master.

@hendrikebbers hendrikebbers merged commit bd8d1a8 into master May 18, 2016

3 of 4 checks passed

code-review/reviewable 28 files, 11 discussions left (aalmiray)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+1.2%) to 65.307%
Details
@aalmiray aalmiray removed the in progress label May 18, 2016
@hendrikebbers hendrikebbers deleted the pm_gc branch May 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment