In-place class metadata initialization #18694

slavapestov · 2018-08-14T04:31:24Z

Up until Swift 4.2, classes with generic ancestry and classes with resilient fields used a metadata initialization scheme where the metadata was emitted statically and initialized at runtime with the right fields (a dynamic superclass in the generic case, dynamic field offsets in the resilient case). I removed this form of initialization while implementing the fully general case of resilient ancestry, where the runtime size of metadata is not known at compile time.

This PR brings back the intermediate in-place initialization style. The main benefit is that we can now define Objective-C categories on classes that use it. It will also allow subclasses of NSObject with resilient-sized fields to be exposed to Clang.

The latter half of this PR mostly restructures metadata initialization for non-generic classes -- both the in-place and resilient case -- to use the new two-phase initialization model, allowing recursive metadata cycles. Note that here, the metadata initialization code uses "in-place" to mean something different -- it means the type descriptor is globally unique, and there is only ever one instance of metadata for that type, but not necessarily that this metadata itself is emitted statically. This is confusing and I plan on renaming this to "singleton metadata" and similar throughout.

slavapestov · 2018-08-14T10:13:28Z

@rjmccall This is still a work in progress but it's coming together. I think I got the new in-place classes working with two-phase initialization also.

I'm still trying to figure out how to port classes with fully resilient ancestry to the two-phase scheme.

For the in-place case I cargo-culted InPlaceValueMetadataInitialization and made it work with classes as well. For resilient classes, should I add a new ResilientClassMetadataInitialization that will do something special somehow?

Once that is done, I can remove emitOnceTypeMetadataAccessFunctionBody(), etc.

rjmccall · 2018-08-14T19:09:31Z

I've been thinking of this as a special case of "in-place" initialization that happens to need allocation — maybe that means "in-place" is the wrong word and it should really be "singleton" or something. Anyway, I'd prefer if this didn't need a new MetadataInitializationKind because we're already using 3 of 4 values for that and the last should really be future-proofed.

rjmccall · 2018-08-14T19:14:28Z

lib/IRGen/GenClass.cpp

+
+bool irgen::doesClassMetadataRequireInitialization(IRGenModule &IGM,
+                                                   ClassDecl *theClass) {
+  // Classes imported from Objective-C never requires dynamic initialization.


This comment (like the place you copied it from 😄) has a grammar error.

slavapestov · 2018-08-15T19:33:28Z

@swift-ci Please test

slavapestov · 2018-08-15T19:33:46Z

@swift-ci Please test source compatibility

slavapestov · 2018-08-15T19:34:18Z

@swift-ci Please test

slavapestov · 2018-08-15T19:34:40Z

@swift-ci Please test source compatibility

rjmccall · 2018-08-15T20:27:01Z

include/swift/ABI/Metadata.h

+  /// The completion function.  The pattern will always be null.
+  TargetRelativeDirectPointer<Runtime, MetadataCompleter>
+    CompletionFunction;
+};


If you're going to use these for classes, maybe Value should be dropped from the class name.

Should the link to the resilient class pattern just be unioned with IncompleteMetadata?

Unioning the resilient pattern with IncompleteMetadata sounds like a good idea. I'll do that.

I also plan on two follow-up cleanups here:

renaming 'InPlaceValueMetadata' and similar to 'SingletonMetadata' throughout.

switching non-generic resilient classes to start from a true-const pattern and not a partial class metadata.

Is it OK if I do the second one in a follow-up PR?

Sure. Do you consider this PR ready for a more complete review?

Yes, except I'm redoing the IncompleteMetadata field part.

swift-ci · 2018-08-15T20:33:54Z

Build failed
Swift Test Linux Platform
Git Sha - 49ba0882371f846c1b6a39b918e542014a4dfa6b

slavapestov · 2018-08-15T21:10:34Z

@jrose-apple You might be interested in the behavior change with categories, and the fact that there are now two kinds of resilient classes.

jrose-apple · 2018-08-15T21:06:22Z

