chore: update asset scheme #109

pollend · 2021-04-26T00:25:30Z

this changes how assets work with the assetmanager and attempts to address some problems.

for transient assets the current scheme will slowly leak weak pointers since they are not properly removed from the multimap.
there is now just a single map from a resource urn to the start of a chain of assets. (non-transient assets use soft-reference and transient assets use a weakreference but weak references use a strong reference so the non-transient asset so it's not disposed unless all transient assets have been disposed in the chain.

the non-transient asset is always placed first and all transient assets use a weak reference
each asset is chained from the beginning forward as new assets are added they are placed at the beginning of the chain unless if its a non-transient asset then it skips the first entry

immortius

Overall, this request needs work. I'm concerned firstly with the scope of the changes. I think it is a bit overdone - in particular I do not believe a node based linked list system needed to be developed, as simpler tweaks around the existing implementation would have enabled the improvements. Also I'm concerned some changes introduce bugs or break encapsulation. Also some style issues.

I do agree with:

Changing normal assets to be kept via soft references
Cleaning up null weak references from the instance asset map
Adding tracking of parent urn to the asset references to enable the above

gestalt-asset-core/src/main/java/org/terasology/gestalt/assets/Asset.java

immortius · 2021-04-26T03:58:27Z

gestalt-asset-core/src/main/java/org/terasology/gestalt/assets/AssetType.java

@@ -642,13 +717,14 @@ public String toString() {
        }
    }

