IRGen: Don't eagerly initialize NSCoding conformers on startup on new deployment targets #23413

slavapestov · 2019-03-19T04:06:57Z

If objc_getClass() can realize arbitrary Swift classes there's no reason
to generate the static constructor.

Fixes rdar://problem/49660515.

slavapestov · 2019-03-19T04:07:08Z

@jrose-apple Before I update the tests and so on -- does this make sense to you?

jrose-apple · 2019-03-19T14:24:24Z

Eagerly registering a class also runs any +initialize it may have, so I'd prefer if you put the check in Sema where the attribute gets added instead. That said, we currently ban writing +initialize implementations in Swift, and don't currently synthesize any, so this will have the same effect in practice today.

slavapestov · 2019-03-19T19:33:57Z

@jrose-apple I understand where you're coming from but as far as I can tell the only effect of the attribute is to add the class to the vector in IRGen. IRGen emits a static constructor, which calls the metadata accessor for each eager class which I assume triggers the call to +initialize. So I don't think my change would be any different than not adding the attribute in Sema in the first place. Am I missing something? Regardless I can do it in Sema too, I was mostly just being lazy because I already have the right deployment check factored out in IRGen.

slavapestov · 2019-03-19T19:39:55Z

@jrose-apple I went and looked and it turns out we have two attributes:

StaticInitializeObjCMetadataAttr is the one I'm changing in this PR, and it only determines if we emit the global constructor or not.
ObjCNonLazyRealizationAttr this one is different. It can only be used for classes with fixed metadata (no generic ancestry) and as far as I see it only exists so that the empty array and empty dictionary singletons can be statically allocated with valid isa pointers in the stdlib. This is probably the one you're thinking of since it triggers Objective-C realization on startup without an opportunity to run Swift metadata accessors.

jrose-apple · 2019-03-19T21:04:27Z

I was thinking that we may have a use for the StaticInitializeObjCMetadataAttr case that isn't just "make sure it can be found by NSClassFromString". We don't today, but the name of the attribute implies that we might. Maybe renaming the attribute would be another way to go?

jrose-apple · 2019-03-19T21:05:19Z

The two attributes have basically the same effect for a class with fixed metadata; it's just that the latter is something the Objective-C runtime handles natively and the former uses a static constructor.

slavapestov · 2019-03-19T22:46:15Z

@jrose-apple Right. We don't actually ever emit StaticInitializeObjCMetadataAddr on classes without generic ancestry, since those could be found by name on the old runtime anyway. I'm not proposing renaming or removing the attribute, just not adding it on classes conforming to NSCoding when your deployment target is sufficiently new.

jrose-apple · 2019-03-19T22:48:59Z

I'm in favor of "don't add the attribute on classes conforming to NSCoding", but this patch doesn't do that; it ignores the attribute in IRGen instead, no matter why it was added.

slavapestov · 2019-03-20T00:36:41Z

@jrose-apple Ok, glad we're on the same page then.

slavapestov · 2019-03-27T00:45:11Z

@swift-ci Please smoke test

jrose-apple · 2019-03-27T00:46:46Z

include/swift/Basic/LangOptions.h

+    // - iOS 12.2
+    // - tvOS 12.2
+    // - watchOS 5.2
+    bool doesPlatformSupportObjCGetClassHook() const;


Nitpick: these should be "target" or "deployment target" or "minimum deployment target", not "platform". "Platform" doesn't include a version number.

jrose-apple · 2019-03-27T00:48:24Z

lib/Basic/LangOptions.cpp

+  if (Target.isWatchOS())
+    return !Target.isOSVersionLT(5, 2);
+
+  return false;


Can you leave a comment noting that this allows us to run ObjC tests on non-Apple platforms? That's the main reason to leave it this way instead of asserting, I think.

jrose-apple · 2019-03-27T00:49:13Z

lib/Sema/TypeCheckProtocol.cpp