lib/IRGen/GenClass.cpp

      return (ClassHasMissingMembers ||
              ClassHasResilientMembers ||
              ClassHasResilientAncestry ||
              ClassHasGenericAncestry);
    }

+    bool doesMetadataRequireRelocation() const {
+      return ClassHasResilientAncestry;


Your commit comment implies that the class itself being generic would affect this too. Does the question just not get asked in that case?

Generic classes go down a totally separate code path. Note that the top-level irgen::doesClassMetadataRequireRelocation() method does return true for generic classes. I can add a comment here.

jrose-apple · 2018-08-15T21:17:29Z

test/Interpreter/objc_extensions.swift

+let y = Derived(t: 100)
+
+// CHECK: 100
+print(y.dynamicMethod())


I'm not sure I trust this test to invoke the method dynamically. Can you add a respondsToSelector check?

// CHECK: respondsToSelector? true print("respondsToSelector? \(y.responds(to: #selector(Derived.dynamicMethod)))")

Well, Derived does not descend from NSObject, which was intentional (remember when we had the resilient class hack in 4.2, and I only did it for @objc classes, and it broke categories).

However, using AnyObject dispatch also clearly demonstrates that its dynamically dispatched, so I'll do that instead.

And FWIW, we infer dynamic on @objc members of extensions, because you're supposed to be able to override them in subclasses.

Yeah, I know it is actually testing all that, but I'd prefer it be a little more direct. AnyObject dispatch sounds good.

swift-ci · 2018-08-15T21:47:20Z

Build failed
Swift Test OS X Platform
Git Sha - 49ba0882371f846c1b6a39b918e542014a4dfa6b

slavapestov · 2018-08-15T22:55:49Z

@swift-ci Please test

slavapestov · 2018-08-15T22:55:55Z

@swift-ci Please test

slavapestov · 2018-08-15T22:56:02Z

@swift-ci Please test

swift-ci · 2018-08-15T22:58:15Z

Build failed
Swift Test Linux Platform
Git Sha - 49ba0882371f846c1b6a39b918e542014a4dfa6b

swift-ci · 2018-08-15T22:58:31Z

Build failed
Swift Test OS X Platform
Git Sha - 49ba0882371f846c1b6a39b918e542014a4dfa6b

rjmccall · 2018-08-15T23:19:28Z

include/swift/ABI/Metadata.h

@@ -3345,8 +3408,11 @@ class TargetTypeContextDescriptor
  const TargetForeignMetadataInitialization<Runtime> &
  getForeignMetadataInitialization() const;

+  const TargetInPlaceValueMetadataInitialization<Runtime> &
+  getInPlaceMetadataInitialization() const;


The reason I didn't have this here (and called it InPlace*Value*MetadataInitialization in the first place) is that I was anticipating that resilient classes might need more members, like a reference to a pattern. So before you unify them, you might want to consider whether you're just making more work for yourself to split them again in a follow-up patch. (It'd be better if value metadata didn't have to pay for an always-null reference to a pattern.)

I think we can put the pattern in the class context descriptor, and use a trailing object for it so it’s only there for resilient classes.

... or just say that IncompleteMetadata is either a metadata or a pattern (instead of metadata or function). That was sort of my plan here. I wasn’t going to add an always-null field.

rjmccall · 2018-08-15T23:23:15Z

include/swift/ABI/MetadataValues.h

+
+  /// If true, don't initialize the vtable, assuming it has been statically
+  /// emitted already.
+  HasStaticVTable   = 0x100,


This comment should be more descriptive than imperative. I assume this means that the v-table is guaranteed to have been initialized in the statically-emitted metadata?

rjmccall · 2018-08-15T23:36:38Z

lib/IRGen/GenDecl.cpp

+    fnType = llvm::FunctionType::get(TypeMetadataPtrTy, argTys,
+                                     /*isVarArg*/ false);
+  }
+


Maybe these should be commented with the name of the corresponding typedef in the runtime.

rjmccall · 2018-08-15T23:59:51Z

lib/IRGen/GenMeta.cpp

@@ -1226,6 +1206,19 @@ namespace {
      addVTable();
    }

