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

Use Finalizable to ensure lexical liveness #754

Merged
merged 7 commits into from
Aug 16, 2022

Conversation

nielsenko
Copy link
Contributor

@nielsenko nielsenko commented Aug 11, 2022

We need to use the new Finalizable marker interface to ensure that native handles survive long enough. However, since Finalizables don't keep member finalizables alive, we also need to add a bunch of pseudo keepAlive extension methods, that are not exposed to the user, and only exists to teach the compiler about the lifetime dependencies of the various Finalizables.

Resolves: #581, resolves: #582

@cla-bot cla-bot bot added the cla: yes label Aug 11, 2022
@nielsenko nielsenko self-assigned this Aug 11, 2022
@nielsenko nielsenko force-pushed the kn/use-dart-2.17-finalizable-interface branch 4 times, most recently from 16abbf2 to 72ab64e Compare August 15, 2022 10:22
@nielsenko nielsenko marked this pull request as ready for review August 15, 2022 10:30
@nielsenko nielsenko force-pushed the kn/use-dart-2.17-finalizable-interface branch 2 times, most recently from 26769d1 to 5938b56 Compare August 15, 2022 11:29
@nirinchev
Copy link
Member

This looks reasonable, but still seems kind of hacky. I'm worried that a linker static analysis can discover those methods are unused and strip them from the compiled application. I'm not sure how much (if any) code stripping the dart linker does, especially when compiling to native, but it'd be interesting to confirm with the dart team this is the idiomatic approach for what we're trying to do.

Copy link
Contributor

@blagoev blagoev left a comment

Choose a reason for hiding this comment

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

Added some suggestions. Also seems containing some debug left overs.

lib/src/native/realm_core.dart Show resolved Hide resolved
lib/src/native/realm_core.dart Show resolved Hide resolved
lib/src/native/realm_core.dart Outdated Show resolved Hide resolved
lib/src/native/realm_core.dart Outdated Show resolved Hide resolved
lib/src/realm_class.dart Outdated Show resolved Hide resolved
lib/src/user.dart Show resolved Hide resolved
@nielsenko
Copy link
Contributor Author

nielsenko commented Aug 15, 2022

This looks reasonable, but still seems kind of hacky. I'm worried that a linker static analysis can discover those methods are unused and strip them from the compiled application. I'm not sure how much (if any) code stripping the dart linker does, especially when compiling to native, but it'd be interesting to confirm with the dart team this is the idiomatic approach for what we're trying to do.

See reply by dartlang developer here: dart-lang/sdk#49643 (comment)

I have actually chosen to make keepAlive an extension method, since (in my tests) it has the same effect, and it allows me to hide the otherwise public keepAlive from the user of our package. Perhaps you can confirm if this just as good, or not?

Yes this is fine, extension methods are basically top level methods with the receiver as first argument, and Finalizable arguments are kept alive.

@nielsenko
Copy link
Contributor Author

BTW, in case it isn't obvious. The need to have these keepAlive extension methods should eventually disappear, once dart-lang/sdk#49643 is resolved.

@nielsenko nielsenko force-pushed the kn/use-dart-2.17-finalizable-interface branch from af44b59 to 973aa18 Compare August 15, 2022 14:54
@nielsenko nielsenko force-pushed the kn/use-dart-2.17-finalizable-interface branch from 3b895fb to 999f547 Compare August 16, 2022 11:26
Classes that implements Finalizable:
* App,
* FlexibleSyncConfiguration,
* HandleBase,
* Realm,
* RealmEntity,
* RealmList,
* RealmResults,
* Subscription,
* SubscriptionSet, and
* User,

The corresponding XInternal extension classes implements a keepAlive
function to ensure finalizable fields always live as long as their
finalizable parent. This is workaround for:
dart-lang/sdk#49643

Resolves: #581, resolves: #582
* RealmObject,
* RealmObjectChanges, and
* RealmCollectionChanges.

Drop from RealmEntity
…n trace. Instead have const flag and depend on tree-shaking
@nielsenko nielsenko force-pushed the kn/use-dart-2.17-finalizable-interface branch from 999f547 to e8b7cc9 Compare August 16, 2022 12:28
@nielsenko nielsenko merged commit 92d151e into master Aug 16, 2022
@nielsenko nielsenko deleted the kn/use-dart-2.17-finalizable-interface branch August 16, 2022 13:30
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Dart 2.17 NativeFinalizer classes Use Dart 2.17 Finalizer marker interface
3 participants