@@ -5039,7 +5043,8 @@ void TypeChecker::checkConformancesInContext(DeclContext *dc,
    if (auto classDecl = dc->getSelfClassDecl()) {
      if (Context.LangOpts.EnableObjCInterop &&
          isNSCoding(conformance->getProtocol()) &&
-          !classDecl->isGenericContext()) {
+          !classDecl->isGenericContext() &&
+          !classDecl->hasClangNode()) {


When does this come up?

Retroactive conformance of imported class to NSCoding. We already checked for hasClangNode() before, but I moved it.

*shrug* All right. It's true that those won't have unstable names either.

jrose-apple · 2019-03-27T00:56:48Z

test/Interpreter/SDK/archive_attributes.swift

@@ -1,7 +1,6 @@
 // RUN: %empty-directory(%t)
 // RUN: %target-build-swift %s -module-name=test -DENCODE -o %t/encode
 // RUN: %target-build-swift %s -module-name=test -o %t/decode
-// RUN: %target-build-swift %s -module-name=test -Xfrontend -disable-llvm-optzns -emit-ir | %FileCheck -check-prefix=CHECK-IR %s
 // RUN: %target-run %t/encode %t/test.arc
 // RUN: plutil -p %t/test.arc | %FileCheck -check-prefix=CHECK-ARCHIVE %s
 // RUN: %target-run %t/decode %t/test.arc | %FileCheck %s


This is an execution test; would it make sense to do it both the old way and the new way if we're on a new enough OS?

(I'm not even 100% sure how to write such a test, but…)

I can build the test with an old and new deployment target, and in the new deployment target case, add a runtime check that skips the test.

slavapestov · 2019-05-01T19:56:04Z

@swift-ci Please smoke test

slavapestov · 2019-05-01T20:16:21Z

@swift-ci Please test source compatibility

@objc

…gets If a class does not have a custom @objc name, objc_getClass() can find it at runtime by calling the Swift runtime's metadata demangler hook. This avoids the static initializer on startup. If the class has a custom runtime name we still need the static initializer unfortunately. Fixes <rdar://problem/49660515>.

slavapestov · 2019-05-01T21:44:53Z

@swift-ci Please smoke test

slavapestov · 2019-05-01T21:45:04Z

@swift-ci Please smoke test

slavapestov · 2019-05-02T00:03:58Z

@swift-ci Please smoke test macOS

slavapestov requested a review from jrose-apple March 19, 2019 04:07

slavapestov force-pushed the phase-out-nscoding-hack-deployment-target branch from 12732a3 to 1a54266 Compare March 27, 2019 00:44

jrose-apple reviewed Mar 27, 2019

View reviewed changes

IRGen: Move some deployment target checks to LangOptions

3b6ec6c

slavapestov force-pushed the phase-out-nscoding-hack-deployment-target branch from 1a54266 to a1363ab Compare May 1, 2019 19:14

slavapestov force-pushed the phase-out-nscoding-hack-deployment-target branch from a1363ab to 6d7d13f Compare May 1, 2019 21:44

slavapestov merged commit 20dd808 into swiftlang:master May 2, 2019

IRGen: Don't eagerly initialize NSCoding conformers on startup on new deployment targets #23413

IRGen: Don't eagerly initialize NSCoding conformers on startup on new deployment targets #23413

Uh oh!

Conversation

slavapestov commented Mar 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slavapestov commented Mar 19, 2019

Uh oh!

jrose-apple commented Mar 19, 2019

Uh oh!

slavapestov commented Mar 19, 2019

Uh oh!

slavapestov commented Mar 19, 2019

Uh oh!

jrose-apple commented Mar 19, 2019

Uh oh!

jrose-apple commented Mar 19, 2019

Uh oh!

slavapestov commented Mar 19, 2019

Uh oh!

jrose-apple commented Mar 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slavapestov commented Mar 20, 2019

Uh oh!

slavapestov commented Mar 27, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slavapestov commented May 1, 2019

Uh oh!

slavapestov commented May 1, 2019

Uh oh!

slavapestov commented May 1, 2019

Uh oh!

slavapestov commented May 1, 2019

Uh oh!

slavapestov commented May 2, 2019

Uh oh!

Uh oh!

slavapestov commented Mar 19, 2019 •

edited

Loading

jrose-apple commented Mar 19, 2019 •

edited

Loading