Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add initial realm database implementation with KeyBindingStore migration #11461

Merged
merged 124 commits into from
Jun 18, 2021

Conversation

peppy
Copy link
Member

@peppy peppy commented Jan 12, 2021

Now that realm have fixed the bug causing windows crashes on multi-threaded usage (realm/realm-dotnet#1933), I've begun to look at how we can make use of Realm. See #7057 for previous experimentation and discussion.

This is a first step towards an attempt to migrate to realm as our database backend. Unlike previous attempts at this (where I tried to update all stores to use realm at once), this takes the lowest hanging fruit and adds complete migration and implementation, while keeping the majority of the game still using the original EF backing.

There are multiple reasons for taking on this simplified direction:

  • Smaller diff means it will likely be easier to parse and merge
  • Tackling the KeyBindingStore avoids much of the consideration required for cross-thread access, as all usages are detached (ie. copied away from realm)
  • Allows for creating a prototype setup for migration of data (previous attempt would start from scratch, which I'd like to avoid if possible)

I'd recommend reading this article as a primer to realm and how it should/shouldn't be used.

Dependencies

Considerations

Live<> (aka RealmWrapper<>) inclusion

Even though I have not used this class in the KeyBindingStore implementation, I have included it in an initial state. It doesn't have test coverage yet, but I wanted to have it present to give an idea on my current approach to non-detached usages. I will likely add some more test coverage over the next couple of days.

Would appreciate any feedback on the general structure of this class, even if it doesn't have explicit usage yet.

Context refreshing

Generally it is recommended that the main realm context is refreshed as part of the program update loop. This is likely something we will want in the future, but unnecessary given the scope of usage right now. A refresh will occur on realm context retrieval for the time being.

This will need to change if/when we start using Live<> data objects, else stale data will start to become a thing.

One consideration here is that we currently open a context per thread accessing realm. These are kept open indefinitely, and in the case of a background thread, may not be refreshed for some time. This is known to cause realm filesize growth (as without having them close or refresh, they will be referencing an old snapshot of the full realm database). I'm still investigating a solution to this. It may require some kind of background process as part of the CompositeDrawable's ThreadedTaskScheduler to run after x seconds of not using a thread (to close the context), or on a regular schedule (to run refresh on the context).

Again, this is not too important this early in implementation and given the limited size of key binding data, but requires further thought.

Remaining questions

There's some remaining questions which I plan to reach out to the realm team to get answers from (after doing my own investigation, in some cases):

  • Is there an overhead for a context when no write operations occurred?
  • Is there any performance consideration/benefit to using ThreadSafeReference vs Find(pk)?
  • At what point is it considered lower overhead to copy from realm rather than access via live
  • Safety of keeping multiple realm contexts open across multiple threads. Ability to close a context from off-thread?
  • [PrimaryKey] is not found if it's on an interface definition
  • What is the overhead of opening a realm? It uses a shared handle (seems to be refcounted per thread internally) which means that opening multiple times if already open should be fast, but what if we stopped doing our own threadLocal management and opened/closed the realm each time?
  • What is the overhead of calling Refresh (every frame)?
  • Any way to handle custom serialisation?
  • Realm throws on concurrent transactions, is there no way to block until previous completes?
  • Are realm file handles supposed to be closed immediately after disposing the last open realm instance?

peppy added 30 commits January 11, 2021 15:52
Also improve the Query method for action types by using generic field
@peppy
Copy link
Member Author

peppy commented Jun 11, 2021

@gagahpangeran care to share your operating system? I've tested with fresh installs on windows and macOS and it works as expected, for what it's worth.

@gagahpangeran
Copy link
Contributor

openSUSE Tumbleweed, linux kernel 5.12.4, X86_64.

@bdach
Copy link
Collaborator

bdach commented Jun 11, 2021

@peppy I have been running under Linux but I haven't admittedly ran the version as it is right now. May be something new-ish.

@bdach
Copy link
Collaborator

bdach commented Jun 11, 2021

I've tried again on my linux box and I cannot seem to reproduce deadlocks, the game launches fine. Tried both with an existing sqlite DB but the realm items removed, and with a fresh install. uname -a says

Linux dachb-7559 4.15.0-144-generic #148-Ubuntu SMP Sat May 8 02:33:43 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux

Maybe the kernel version matters somehow? I haven't (manually) updated this box for a while now.

@smoogipoo
Copy link
Contributor

Mine: Linux smgi 5.10.30-1-MANJARO #1 SMP Wed Apr 14 08:07:27 UTC 2021 x86_64 GNU/Linux

@smoogipoo
Copy link
Contributor

smoogipoo commented Jun 11, 2021

Here's a native stack:

* thread #85, name = 'GameThread.Upda'
  * frame #0: 0x00007f2b995f09ba libpthread.so.0`__futex_abstimed_wait_common64 + 202
    frame #1: 0x00007f2b995ea260 libpthread.so.0`pthread_cond_wait@@GLIBC_2.3.2 + 512
    frame #2: 0x00007f2b994c0bb1 libstdc++.so.6`std::condition_variable::wait(std::unique_lock<std::mutex>&) [inlined] __gthread_cond_wait(__mutex=<unavailable>, __cond=<unavailable>) at gthr-default.h:865:38
    frame #3: 0x00007f2b994c0ba8 libstdc++.so.6`std::condition_variable::wait(this=<unavailable>, __lock=<unavailable>) at condition_variable.cc:53
    frame #4: 0x00007f291d307453 librealm-wrappers.so`realm::_impl::NotifierPackage::package_and_wait(realm::util::Optional<unsigned long>) + 867
    frame #5: 0x00007f291d324c18 librealm-wrappers.so`realm::Realm::do_refresh() + 408
    frame #6: 0x00007f291d37abf3 librealm-wrappers.so`shared_realm_refresh + 67
    frame #7: 0x00007f2b23330f3a
    frame #8: 0x00007f2b24054cf0
    frame #9: 0x00007f2b24054c96
    frame #10: 0x00007f2b24054c07
    frame #11: 0x00007f2b23f8deed
    frame #12: 0x00007f2b23f8dcbd
    frame #13: 0x00007f2b23f8dcbd
    frame #14: 0x00007f2b23f8dcbd
    frame #15: 0x00007f2b23f8dcbd
    frame #16: 0x00007f2b23f8dcbd
    frame #17: 0x00007f2b21d35914
    frame #18: 0x00007f2b21d2eaf5
    frame #19: 0x00007f2b21d2dadb
    frame #20: 0x00007f2b1f110442
    frame #21: 0x00007f2b1f119841
    frame #22: 0x00007f2b1f11055e
    frame #23: 0x00007f2b98a71ab7 libcoreclr.so`___lldb_unnamed_symbol9785$$libcoreclr.so + 124
    frame #24: 0x00007f2b988c08ab libcoreclr.so`___lldb_unnamed_symbol4377$$libcoreclr.so + 1643
    frame #25: 0x00007f2b988d1572 libcoreclr.so`___lldb_unnamed_symbol4565$$libcoreclr.so + 434
    frame #26: 0x00007f2b9888960a libcoreclr.so`___lldb_unnamed_symbol3721$$libcoreclr.so + 330
    frame #27: 0x00007f2b98889c1d libcoreclr.so`___lldb_unnamed_symbol3722$$libcoreclr.so + 45
    frame #28: 0x00007f2b988d16e0 libcoreclr.so`___lldb_unnamed_symbol4566$$libcoreclr.so + 192
    frame #29: 0x00007f2b98bf065e libcoreclr.so`___lldb_unnamed_symbol15243$$libcoreclr.so + 590
    frame #30: 0x00007f2b995e4299 libpthread.so.0`start_thread + 233
    frame #31: 0x00007f2b991ca053 libc.so.6`__clone + 67

I've also tested 10.2.0-beta.2 and 10.1.4 (previous stable version) and this still occurs.

@gagahpangeran
Copy link
Contributor

Is this still related to realm/realm-dotnet#2245 ?

@peppy
Copy link
Member Author

peppy commented Jun 11, 2021

While it might be related to some similar scenario in the linux implementation, the report in the linked issue (happening on windows) is already fixed in the beta releases, as already mentioned in this thread.

@gagahpangeran
Copy link
Contributor

As a heads up, I test this PR again with the latest release of Realm version 10.2.0 and now it works.

@smoogipoo
Copy link
Contributor

Screenshot_2021-06-18_16-42-30

Do you get this, when pressing the gameplay mouse buttons binding?

@peppy
Copy link
Member Author

peppy commented Jun 18, 2021

Can reproduce; will investigate.

@peppy
Copy link
Member Author

peppy commented Jun 18, 2021

Should be resolved, along with a weird thread concurrency issue.

Comment on lines +427 to +428
if (keyBinding.IsManaged)
throw new ArgumentException("Key binding should not be attached as we make temporary changes", nameof(keyBinding));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it was done this way to keep it simple and not have many abstractions over database model doing magic behind the scenes.

I would agree with immutability, though.

List<RealmKeyBinding> bindings;

using (var usage = realmFactory.GetForRead())
bindings = usage.Realm.All<RealmKeyBinding>().Where(b => b.RulesetID == rulesetId && b.Variant == variant).Detach();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference between calling Detach() and Freeze() in this scenario? Is Freeze() more expensive because it creates a frozen realm?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Freezing objects requires the source realm (the one which the freeze was run on) to be kept open, and disposed of once it's finished. Keeping frozen objects/realms open will keep that version of the realm on disk, which can lead to filesize bloat depending on how many follow-up changes are made while it's kept frozen.

@smoogipoo
Copy link
Contributor

Looks good to me. I personally think the build time is acceptable, but surely we could improve it by moving models to a separate project. Maybe best done as a follow-up?

For me, I don't know anything about Fody, and I can tell that the issue is already going to be super deep and not easy to resolve - at least several days of work and figuring out how to even debug such a compile-time process.

@peppy peppy merged commit 3f336d8 into ppy:master Jun 18, 2021
@peppy peppy deleted the realm-key-binding-store branch June 21, 2021 04:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
realm deals with local realm database size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants