Skip to content

[Distributed] deinit aware of remote "properties" (lack thereof in storage) #38870

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

Merged
merged 2 commits into from
Aug 16, 2021

Conversation

ktoso
Copy link
Contributor

@ktoso ktoso commented Aug 13, 2021

Class deinit, iff the class is a distributed actor must become aware that not all properties actually "really exist".

Only @_distributedActorIndependent properties are actually stored and may be released.
The other properties have never been initialized, and thus we must not destroy them.

A distributed deinit must:

  • resignIdentity(self) we already do this earlier in SILGenDestructor, by calling emitDistributedActor_resignAddress(dd, selfValue, continueBB);
  • it must check if it is remote, and if yes, it must NOT touch any properties in destruction since they are not "really there"
  • if it is local, continue with the usual deinit

  • I will refactor this more so it lives in SILGenDistributed and does not make the SILGenDestructor so ugly.
  • clean up more of the warnings in SILGen

Review and hints how to better do this SIL very welcome! The good news is that non-distributed actors are of course not impacted by any change here.

@ktoso
Copy link
Contributor Author

ktoso commented Aug 13, 2021

Note that today the memory actually exists for the actor, but it is zeroed out:

  HeapObject *alloc = swift_allocObject(classMetadata,
                                        classMetadata->getInstanceSize(),
                                        classMetadata->getInstanceAlignMask());

  // TODO: remove this memset eventually, today we only do this to not have
  //       to modify the destructor logic, as releasing zeroes is no-op
  memset(alloc + 1, 0, classMetadata->getInstanceSize() - sizeof(HeapObject));

we will remove that memset and any access to those "local only" properties would be reading/writing random memory, this is why this change in the destructor.

@ktoso
Copy link
Contributor Author

ktoso commented Aug 13, 2021

@swift-ci please smoke test

@ktoso ktoso added the distributed Feature → concurrency: distributed actor label Aug 13, 2021
@ktoso
Copy link
Contributor Author

ktoso commented Aug 16, 2021

Heh silly failure because lack of [available 12.0] in SIL on linux

@ktoso
Copy link
Contributor Author

ktoso commented Aug 16, 2021

@swift-ci please smoke test and merge

@ktoso
Copy link
Contributor Author

ktoso commented Aug 16, 2021

@swift-ci please smoke test

@swift-ci swift-ci merged commit c035022 into swiftlang:main Aug 16, 2021
@ktoso ktoso deleted the wip-dist-destructor-sil branch August 16, 2021 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
distributed Feature → concurrency: distributed actor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants