From e40e5096ea7a7d71e4070aa9e1e095b5b4cc1f7b Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 26 Nov 2021 14:43:35 +0900 Subject: [PATCH 1/6] Remove `RealmLive` context re-fetch optimisation for now --- osu.Game/Database/RealmLive.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/osu.Game/Database/RealmLive.cs b/osu.Game/Database/RealmLive.cs index 73e6715aaa..dae4197309 100644 --- a/osu.Game/Database/RealmLive.cs +++ b/osu.Game/Database/RealmLive.cs @@ -114,7 +114,10 @@ namespace osu.Game.Database } } - private bool originalDataValid => !IsManaged || (isCorrectThread && data.IsValid); + // TODO: Revisit adding these conditionals back as an optimisation: || (isCorrectThread && data.IsValid); + // They have temporarily been removed due to an oversight involving .AsQueryable, see https://github.com/realm/realm-dotnet/discussions/2734. + // This means we are fetching a new context every `PerformRead` or `PerformWrite`, even when on the correct thread. + private bool originalDataValid => !IsManaged; // this matches realm's internal thread validation (see https://github.com/realm/realm-dotnet/blob/903b4d0b304f887e37e2d905384fb572a6496e70/Realm/Realm/Native/SynchronizationContextScheduler.cs#L72) private bool isCorrectThread From cb8fa803524d951d94dbb406322de050b304f9be Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 26 Nov 2021 21:33:11 +0900 Subject: [PATCH 2/6] Don't dispose fetched realm instance when using `RealmLive.Value` See https://github.com/realm/realm-dotnet/discussions/2734#discussioncomment-1705038 for reasoning. --- osu.Game/Database/RealmLive.cs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/osu.Game/Database/RealmLive.cs b/osu.Game/Database/RealmLive.cs index dae4197309..7ae7d8544a 100644 --- a/osu.Game/Database/RealmLive.cs +++ b/osu.Game/Database/RealmLive.cs @@ -102,10 +102,11 @@ namespace osu.Game.Database if (originalDataValid) return data; - T retrieved; + if (!isCorrectThread) + throw new InvalidOperationException($"Can't use {nameof(Value)} unless on the same thread the original data was fetched from."); - using (var realm = Realm.GetInstance(data.Realm.Config)) - retrieved = realm.Find(ID); + var realm = Realm.GetInstance(data.Realm.Config); + var retrieved = realm.Find(ID); if (!retrieved.IsValid) throw new InvalidOperationException("Attempted to access value without an open context"); From 791f7e380139b40389cde6d8927e4683dc25607e Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 29 Nov 2021 15:14:27 +0900 Subject: [PATCH 3/6] Update `RealmLive` tests in line with modified behaviour --- osu.Game.Tests/Database/RealmLiveTests.cs | 68 ++++++++++++++--------- 1 file changed, 42 insertions(+), 26 deletions(-) diff --git a/osu.Game.Tests/Database/RealmLiveTests.cs b/osu.Game.Tests/Database/RealmLiveTests.cs index f86761fdc8..63566394d6 100644 --- a/osu.Game.Tests/Database/RealmLiveTests.cs +++ b/osu.Game.Tests/Database/RealmLiveTests.cs @@ -66,19 +66,29 @@ namespace osu.Game.Tests.Database Task.Factory.StartNew(() => { - Assert.DoesNotThrow(() => + // TODO: The commented code is the behaviour we hope to obtain, but is temporarily disabled. + // See https://github.com/ppy/osu/pull/15851 + using (realmFactory.CreateContext()) { - using (realmFactory.CreateContext()) + Assert.Throws(() => { - var resolved = liveBeatmap.Value; + var __ = liveBeatmap.Value; + }); + } - Assert.IsTrue(resolved.Realm.IsClosed); - Assert.IsTrue(resolved.IsValid); - - // can access properties without a crash. - Assert.IsFalse(resolved.Hidden); - } - }); + // Assert.DoesNotThrow(() => + // { + // using (realmFactory.CreateContext()) + // { + // var resolved = liveBeatmap.Value; + // + // Assert.IsTrue(resolved.Realm.IsClosed); + // Assert.IsTrue(resolved.IsValid); + // + // // can access properties without a crash. + // Assert.IsFalse(resolved.Hidden); + // } + // }); }, TaskCreationOptions.LongRunning | TaskCreationOptions.HideScheduler).Wait(); }); } @@ -199,23 +209,29 @@ namespace osu.Game.Tests.Database Assert.AreEqual(0, updateThreadContext.All().Count()); Assert.AreEqual(0, changesTriggered); - var resolved = liveBeatmap.Value; - - // retrieval causes an implicit refresh. even changes that aren't related to the retrieval are fired at this point. - Assert.AreEqual(2, updateThreadContext.All().Count()); - Assert.AreEqual(1, changesTriggered); - - // even though the realm that this instance was resolved for was closed, it's still valid. - Assert.IsTrue(resolved.Realm.IsClosed); - Assert.IsTrue(resolved.IsValid); - - // can access properties without a crash. - Assert.IsFalse(resolved.Hidden); - - updateThreadContext.Write(r => + // TODO: Originally the following was using `liveBeatmap.Value`. This has been temporarily disabled. + // See https://github.com/ppy/osu/pull/15851 + liveBeatmap.PerformRead(resolved => { - // can use with the main context. - r.Remove(resolved); + // retrieval causes an implicit refresh. even changes that aren't related to the retrieval are fired at this point. + // ReSharper disable once AccessToDisposedClosure + Assert.AreEqual(2, updateThreadContext.All().Count()); + Assert.AreEqual(1, changesTriggered); + + // TODO: as above, temporarily disabled as it doesn't make sense inside a `PerformRead`. + // // even though the realm that this instance was resolved for was closed, it's still valid. + // Assert.IsTrue(resolved.Realm.IsClosed); + // Assert.IsTrue(resolved.IsValid); + + // can access properties without a crash. + Assert.IsFalse(resolved.Hidden); + + // ReSharper disable once AccessToDisposedClosure + updateThreadContext.Write(r => + { + // can use with the main context. + r.Remove(resolved); + }); }); } From 35d68d6ab039318bf024853ca3214c4d25368d2d Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 30 Nov 2021 11:47:29 +0900 Subject: [PATCH 4/6] Remove all optimisations from `RealmLive` --- osu.Game/Database/RealmLive.cs | 43 +++++++++------------------------- 1 file changed, 11 insertions(+), 32 deletions(-) diff --git a/osu.Game/Database/RealmLive.cs b/osu.Game/Database/RealmLive.cs index 7ae7d8544a..0b422fd36f 100644 --- a/osu.Game/Database/RealmLive.cs +++ b/osu.Game/Database/RealmLive.cs @@ -2,7 +2,7 @@ // See the LICENCE file in the repository root for full licence text. using System; -using System.Threading; +using osu.Framework.Development; using Realms; #nullable enable @@ -17,10 +17,7 @@ namespace osu.Game.Database { public Guid ID { get; } - public bool IsManaged { get; } - - private readonly SynchronizationContext? fetchedContext; - private readonly int fetchedThreadId; + public bool IsManaged => data.IsManaged; /// /// The original live data used to create this instance. @@ -35,14 +32,6 @@ namespace osu.Game.Database { this.data = data; - if (data.IsManaged) - { - IsManaged = true; - - fetchedContext = SynchronizationContext.Current; - fetchedThreadId = Thread.CurrentThread.ManagedThreadId; - } - ID = data.ID; } @@ -52,7 +41,7 @@ namespace osu.Game.Database /// The action to perform. public void PerformRead(Action perform) { - if (originalDataValid) + if (!IsManaged) { perform(data); return; @@ -71,7 +60,7 @@ namespace osu.Game.Database if (typeof(RealmObjectBase).IsAssignableFrom(typeof(TReturn))) throw new InvalidOperationException($"Realm live objects should not exit the scope of {nameof(PerformRead)}."); - if (originalDataValid) + if (!IsManaged) return perform(data); using (var realm = Realm.GetInstance(data.Realm.Config)) @@ -99,31 +88,21 @@ namespace osu.Game.Database { get { - if (originalDataValid) + if (!IsManaged) return data; - if (!isCorrectThread) - throw new InvalidOperationException($"Can't use {nameof(Value)} unless on the same thread the original data was fetched from."); + if (!ThreadSafety.IsUpdateThread) + throw new InvalidOperationException($"Can't use {nameof(Value)} on managed objects from non-update threads"); + + // When using Value, we rely on garbage collection for the realm instance used to retrieve the instance. + // As we are sure that this is on the same thread var realm = Realm.GetInstance(data.Realm.Config); - var retrieved = realm.Find(ID); - if (!retrieved.IsValid) - throw new InvalidOperationException("Attempted to access value without an open context"); - - return retrieved; + return realm.Find(ID); } } - // TODO: Revisit adding these conditionals back as an optimisation: || (isCorrectThread && data.IsValid); - // They have temporarily been removed due to an oversight involving .AsQueryable, see https://github.com/realm/realm-dotnet/discussions/2734. - // This means we are fetching a new context every `PerformRead` or `PerformWrite`, even when on the correct thread. - private bool originalDataValid => !IsManaged; - - // this matches realm's internal thread validation (see https://github.com/realm/realm-dotnet/blob/903b4d0b304f887e37e2d905384fb572a6496e70/Realm/Realm/Native/SynchronizationContextScheduler.cs#L72) - private bool isCorrectThread - => (fetchedContext != null && SynchronizationContext.Current == fetchedContext) || fetchedThreadId == Thread.CurrentThread.ManagedThreadId; - public bool Equals(ILive? other) => ID == other?.ID; } } From 2e31f5a338988fb69dd77c709c1e36f742dbb943 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 30 Nov 2021 11:55:13 +0900 Subject: [PATCH 5/6] Update tests to match new behaviour --- osu.Game.Tests/Database/RealmLiveTests.cs | 107 +++++++++++----------- 1 file changed, 53 insertions(+), 54 deletions(-) diff --git a/osu.Game.Tests/Database/RealmLiveTests.cs b/osu.Game.Tests/Database/RealmLiveTests.cs index 63566394d6..41c82399dc 100644 --- a/osu.Game.Tests/Database/RealmLiveTests.cs +++ b/osu.Game.Tests/Database/RealmLiveTests.cs @@ -46,53 +46,6 @@ namespace osu.Game.Tests.Database Assert.IsFalse(liveBeatmap.PerformRead(l => l.Hidden)); } - [Test] - public void TestValueAccessWithOpenContext() - { - RunTestWithRealm((realmFactory, _) => - { - RealmLive? liveBeatmap = null; - Task.Factory.StartNew(() => - { - using (var threadContext = realmFactory.CreateContext()) - { - var beatmap = threadContext.Write(r => r.Add(new RealmBeatmap(CreateRuleset(), new RealmBeatmapDifficulty(), new RealmBeatmapMetadata()))); - - liveBeatmap = beatmap.ToLive(); - } - }, TaskCreationOptions.LongRunning | TaskCreationOptions.HideScheduler).Wait(); - - Debug.Assert(liveBeatmap != null); - - Task.Factory.StartNew(() => - { - // TODO: The commented code is the behaviour we hope to obtain, but is temporarily disabled. - // See https://github.com/ppy/osu/pull/15851 - using (realmFactory.CreateContext()) - { - Assert.Throws(() => - { - var __ = liveBeatmap.Value; - }); - } - - // Assert.DoesNotThrow(() => - // { - // using (realmFactory.CreateContext()) - // { - // var resolved = liveBeatmap.Value; - // - // Assert.IsTrue(resolved.Realm.IsClosed); - // Assert.IsTrue(resolved.IsValid); - // - // // can access properties without a crash. - // Assert.IsFalse(resolved.Hidden); - // } - // }); - }, TaskCreationOptions.LongRunning | TaskCreationOptions.HideScheduler).Wait(); - }); - } - [Test] public void TestScopedReadWithoutContext() { @@ -148,6 +101,59 @@ namespace osu.Game.Tests.Database }); } + [Test] + public void TestValueAccessNonManaged() + { + RunTestWithRealm((realmFactory, _) => + { + var beatmap = new RealmBeatmap(CreateRuleset(), new RealmBeatmapDifficulty(), new RealmBeatmapMetadata()); + var liveBeatmap = beatmap.ToLive(); + + Assert.DoesNotThrow(() => + { + var __ = liveBeatmap.Value; + }); + }); + } + + [Test] + public void TestValueAccessWithOpenContextFails() + { + RunTestWithRealm((realmFactory, _) => + { + RealmLive? liveBeatmap = null; + Task.Factory.StartNew(() => + { + using (var threadContext = realmFactory.CreateContext()) + { + var beatmap = threadContext.Write(r => r.Add(new RealmBeatmap(CreateRuleset(), new RealmBeatmapDifficulty(), new RealmBeatmapMetadata()))); + + liveBeatmap = beatmap.ToLive(); + } + }, TaskCreationOptions.LongRunning | TaskCreationOptions.HideScheduler).Wait(); + + Debug.Assert(liveBeatmap != null); + + Task.Factory.StartNew(() => + { + // Can't be used, without a valid context. + Assert.Throws(() => + { + var __ = liveBeatmap.Value; + }); + + // Can't be used, even from within a valid context. + using (realmFactory.CreateContext()) + { + Assert.Throws(() => + { + var __ = liveBeatmap.Value; + }); + } + }, TaskCreationOptions.LongRunning | TaskCreationOptions.HideScheduler).Wait(); + }); + } + [Test] public void TestValueAccessWithoutOpenContextFails() { @@ -209,8 +215,6 @@ namespace osu.Game.Tests.Database Assert.AreEqual(0, updateThreadContext.All().Count()); Assert.AreEqual(0, changesTriggered); - // TODO: Originally the following was using `liveBeatmap.Value`. This has been temporarily disabled. - // See https://github.com/ppy/osu/pull/15851 liveBeatmap.PerformRead(resolved => { // retrieval causes an implicit refresh. even changes that aren't related to the retrieval are fired at this point. @@ -218,11 +222,6 @@ namespace osu.Game.Tests.Database Assert.AreEqual(2, updateThreadContext.All().Count()); Assert.AreEqual(1, changesTriggered); - // TODO: as above, temporarily disabled as it doesn't make sense inside a `PerformRead`. - // // even though the realm that this instance was resolved for was closed, it's still valid. - // Assert.IsTrue(resolved.Realm.IsClosed); - // Assert.IsTrue(resolved.IsValid); - // can access properties without a crash. Assert.IsFalse(resolved.Hidden); From f3f77fa05316faa2e046edf7ef116058f190c9fa Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 30 Nov 2021 11:56:53 +0900 Subject: [PATCH 6/6] Update missed xmldoc/comments --- osu.Game/Database/ILive.cs | 4 ++-- osu.Game/Database/RealmLive.cs | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/osu.Game/Database/ILive.cs b/osu.Game/Database/ILive.cs index a863339f11..3011754bc1 100644 --- a/osu.Game/Database/ILive.cs +++ b/osu.Game/Database/ILive.cs @@ -38,10 +38,10 @@ namespace osu.Game.Database bool IsManaged { get; } /// - /// Resolve the value of this instance on the current thread's context. + /// Resolve the value of this instance on the update thread. /// /// - /// After resolving the data should not be passed between threads. + /// After resolving, the data should not be passed between threads. /// T Value { get; } } diff --git a/osu.Game/Database/RealmLive.cs b/osu.Game/Database/RealmLive.cs index 0b422fd36f..2accea305a 100644 --- a/osu.Game/Database/RealmLive.cs +++ b/osu.Game/Database/RealmLive.cs @@ -95,8 +95,7 @@ namespace osu.Game.Database throw new InvalidOperationException($"Can't use {nameof(Value)} on managed objects from non-update threads"); // When using Value, we rely on garbage collection for the realm instance used to retrieve the instance. - // As we are sure that this is on the same thread - + // As we are sure that this is on the update thread, there should always be an open and constantly refreshing realm instance to ensure file size growth is a non-issue. var realm = Realm.GetInstance(data.Realm.Config); return realm.Find(ID);