Add deterministic asset id generation process for asset caches. #264

chrisfromwork · 2019-10-31T19:27:22Z

This review addresses: #47. It contains the following changes:

A new AssetId class has been created to use as a key for AssetCacheEntries. This type contains a GUID and long is based on Asset file GUIDs and file identifiers
Asset caches are updated to use this new AssetId key

Breaking Change Details:

Notes:

StringGuid keys for asset caches have been updated to AssetIds so that they can be deterministically created on different computers. This will break old AssetCaches.

Migration Instructions:

Delete asset caches in your Generated.StateSynchronization.AssetCaches\Resources folder (these will be the .asset files).
Call Spectator View -> Update All Asset Caches in your project to regenerate asset caches.
Recompile all player flavors (UWP, iOS, Android) of your application.

chrisfromwork · 2019-10-31T19:34:06Z

...w/Scripts/StateSynchronization/NetworkedComponents/TextMeshPro/TextMeshProBroadcasterBase.cs

@@ -59,6 +59,8 @@ protected override TextMeshProBroadcasterChangeType CalculateDeltaChanges()

        protected override void SendCompleteChanges(IEnumerable<SocketEndpoint> endpoints)
        {
+            previousText = this.textMesh.text;
+            previousProperties = new TextMeshProperties(textMesh);


There was some weird behavior here where font details were sent prior to populating on this first call to SendCompleteChanges

wiwei · 2019-11-07T17:41:12Z

...emo.Unity/Assets/Generated.StateSynchronization.AssetCaches/Resources/SpriteAssetCache.asset

-      m_storage: 74fae14d-39c0-497e-ae6d-72f1f2084231
-    Asset: {fileID: 21300000, guid: d651fc00b471cfe44834a222e9433461, type: 3}
+      guid:
+        m_storage: 00000000-0000-0000-f000-000000000000


00000000-0000-0000-f000-000000000000 [](start = 19, length = 36)

This GUID seems to be strannnnnnnnnnngely full of zeros - is it just the case that some portion of our deterministic guids are going to look like this?

It's just interesting seeing a bunch of other GUIDs that look like regular GUIDs, and then seeing this guid (and the c000 one) which looks like a bunch of zeroes that then got smashed by a cosmic ray.

I have seen guids like this for Unity's default assets. Things like cube and cone meshes have similar guids.

wiwei · 2019-11-07T17:44:50Z

src/SpectatorView.Unity/Assets/SpectatorView/Scripts/StateSynchronization/AssetCache.cs

                }
            }

            CleanUpUnused(oldAssets, unvisitedAssets);
-            assets = oldAssets.Values.ToArray();
+            assets = oldAssets.Values.OrderBy(x => x.AssetId.FileIdentifier).ThenBy(x => x.AssetId.Guid).ToArray();


OrderBy(x => x.AssetId.FileIdentifier).ThenBy(x => x.AssetId.Guid) [](start = 38, length = 66)

If we're changing the mechanics of how our assets are enumerated (i.e. the EnumerateAllAssets). it might be worth documenting in the surfaces where this is exposed (i.e. EnumerateAllAssets will enumerate assets on File Identifier and then Guid)

Wouldn't we want to orderby Guid first, then file Id? As Guid represents the file, and fileId is the id within that file

wiwei · 2019-11-07T17:46:29Z

src/SpectatorView.Unity/Assets/SpectatorView/Scripts/StateSynchronization/AssetCacheEntry.cs

+        [SerializeField]
+        private StringGuid guid;
+
+        public long FileIdentifier => fileIdentifier;


FileIdentifier [](start = 20, length = 14)

Can you add a comment about what "FileIdentifier" is?