+    void addIncompleteMetadataOrRelocationFunction() {


In a world in which InPlaceClassMetadataInitialization requires more fields than InPlaceValueMetadataInitialization, I think a cleaner solution might be to have ClassMetadataBuilder provide its own, independent definition of addInPlaceMetadataInitialization which contains this logic.

Should we actually add more fields to it, or have it point to a separate pattern structure in place of the incomplete metadata?

Good question. The only reason we wouldn't want to just replace the IncompleteMetadata with a pattern reference is if there's a situation where we'd want an allocation function but not a pattern, and that just seems really unlikely to me. The reverse scenario — where we want a patten but not an allocation function, e.g. as a load-time optimization — is more plausible but doesn't need to be specially optimized because it's easy to fit a metadata pointer into the pattern — we can just union the allocation function in the pattern with a constant metadata, discriminated by some flag in the pattern (which needs flags anyway).

That does make me wonder, though, whether having a resilient superclass is the right condition to flag the presence of a pattern. Is there any other reasonable way to store this?

We could just add another flag to the type context descriptor flags.

Yeah. Or maybe make IncompleteMetadata a pointer union, since they're both pointers to structures now instead of one side being a pointer to code.

rjmccall · 2018-08-16T00:02:20Z

lib/IRGen/GenMeta.cpp

+      auto type = Type->getDeclaredTypeInContext()->getCanonicalType();
+      auto metadata = IGM.getAddrOfTypeMetadata(type);
+      B.addRelativeAddress(metadata);
+    }


I feel like this function ought to be called addIncompleteMetadata. You can still have an addIncompleteMetadataOrRelocationFunction that just calls it if that's the right design. But calling this the right thing will make the super-call to it much clearer.

Sounds good

rjmccall · 2018-08-16T00:08:03Z

stdlib/public/runtime/Metadata.cpp

+    fullPattern -= pattern->getClassAddressPoint();
+    memcpy(bytes, fullPattern, patternSize);
+    memset(bytes + patternSize, 0,
+           metadataSize - patternSize);


If we're calling swift_initClassMetadata properly on a relocated class now, I'm not sure how necessary this copy is. But I guess you were planning on reworking all this in a follow-up?

Yes, I’m going to rework it. There’s actually very little useful data that gets copied here.

…two predicates - doesClassMetadataRequireRelocation() -- returns true if we must allocate new metadata at runtime and fill it in, because the class has multiple instantiations (generic case) or because the total size of the metadata is not known at compile time (resilient ancestry). - doesClassMetadataRequireInitialization() -- weaker condition than the above. It's true if the metadata must be relocated, but it is also true if the metadata has otherwise fixed size but must be filled in dynamically. This occurs if the class has generic ancestry but is itself not generic, or if the class has resiliently-sized fields, or missing members. For now, we don't actually care about the distinciton anywhere, because we cannot do in-place initialization of class metadata yet.

…resiliently-sized fields If a class has generic ancestry or resiliently-sized fields, but is itself not generic and does not have resilient ancestry, we must perform runtime metadata initialization, but we can initialize the metadata in-place. As with generic classes or classes with resilient ancestry, we copy generic requirements and field offset vectors from the superclass. We also calculate the layout of the class's direct fields at runtime. Unlike the fully resilient case, we don't copy vtable entries from the superclass, or install the direct class's vtable entries from the type context descriptor. Instead, we statically emit the vtable as with fixed-size class metadata. Both the in-place and resilient case call the same runtime entry point to initialize class metadata; the new HasStaticVTable flag in ClassLayoutFlags is used to select between the two behaviors concerning the vtable.

…generic classes In-place initialization means the class has a symbol we can reference from the category, so there's nothing to do on the IRGen side. For JIT mode, we just need to realize the class metadata by calling an accessor instead of directly referencing the symbol though.

…tent" class initialization This is the simplest case, and not very interesting.

…asses Note that this patch also consolidates the recursive metadata tests into one place while adding an execution test.

