Speed-up calls to Realm.Manage #857

nirinchev · 2016-10-06T13:06:37Z

This is a change that aims two things

Bring the performance of Realm.Manage closer to Realm.CreateObject
Pave the way for AddOrUpdate (Update object by PrimaryKey #808)

The essence here is weaving CopyToRealm in IRealmObjectHelper implementors that replaces the reflection that we used before. Here's the performance comparison

	CreateObject	Manage (woven)	Manage (reflection)
Sparse	1235	~~2931~~ 1523	11291
Dense	827	951	2527

Times are in ms and the test was performed with 100000 objects.
Sparse refers to objects with a lot of properties, where only a few are set.
Dense refers to objects where all properties are set.

The current implementation guards against hitting ObjectStore by doing

if (field != default(fieldType))
{
    Property = field
}

only for value types, objects, and lists. It doesn't support nullable value types because the IL is getting out of hand as is and didn't seem like it would add a lot of value.

fealebenpae

This looks very good, aside from a couple of things noted.

It seems to me that it would be very cheap for us to always compare scalar fields against their default value, and it shouldn't be much effort to add this check. Can you extend this PR with that?

fealebenpae · 2016-10-06T13:38:23Z

Realm.Shared/Dynamic/DynamicRealmObjectHelper.cs

@@ -25,6 +27,34 @@ internal class DynamicRealmObjectHelper : IRealmObjectHelper
    {
        internal static readonly DynamicRealmObjectHelper Instance = new DynamicRealmObjectHelper();

+        public void CopyToRealm(RealmObject instance)


Objects whose helper is DynamicRealmObjectHelper cannot exist in an unmanaged state - they always belong to a realm. I think it's safe to make this method throw an exception instead.

fealebenpae · 2016-10-06T13:44:18Z

RealmWeaver/ModuleWeaver.cs

+				*foreach* list woven property in casted's schema
+				var list = casted.field;
+				casted.field = null;
+				((ICopyValuesFrom)casted.Property).CopyValuesFrom(list);


We should get rid of ICopyValuesFrom instead of making it public. Let's add a public AddRange(IEnumerable<T>) to RealmList<T> and call that instead.

That will clash with our desire to make RealmList<T> internal (#853), so which one do we want more?

We want both :-) Do you think it will be too much for this PR?

Hmm, you're right. How about we weave the foreach cycling in CopyToRealm which would call IList<T>.Add(T)?

That was a bit of a pain, but I got rid of ICopyValuesFrom 😤 I'll leave internalizing RealmList to #858.

Very nice!!

nirinchev · 2016-10-06T14:32:53Z

I figured it's better to split it in two, so that we can brag about "major speed improvements to Manage" in our release notes. Then after a release or two, we could add the default check and brag about speed improvements again 😆 If we make it ultra-fast in one go, people will think we're not improving anymore!

fealebenpae · 2016-10-06T15:22:45Z

Oh haha, fair point. We can save some performance improvements for the next release.

kristiandupont · 2016-10-06T16:31:34Z

Tests/IntegrationTests.Shared/StandAloneObjectTests.cs

@@ -85,5 +82,90 @@ public void RealmObject_WhenStandalone_ShouldHaveDefaultEqualsImplementation()

            Assert.DoesNotThrow(() => _person.Equals(otherPerson));
        }
+
+        [Test]
+        public void RealmObject_WhenManaged_ShouldNotThrow()


This test is fine but I would also like a test in WeaverTests as well that verifies that the expected get and set methods are invoked.

Sure, I will take a look at the WeaverTests infrastructures and try to come up with one.

kristiandupont · 2016-10-06T16:32:21Z

RealmWeaver/ModuleWeaver.cs

+				*foreach* list woven property in casted's schema
+				var list = casted.field;
+				casted.field = null;
+				((ICopyValuesFrom)casted.Property).CopyValuesFrom(list);


We want both :-) Do you think it will be too much for this PR?

nirinchev · 2016-10-06T22:55:42Z

Ping @fealebenpae, @kristiandupont care to take a second look?

kristiandupont · 2016-10-06T23:16:26Z

RealmWeaver/ModuleWeaver.cs

@@ -810,6 +809,83 @@ private static bool IsRealmList(PropertyDefinition property)
        return property.PropertyType.Name == "RealmList`1" && property.PropertyType.Namespace == "Realms";
    }

+    private static bool IsDateTimeOffset(PropertyDefinition property)


I know we've been using strings already, but would it be nicer to create something like

private static bool Is<T>(PropertyDefinition property) { return property.PropertyType.Name == typeof(T).Name && ... }

?

Yes it would, but I didn't want to diverge too much from the coding style. Also, when we add StyleCop, we'll probably have to reorder/reformat the whole file and it'll complain about constants/nameof/typeof :)

It bugs me that we do string comparison in the first place - I'd much rather we compare TypeDefinition instances.

fealebenpae · 2016-10-07T11:53:03Z

I believe that you should remove the BackingFieldName property of WovenPropertyAttribute - it shouldn't be used anymore.

nirinchev · 2016-10-07T12:14:00Z

~~Do we need the WovenPropertyAttribute in that case?~~ It seems we do need it after all. I'll get rid of just the backing field then.

…equired

fealebenpae · 2016-10-07T13:02:15Z

We could get rid of WovenPropertyAttribute, but right now it's a very useful marker for properties that are persisted. Prior to that we had duplicate code in the library and the weaver.

👍 from me.

kristiandupont · 2016-10-07T16:31:44Z

👍 from me as well. Very nice work!

nirinchev added 2 commits October 6, 2016 13:55

Weave in a CopyToRealm to IRealmObjectHelper

56f9800

Add some tests

6bf6655

nirinchev added the T-Enhancement label Oct 6, 2016

nirinchev added this to the v1.0 milestone Oct 6, 2016

nirinchev assigned fealebenpae and kristiandupont Oct 6, 2016

realm-ci assigned nirinchev Oct 6, 2016

Fix whitespace issues

8aec775

nirinchev force-pushed the ni/manage-speedup branch from 1c8532f to 8aec775 Compare October 6, 2016 13:17

fealebenpae requested changes Oct 6, 2016

View reviewed changes

nirinchev mentioned this pull request Oct 6, 2016

Rename public API to adhere to .NET standards #858

Merged

11 tasks

kristiandupont added the S:Review label Oct 6, 2016

kristiandupont reviewed Oct 6, 2016

View reviewed changes

Add an if-check before setting properties in Manage

cd87b56

kristiandupont reviewed Oct 6, 2016

View reviewed changes

fealebenpae approved these changes Oct 7, 2016

View reviewed changes

Add weaver test for CopyToRealm

df90332

Remove WovenPropertyAttribute.BackingFieldName since it's no longer r…

0f0b46b

…equired

nirinchev merged commit 6eee401 into master Oct 7, 2016

nirinchev deleted the ni/manage-speedup branch October 7, 2016 18:38

nirinchev removed the S:Review label Oct 7, 2016

nirinchev mentioned this pull request Mar 28, 2023

Annotate the test assemblies for nullability #3274

Merged

2 tasks

github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Speed-up calls to Realm.Manage #857

Speed-up calls to Realm.Manage #857

nirinchev commented Oct 6, 2016 •

edited

Loading

fealebenpae left a comment

fealebenpae Oct 6, 2016

fealebenpae Oct 6, 2016

nirinchev Oct 6, 2016

kristiandupont Oct 6, 2016

fealebenpae Oct 6, 2016

nirinchev Oct 6, 2016 •

edited

Loading

kristiandupont Oct 6, 2016

nirinchev commented Oct 6, 2016

fealebenpae commented Oct 6, 2016

kristiandupont Oct 6, 2016

nirinchev Oct 6, 2016

kristiandupont Oct 6, 2016

nirinchev commented Oct 6, 2016 •

edited

Loading

kristiandupont Oct 6, 2016

nirinchev Oct 6, 2016

fealebenpae Oct 7, 2016

fealebenpae commented Oct 7, 2016

nirinchev commented Oct 7, 2016 •

edited

Loading

fealebenpae commented Oct 7, 2016

kristiandupont commented Oct 7, 2016

Speed-up calls to Realm.Manage #857

Speed-up calls to Realm.Manage #857

Conversation

nirinchev commented Oct 6, 2016 • edited Loading

fealebenpae 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

nirinchev Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nirinchev commented Oct 6, 2016

fealebenpae commented Oct 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nirinchev commented Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fealebenpae commented Oct 7, 2016

nirinchev commented Oct 7, 2016 • edited Loading

fealebenpae commented Oct 7, 2016

kristiandupont commented Oct 7, 2016

nirinchev commented Oct 6, 2016 •

edited

Loading

nirinchev Oct 6, 2016 •

edited

Loading

nirinchev commented Oct 6, 2016 •

edited

Loading

nirinchev commented Oct 7, 2016 •

edited

Loading