It's possible that this location isn't the right place for the comment (i.e. it's somewhat abstract at this location) but I'm trying to get a little more clarity for a reader who sees an abstract number (i.e. file ID), then look at this code, and ideally there should be some explanation/direction that this point that guides them to what's up

src/SpectatorView.Unity/Assets/SpectatorView/Scripts/StateSynchronization/AssetCacheEntry.cs

wiwei · 2019-11-07T17:56:14Z

        return new StringGuid { m_storage = rhs.ToString("D") };

Now that I'm seeing this in two places, what's the significance of "D"? Is it just a sentinel string (i.e. "private static string INVALID_GUID = "D"?)

Refers to: src/SpectatorView.Unity/Assets/SpectatorView/Scripts/StateSynchronization/StringGuid.cs:17 in 07dadf2. [](commit_id = 07dadf2, deletion_comment = False)

wiwei

andreiborodin · 2019-11-08T02:43:48Z

src/SpectatorView.Unity/Assets/SpectatorView/Scripts/StateSynchronization/AssetCacheEntry.cs

+
+        public override int GetHashCode()
+        {
+            return FileIdentifier.GetHashCode() ^ Guid.ToString().GetHashCode();


This will do an allocation everytime you add to the dictionary, because GetHashCode is called, which will create a string from Guid.

Instead, just do Guid.GetHashCode().

Sounds good. I'll pick these changes up in stabilization/1.1.0 with the expectation to merge back into master

andreiborodin · 2019-11-08T02:46:13Z

src/SpectatorView.Unity/Assets/SpectatorView/Scripts/StateSynchronization/AssetCacheEntry.cs

+        }
+
+        public static bool operator ==(AssetId lhs, AssetId rhs)
+        {


I would simplify this to: Equals(lhs, rhs); which would propogate to your Equals override above.

I would update that method to not use this one, as you already did a null check, and "this" can't be null. So just compare Guid and FileIndentierifer.

Also, don't do Guid.ToString() for comparison, just compare Guids. Guid.Tostring() is an allocation

hm, what aspect of Guid.ToString() is an allocation? StringGuid has a custom ToString definition that just returns the internally stored string value for the Guid. I'm not 100% if historically this type was made for serializtion, but we aren't currently using the default Guid type.

public override string ToString() { return (m_storage == null) ? System.Guid.Empty.ToString("D") : m_storage; }

Now looking at:
https://github.com/microsoft/referencesource/blob/master/mscorlib/system/guid.cs

if (formatCh == 'D' || formatCh == 'd') { guidString = string.FastAllocateString(36); }

Looks like there is no special-casing for an empty guid, unless I missed it.

ivte-ms · 2019-11-08T18:55:04Z

src/SpectatorView.Unity/Assets/SpectatorView/Scripts/StateSynchronization/StringGuid.cs

@@ -7,7 +7,7 @@
 namespace Microsoft.MixedReality.SpectatorView
 {
    [Serializable]
-    internal class StringGuid
+    internal class StringGuid : IComparable


StringGuid [](start = 19, length = 10)

Out of curiosity, what's wrong with the ordinary GUID? It's already comparable, and should be way faster to work with.

I'm not sure historically why the GUID wasn't used. is a GUID serializable?

The docs say that it has SerializableAttribute, so it should be.

I'm genuinely curious why the string representation was chosen instead. Maybe there was some issue with the Unity integration?

chrisfromwork added the breaking change Pull request contains a breaking change and requires additional information. label Oct 31, 2019

chrisfromwork requested review from andreiborodin, wiwei, matthejo and mrbobbybobberson October 31, 2019 19:27

chrisfromwork commented Oct 31, 2019

View reviewed changes

merge past commits

6448530

chrisfromwork force-pushed the largeAssets branch from da96fe6 to 6448530 Compare October 31, 2019 19:37

remove debug Log

07dadf2

chrisfromwork force-pushed the largeAssets branch from daf2b36 to 75c1fe1 Compare November 1, 2019 23:30

chrisfromwork requested review from ivte-ms, fibann and stephenatwork November 5, 2019 17:07

chrisfromwork changed the title ~~Add deterministic asset id generation process for asset caches.~~ Add deterministic asset id generation process for asset caches and defer loading assets. Nov 5, 2019

chrisfromwork force-pushed the largeAssets branch from dd422d1 to 07dadf2 Compare November 6, 2019 23:10

chrisfromwork changed the title ~~Add deterministic asset id generation process for asset caches and defer loading assets.~~ Add deterministic asset id generation process for asset caches. Nov 6, 2019

fix meta files

47e49a0

wiwei reviewed Nov 7, 2019

View reviewed changes

src/SpectatorView.Unity/Assets/SpectatorView/Scripts/StateSynchronization/AssetCacheEntry.cs Show resolved Hide resolved

wiwei reviewed Nov 7, 2019

View reviewed changes

src/SpectatorView.Unity/Assets/SpectatorView/Scripts/StateSynchronization/AssetCacheEntry.cs Show resolved Hide resolved

wiwei approved these changes Nov 7, 2019

View reviewed changes

Chris Barth added 2 commits November 7, 2019 09:58

add comment around organized asset cache

9c8995b

add comments to fields

40ea271

chrisfromwork merged commit 049aa75 into microsoft:master Nov 7, 2019

andreiborodin reviewed Nov 8, 2019

View reviewed changes

ivte-ms reviewed Nov 8, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add deterministic asset id generation process for asset caches. #264

Add deterministic asset id generation process for asset caches. #264

chrisfromwork commented Oct 31, 2019 •

edited

Loading

chrisfromwork Oct 31, 2019

wiwei Nov 7, 2019

chrisfromwork Nov 7, 2019

wiwei Nov 7, 2019

andreiborodin Nov 8, 2019

wiwei Nov 7, 2019

wiwei commented Nov 7, 2019

wiwei left a comment

andreiborodin Nov 8, 2019

chrisfromwork Nov 8, 2019

andreiborodin Nov 8, 2019

chrisfromwork Nov 8, 2019

ivte-ms Nov 8, 2019

ivte-ms Nov 8, 2019

chrisfromwork Nov 8, 2019

ivte-ms Nov 11, 2019

Add deterministic asset id generation process for asset caches. #264

Add deterministic asset id generation process for asset caches. #264

Conversation

chrisfromwork commented Oct 31, 2019 • edited Loading

Breaking Change Details:

Notes:

Migration Instructions:

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

wiwei commented Nov 7, 2019

wiwei left a 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

chrisfromwork commented Oct 31, 2019 •

edited

Loading