Internal refactor wishlist

[cmelchior]

This issue is a meta issue for tracking and discussing the most important internal refactors we would like to do in the short-team:

So far I have compiled the following list:

Tracked here:

• (Claus - 3) Rename RealmObjectHelper to AccessorUtils/Helper or something alike.

• (Christian - 2/3) Cleanup the structure of our unit tests and make sure they follow a defined pattern.

  • (Claus) We should revisit the concept of having a test interface for a specific concept that is applied/enforced across all the implementing types. NotificationTests is an example of this, but is currently a mess of different patterns (empty default implementations, ignored tests, etc) and is maybe not playing well with running JVM tests. There are plenty of things that could probably benefit from such a pattern (Deletable, all managed entities (update outside transaction, after close, etc.), ...), so we should either verify the approach or come up with another pattern.
  • (Claus) With the RealmSchema classes it should be easier to write tests that enforce exhaustiveness by matching the actual schema information (RealmSchema and RealmStorageTypes) against our existing test metadata infrastructure (TypeDescriptor.kt).

• (Claus - 3) Cleanup compiler plugin; better naming, reuse RealmSchema API through out analysis/transformation phases (eliminate looking up the same information twice), consider putting custom origins on all our modifications.

  • Needs to be clarified exactly what is needed.
  • Also need documentation on all classes.

• (Claus - 3) Cleaning internal package namespace. Since there is no concept of package private we might as well group interfaces more clearly.

  • Maybe just document for future code then cleanup later

• (Claus - 3) CI Parallelization. I guess it has its whole own issue, but should prioritize this soon.

• (Claus - 4) Typed enums. Clemente and I tried out if we could used java enums for C-API enums. We already put up the PR for it in #458, but didn't end on a conclusion whether it was worth it.

• (Claus - 3) Adding an explicit RealmObjectCompanion to DynamicRealmObject/DynamicMutableRealmObject could simplify (or potentially eliminate 🤔) the Mediator as we would be able to fetch the RealmObjectCompanion directly from any KClass<out RealmObject> and instantiate objects through its newInstance method instead of going through Mediator.createInstanceOf().

  • Future improvement?

• (Claus - 3) Bundling up RealmContext of our current state objects. We currently have an awful lot of methods taking a RealmReference and the Mediator. Maybe we can eliminate the Mediator completely (see above), but otherwise it would be sane to do a composite object of the information needed to link an object to its current realm-context. Have in mind not to create a God object that leaks grants access to all information, but still ... 🤔

Tracked elsewhere:

(Christian - 1-3?) We need to figure out how to split cinterop so it supports both base and sync builds. Instead of right now where sync native code is getting shipped with the base library. The networking refactor in core, should in theory remove the need to depend on us shipping SSL, but it is unclear what their timeline is.

• Claus has some ideas on this.

• We should try to avoid leaking internal implementations. Our different modules causes symboles to leak into the public interface.

• How far is Core with unified networking?

• Need to re-evaluate hierarchical layout.

• (Christian - 2) RealmInterop is too huge and annoying to reason about. I would like to split it into more manageable chunks that mirror the ObjectStore classes: Realm, Results, List, Object, or something along those lines.

• (Claus -2) Type safety of NativePointer. Maybe by generic marker type (NativePointer<Realm>, NativePointer<RealmObject>, etc.). Maybe it's better to do actual specific typed instance like the realm-java OS<X>s, so requires some thought/experiments.

  • Should investigate this as part of the above.

• (Claus - 2) Interface for objects holding a native pointer. For close() on all managed resources, and in general common operations like realm_release, realm_equals, etc.

  • Consider as part of the above

• (Christian - 1) Add Benchmarking setup. The first step should be to set up benchmarks for Android (since that is our slowest platform), then later for iOS.

  • Keep in mind that it should eventually work in CI context
  • And be able to compare results against previous results.

