-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Introduce _customRealmProperties()
to all Realm Objects to allow for client-generated RLMProperties (via Swift Macros)
#8490
Introduce _customRealmProperties()
to all Realm Objects to allow for client-generated RLMProperties (via Swift Macros)
#8490
Conversation
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Robert J Chatfield.
|
Hey, thanks for this. We'll have a meeting on Monday to go over it - the initial feedback from the team is that the approach seems reasonable. A few things would be needed before we can accept this though:
|
Thanks for getting back to me.
|
Realm/RLMObjectBase.h
Outdated
@@ -35,6 +36,9 @@ RLM_HEADER_AUDIT_BEGIN(nullability, sendability) | |||
+ (nullable NSString *)_realmObjectName; | |||
+ (nullable NSDictionary<NSString *, NSString *> *)_realmColumnNames; | |||
|
|||
/// Allow client code to generate properties (ie. via Swift Macros) | |||
+ (nullable NSArray<RLMProperty *> *)_customRealmProperties; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll see here I've added a static method directly to the base class here. In the original code by @tgoyne, he defined a Swift Protocol and then added conformance via the Macro. I couldn't work with that solution for two main reasons:
- Supporting Object subclassing was really tricky. There is no way to know from an Extension Macro if the class subclasses from a type that already conforms to this protocol - it just sees the exact syntax of this local type. So the Macro would naively add the conformance, then generate a compile time error that the type already conforms.
- The purpose of this Macro is to improve runtime performance. In profiling my own use case, I noticed that adding this casting of the generic Base class to this Swift protocol type was not free. It added ~100ms to the schema generation. Which is almost equivalent to the time taken to generate all the actual properties for all my Objects. By defining the method on all Base Objects avoided this cast and that cost.
self.isPrimary = primaryKey | ||
self.linkOriginPropertyName = originProperty | ||
V._rlmPopulateProperty(self) | ||
V._rlmSetAccessor(self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note: This line is quite slow, particularly for the List Accessor which is generic over the list's Element. There is a runtime cast of the List Accessor to AnyObject?
that takes a long time for the Swift runtime to lookup class conformance. I am no expert in this kind of optimisation, but I believe it's because the class type is defined in your module and the generic argument is defined in my module, and Swift has no way to reason about and inline it at compile time... so it just does it at runtime.
Fixing this somehow is one of the one last bottlenecks for an insanely fast schema initialisation. I'm happy to make a separate GitHub issue for that if desired.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be one of the things covered by the dyld cache on iOS, which means that it'll only be slow the first time an app is launched after each installation. If not, it is just frontloading work that'll need to happen at some point. Theoretically it could be done lazily, but right now we take advantage of that the schema is immutable after construction to share it between threads without any locking, and introducing locking could be a meaningful perf hit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this approach is fine. I had hoped that overriding the existing sharedSchema
class method would work, but it really wouldn't without significant changes.
|
||
/// Exposed for Macros. | ||
/// Important: Keep args in same order & default value as `@Persisted` property wrapper | ||
public convenience init<O: ObjectBase, V: _Persistable>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not actually verified that this works, but if it does this should be @_spi(RealmSwiftPrivate)
so that code using it has to do @_spi(RealmSwiftPrivate) import RealmSwift
to reflect that this isn't a stable part of the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love this. In fact, I've pushed another commit to remove _customRealmProperties
from the Objective-C files and done it as an extension in RealmSwift and annotated it with this @_spi
. Since we only want to expose it to Swift Macros, makes sense yeah?
What do you think?
- [RLMObjectBase] Add _customRealmProperties() - [RealmSwift] Check _customRealmProperties() from within getProperties() - [RLMProperty] Expose convenience init - Add tests
3aa82fa
to
36ffd79
Compare
Updates:
|
2f622a8
to
4f9a795
Compare
Goals
Allow client code to generate RLMProperties for Objects.
Background
Late last year I started investigating performance problems in our app. Specifically, App Launch was taking ~700ms to perform the first read from a Realm: without migration, without Realm Sync. I isolated it to the creation of
RLMSchema.shared()
. I then attempted to use Swift Macros to try and generate the schema during compile time, but I had no luck.Reading the GitHub issues and discussions for "macros", I found these:
@tgoyne was kind enough to reply and shared his experimental work from back in July to do exactly what I wanted. His branch also included some explorations into something similar to Apple's
@Model
, which I'm not interested in at this point. But it was a great jumping off point.This PR
This PR introduces the minimum set of changes required for the Swift Macro to work.
_customRealmProperties()
getProperties()
@Persisted
property wrapperMore about the Macro
You can find the macro here: https://github.com/rjchatfield/realm-swift-macro
I'm pretty excited about this. I'm new to macros, and used a lot of what @tgoyne built last year. I stripped it down to just generating
class func _customRealmProperties() -> [RLMProperties]
. I tested this out in my own codebase and saw promising improvements.Here is an example:
The macro's output:
I then used this in my own codebase. I annotated the 467x Objects & EmbeddedObjects with
@CompileTimeSchema
(in 322 files). There is a bug in Swift which stopped me using it in 7 places, but that is already fixed in the next version of Swift.The performance is great.
RLMSchema.shared()
This Macro SPM package depends on nothing else by Swift Syntax (which may one day be exposed through the Swift Toolchain and avoid re-compilation). I separated out all the tests into a separate module to avoid some SPM resolution issues I had in my codebase. https://github.com/rjchatfield/realm-swift-macro-tests/tree/main
Requested stats
Thanks for reading
This is my first PR into Realm. I'm open to any and all feedback at this point. I also understand this change may not be wanted by Realm/Mongo, and I accept what ever you want to do with this PR. Thanks. :)