-    private static final class AssetReference<T> extends PhantomReference<T> {
+    public final class AssetReference<T extends Asset<?>> extends PhantomReference<T> {


Why is this public now? Why is it no longer static? There's no reason other classes should be doing anything with an asset reference. And AssetReference is not doing anything with it's parent asset type?

gestalt-asset-core/src/main/java/org/terasology/gestalt/assets/AssetType.java

pollend · 2021-04-26T05:18:17Z

Overall, this request needs work. I'm concerned firstly with the scope of the changes. I think it is a bit overdone - in particular I do not believe a node based linked list system needed to be developed, as simpler tweaks around the existing implementation would have enabled the improvements. Also I'm concerned some changes introduce bugs or break encapsulation. Also some style issues.

I do agree with:
* Changing normal assets to be kept via soft references

* Cleaning up null weak references from the instance asset map

* Adding tracking of parent urn to the asset references to enable the above 

One of the benefits with this scheme is it decouples a lot of the book keeping from AssetType. Assets can clone and insert a reference into the chain without depending on or notifying assetType of the change. at most you would just need to introduce a way to lock the urn which isn't too hard to add to the current code. an AssetType would just manage the bookkeeping and clean up references as they go stale. The implementation could be cleaned up quite a bit since there is a large amount of book keeping with the code I added. The PR is pretty rough since the implementation is quite a bit more complicated.

pollend · 2021-05-02T23:34:25Z

@immortius so I think this is a lot simpler then the original system. I decoupled the ownership of instanced assets from assetTypes. so assetTypes now only track non instanced assets. assets that create instances will now push it to a linked list. reloading just follows the chain of instanced assets and since each instance also has a strong link back to the parent asset then that resource is only held on as long as instanced assets are still valid. if a parent becomes invalid then all instanced assets should become invalid as well :D.

pollend · 2021-05-08T16:22:52Z

Update Asset Scheme for gestalt

gestalt-asset-core/src/main/java/org/terasology/gestalt/assets/Asset.java

immortius · 2021-06-14T02:08:40Z

gestalt-asset-core/src/main/java/org/terasology/gestalt/assets/Asset.java

-    final synchronized Optional<? extends Asset<T>> createCopy(ResourceUrn copyUrn) {
-        Preconditions.checkState(!disposed);
-        return doCreateCopy(copyUrn, assetType);
+    public final Asset<T> getNormalAsset() {


Please add a javadoc to this method (all public methods should have javadoc to assist a user.
Not clear from the name what this does - what makes an asset "normal"? I might suggest getArchetype() or something like that, not sure if that adds clarity though.

@pollend I think it's still a bit unclear what "the normal type" here is 🤔

getConcreteAsset doesn't really make this clearer to me.
please add a comment in code that explains what characterizes an asset as "concrete"

I reverted it back to normal asset and updated the javadocs.

gestalt-asset-core/src/main/java/org/terasology/gestalt/assets/AssetType.java

gestalt-asset-core/src/main/java/org/terasology/gestalt/assets/Asset.java

immortius · 2021-07-06T12:24:36Z

gestalt-asset-core/src/main/java/org/terasology/gestalt/assets/Asset.java

+        }
+    }
+
+    protected Iterable<WeakReference<Asset<T>>> instances() {


Can this be private/what reason is there for implementors to override this?

Or call it.

@pollend I think this question was not answered, was it? I see that you made it final, but why does it need to be on protected level?

@pollend this question is still open

you should be able to access all the list of instances from within the asset.

gestalt-asset-core/src/main/java/org/terasology/gestalt/assets/AssetType.java

immortius · 2021-07-06T12:32:19Z

gestalt-asset-core/src/main/java/org/terasology/gestalt/assets/AssetType.java

+        Reference<T> reference = loadedAssets.get(target);
+        Asset<U> current = reference.get();
+        if(current != null && current.isDisposed() && Iterables.isEmpty(current.instances())) {
+            logger.warn("non instanced asset is disposed with instances. instances will become orphaned.");


Probably would be cleaner to dispose the children when the parent is disposed. But maybe for a future PR.

immortius · 2021-07-06T12:36:57Z

gestalt-asset-core/src/main/java/org/terasology/gestalt/assets/AssetType.java

+            loadedAssets.remove(target);
+            return;
+        }
+        current.cleanup();


This line probably is unreachable without an asset being loaded fresh into the urn in the meantime. Which I guess is possible through the onAssetDisposed pathway, since it isn't synchronized. Not sure it is a meaningful action regardless.

gestalt-asset-core/src/main/java/org/terasology/gestalt/assets/AssetType.java

pollend · 2021-07-14T04:39:35Z

gestalt-asset-core/src/main/java/org/terasology/gestalt/assets/AssetType.java

    void onAssetDisposed(Asset<U> asset) {
-        if (asset.getUrn().isInstance()) {
-            instanceAssets.get(asset.getUrn()).remove(new WeakReference<>(assetClass.cast(asset)));
-        } else {
-            loadedAssets.remove(asset.getUrn());
+        if (!asset.getUrn().isInstance()) {
+            disposeAsset(asset.getUrn());
        }
    }


@immortius I don't think we need this onAssetDisposed call anymore. looks like the asset is already managed in the processDisposal call.

pollend requested review from immortius, 4Denthusiast and skaldarnar April 26, 2021 00:25

immortius requested changes Apr 26, 2021

View reviewed changes

pollend requested a review from immortius May 2, 2021 23:32

immortius requested changes Jul 6, 2021

View reviewed changes

immortius reviewed Jul 6, 2021

View reviewed changes

gestalt-asset-core/src/main/java/org/terasology/gestalt/assets/Asset.java Outdated Show resolved Hide resolved

immortius reviewed Jul 6, 2021

View reviewed changes

gestalt-asset-core/src/main/java/org/terasology/gestalt/assets/AssetType.java Outdated Show resolved Hide resolved

immortius reviewed Jul 6, 2021

View reviewed changes

gestalt-asset-core/src/main/java/org/terasology/gestalt/assets/AssetType.java Outdated Show resolved Hide resolved

pollend commented Jul 15, 2021

View reviewed changes

pollend added 11 commits November 21, 2021 20:26

chore: update asset scheme

3fe11d5

cleanup

fdcb4eb

simplify asset logic

02481af

chore: simplify cleanup logic

6302a1f

hide implementation details for instances

76e3b08

cleanup imports

2da83d1

core type argument

6d23d42

chore: remove custom linkedlist

186ad9c

chore: minor correct logic to handle asset cleanup and add javadocs

652f1a9

chore: update layout

41f75bc

chore: update assettype

29e14cd

pollend force-pushed the feature/update-asset-scheme branch from 305644f to 29e14cd Compare November 22, 2021 04:42

chore: correctly handled disposed asset

6f22479

pollend requested a review from immortius November 22, 2021 04:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: update asset scheme #109

chore: update asset scheme #109

pollend commented Apr 26, 2021 •

edited

immortius left a comment

immortius Apr 26, 2021

pollend commented Apr 26, 2021

pollend commented May 2, 2021 •

edited

pollend commented May 8, 2021

immortius Jun 14, 2021

jdrueckert Jul 25, 2021

jdrueckert Jul 31, 2021

pollend Aug 1, 2021

immortius Jul 6, 2021

immortius Jul 6, 2021

jdrueckert Jul 25, 2021

jdrueckert Jul 31, 2021

pollend Aug 1, 2021

immortius Jul 6, 2021

immortius Jul 6, 2021

pollend Jul 14, 2021

chore: update asset scheme #109

Are you sure you want to change the base?

chore: update asset scheme #109

Conversation

pollend commented Apr 26, 2021 • edited

immortius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pollend commented Apr 26, 2021

pollend commented May 2, 2021 • edited

pollend commented May 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pollend commented Apr 26, 2021 •

edited

pollend commented May 2, 2021 •

edited