…m the class descriptor Using the superclass metadata here no longer makes sense with two-phase init, in case the superclass metadata depends on the class being instantiated. It would also be nice to rework the resilient class metadata 'pattern' to be its own data structure that's true const, instead of just the prefix of a real class metadata, but for now let's keep the existing crappy design.

Now that we don't need the superclass before calling swift_relocateClassMetadata(), it seems simpler to set it here instead of doing it in various places in IRGen.

Similar to the non-resilient case, except we also emit a 'relocation function'. The class descriptor now contains this relocation function if the class has resilient ancestry, and the relocation function calls the runtime's swift_relocateClassMetadata() entry point. The metadata completion function calls swift_initClassMetadata() and does layout, just like the non-resilient case. Fixes <rdar://problem/40810002>.

slavapestov · 2018-08-20T23:35:51Z

@swift-ci Please test

slavapestov · 2018-08-20T23:35:56Z

@swift-ci Please test source compatibility

slavapestov force-pushed the in-place-class-metadata-init branch 2 times, most recently from 6879294 to 153db56 Compare August 14, 2018 10:10

slavapestov requested a review from rjmccall August 14, 2018 10:13

rjmccall reviewed Aug 14, 2018

View reviewed changes

slavapestov force-pushed the in-place-class-metadata-init branch 3 times, most recently from cc8b52e to 49ba088 Compare August 15, 2018 12:48

rjmccall reviewed Aug 15, 2018

View reviewed changes

slavapestov changed the title ~~In-place class metadata initialization [WIP]~~ In-place class metadata initialization Aug 15, 2018

slavapestov requested a review from jrose-apple August 15, 2018 20:59

jrose-apple reviewed Aug 15, 2018

View reviewed changes

slavapestov force-pushed the in-place-class-metadata-init branch from 49ba088 to 17e514e Compare August 15, 2018 22:54

rjmccall reviewed Aug 16, 2018

View reviewed changes

slavapestov force-pushed the in-place-class-metadata-init branch from 17e514e to 2f80c78 Compare August 20, 2018 21:32

slavapestov added 8 commits August 20, 2018 16:23

IRGen: Teach emitDirectTypeMetadataAccessFunctionBody() about "idempo…

1fbdf03

…tent" class initialization This is the simplest case, and not very interesting.

Runtime/IRGen: Two-phase metadata initialization for non-resilient cl…

a86a71d

…asses Note that this patch also consolidates the recursive metadata tests into one place while adding an execution test.

Runtime: Set the superclass in swift_initClassMetadata()

50a037d

Now that we don't need the superclass before calling swift_relocateClassMetadata(), it seems simpler to set it here instead of doing it in various places in IRGen.

RelativePointer::operator() should be const

c71f1e4

SILGen: Remove unused variable

1eed99d

slavapestov force-pushed the in-place-class-metadata-init branch from 2f80c78 to 1eed99d Compare August 20, 2018 23:35

slavapestov merged commit d245831 into swiftlang:master Aug 21, 2018

belkadan mentioned this pull request Nov 29, 2018

[SR-7876] Runtime crash on 'type metadata accessor for %Typename%' #50411

Closed

In-place class metadata initialization #18694

In-place class metadata initialization #18694

Conversation

slavapestov commented Aug 14, 2018 • edited Loading

slavapestov commented Aug 14, 2018

rjmccall commented Aug 14, 2018

Choose a reason for hiding this comment

slavapestov commented Aug 15, 2018

slavapestov commented Aug 15, 2018

slavapestov commented Aug 15, 2018

slavapestov commented Aug 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swift-ci commented Aug 15, 2018

slavapestov commented Aug 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slavapestov Aug 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swift-ci commented Aug 15, 2018

slavapestov commented Aug 15, 2018

slavapestov commented Aug 15, 2018

slavapestov commented Aug 15, 2018

swift-ci commented Aug 15, 2018

swift-ci commented Aug 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slavapestov commented Aug 20, 2018

slavapestov commented Aug 20, 2018

slavapestov commented Aug 14, 2018 •

edited

Loading

slavapestov Aug 15, 2018 •

edited

Loading