• (Claus - 1) Bundle all managed properties into one composite object. Initial thought is to substitute RealmObject's inheritance from RealmObjectInteral to a composition of an managed internal property. It will significantly reduce the amount of code in our compiler plugin, help us enforce invariants and improve type-safety in our internal code. Could probably also do something similar with RealmObjectCompanion to keep public visible things at a minimum.

• (Claus -1 ) Clean up construction of managed/linked objects. Should probably go hand-in-hand with moving RealmObjectInternal into a property of RealmObject instead of the current inheritance chain.

  • Will probably be fixed as part of the above
  • Claus will add a link to the problem.

• (Claus - 2) Make a single code path for value transformations going in and out of C-interop layer. We currently have multiple places where C-API values are translated between C-interop types and user facing types. Most notably the various differentiated methods in RealmObjectHelper and ElementConverters in RealmListInternal.kt, but the need will increase with support for sets, mixed and dictionaries.

  • Required as part of a public Type converter API.
  • Do it after adding composite internal object.

• (Claus - 2) Clean up internal Realm-hierarchy. MutableRealm was originally constructed as the user-facing writable realm, but with introduction of DynamicMutableRealm we have some common stuff across a the mutable realm and some parts of MutableRealm are not on all the actual mutable realms ... so maybe time to revisit. I tried to pull out some of the functionality into internal interfaces with default implementations in #615, but there are still things to do (ex. issues highlighted in #723)

Feel free to edit or clarify if I'm missing something

[rorbech]

(Christian - 1-3?) We need to figure out how to split cinterop so it supports both base and sync builds. Instead of right now where sync native code is getting shipped with the base library. The networking refactor in core, should in theory remove the need to depend on us shipping SSL, but it is unclear what their timeline is.

• Claus has some ideas on this.

According to my initial investigations (more that a year ago 🙈) it should be possible to achieve a concept like build variants through custom compilations: https://kotlinlang.org/docs/multiplatform-configure-compilations.html

With the current setup (different modules for library-base and library-sync) it should also be possible to configure that library-base-dependency of library-sync to override the cinterop-dependency to such a new cinterop-sync variant.

Maybe worth to re-investigate if there is a formal KMP-variant concept on its way.

[rorbech]

(Claus -1 ) Clean up construction of managed/linked objects. Should probably go hand-in-hand with moving RealmObjectInternal into a property of RealmObject instead of the current inheritance chain.

• Will probably be fixed as part of the above
• Claus will add a link to the problem.

The motivation for mentioning this is that we have quite many different callsite to the RealmObjectUtil.kt methods manage and link. All callsites constructs an unmanaged object and manage/link it afterwards. We should just group creation of the Kotlin class and linking into a single method to ensure consistency. I assume that this will be an obvious thing to do as part of moving RealmObjectInternal out of the class hierarchy of RealmObject as that would be touching the same codepaths.

Optimization: Combined with the potential rework of RealmObjectInternal we could let the internal reference to the native shared object be a lazy resolved property, as we technically don't need to resolve and maintain the native pointer until we need to access the object pointed to by the link (think this was where the differentiation of the methods actually came from).

[rorbech]

• (Claus - 1) Bundle all managed properties into one composite object. Initial thought is to substitute RealmObject's inheritance from RealmObjectInteral to a composition of an managed internal property. It will significantly reduce the amount of code in our compiler plugin, help us enforce invariants and improve type-safety in our internal code. Could probably also do something similar with RealmObjectCompanion to keep public visible things at a minimum.

As mentioned in the meeting, I am not sure that the above is without complications. I would imagine the major obstacle being that it is no longer possible to return the RealmObjectInternal as an RealmObject or grab the actual instance type from ::class. So we should find and identify places where that is required early in the process and verify it that is a showstopper for the refactor. I guess we could just hold a KClass<out RealmObject> and a parent: out RealmObject in the new ManagedRealmObject to overcome that, but maybe that clutters the gain/perspective of the refactor itself.