Remove three ObjC metadata fields from non-ObjC runtime #31811

davezarzycki · 2020-05-15T09:35:08Z

Hi @rjmccall – Before I finish this draft pull request, I'd like to verify that:

The non-Apple ABI is still not locked down.
This change is reasonable.

I'll finish fixing all the broken tests if this is the case. Thanks! As a bonus, what other fields are good candidates for removal from the non-ObjC runtime?

[EDIT] – A related followup question for a followup PR: Should the vtable layout be more like C++? I.e. where the positive displacements are vtable slots and the negative displacements are language/runtime metadata? This would allow for better code density.

mikeash

Seems reasonable to me.

mikeash · 2020-05-15T14:10:51Z

include/swift/Remote/MetadataReader.h

    StoredPointer dataPtr;
    if (!Reader->readInteger(RemoteAddress(classAddress +
                               TargetClassMetadata<Runtime>::offsetToData()),
                             &dataPtr))
      return StoredPointer();
+#else
+    StoredPointer dataPtr = SWIFT_CLASS_IS_SWIFT_MASK;


You can just return StoredPointer(); here. You'll end up hitting the if (!dataPtr) path a few lines later anyway.

rjmccall · 2020-05-15T17:15:07Z

Ideally, MetadataReader wouldn't be sensitive to target #if conditions, and instead ObjC interop-ness would be reflected in the Target abstraction. That would e.g. allow tools built one way to debug/analyze programs built the other. To make that work, you'll have to play some template games in Metadata.h, but it shouldn't be too bad.

jckarter · 2020-05-15T17:25:49Z

You are correct that the non-Darwin ABI is still up for grabs. I agree with @rjmccall that it'd be nice if we could make this dependent on the Runtime template argument instead of using conditional compilation.

A good followup for non-Darwin optimization would be to start treating class objects as being directly referenceable as metadata, like struct/enum metadata is, since there's no ObjC realization to worry about. That should save a good amount of code size from metadata accesses, and allow dynamic class metadata instantiation for generic or resilient classes to be more efficient and avoid some synchronization that's necessary on Darwin to coordinate with ObjC realization.

rjmccall · 2020-05-15T17:46:06Z

Yeah. It'll need some new logic in order to check for transitive direct-referenceability of superclasses, since right now the condition is just "no, you can't emit this reference directly", but it should be possible to be much more efficient than we are on ObjC-interop platforms.

compnerd · 2020-05-15T20:47:50Z

lib/IRGen/ClassMetadataVisitor.h

@@ -65,8 +65,10 @@ template <class Impl> class ClassMetadataVisitor
    // FIXME: Figure out what can be removed altogether in non-objc-interop
    // mode and remove it. rdar://problem/18801263


Does this not address this FIXME?

I'm not sure and that's why I asked at the top: "what other fields are good candidates for removal from the non-ObjC runtime?"

I think that @rjmccall and @jckarter got too excited about other things that need fixing, so they missed that question. ;-)

The three extra padding fields seem like the biggest obvious thing to remove. I can't off the top of my head think of any other places where we could wholesale remove fields. I'd be fine removing the fixme.

davezarzycki · 2020-05-16T10:01:15Z

Hi @rjmccall and @jckarter – I'd like to do incremental updates. Would you be okay with focusing in unnecessary field removal in this PR? In particular, the MetadataReader was already using ifdefs, so I'd like to leave fixing that issue to a later PR.

rjmccall · 2020-05-16T17:48:43Z

I'm not sure and that's why I asked at the top: "what other fields are good candidates for removal from the non-ObjC runtime?"

That addresses it for classes, which is the scope of this particular FIXME.

Assuming that we're not interested in highly-disruptive layout changes like putting the VWT at a positive offset, I think most of the other ways that ObjC inserts itself into the runtime representation are just extra alternatives in various metadata references — things like protocol references possibly being to ObjC protocols, or type references possibly being to ObjC classes.

Would you be okay with focusing in unnecessary field removal in this PR? In particular, the MetadataReader was already using ifdefs, so I'd like to leave fixing that issue to a later PR.

I think it's fine to make MetadataReader work cross-ObjC-interop in a different patch. Please base the conditionalization in Metadata.h on the Runtime parameter, though, otherwise it's just not being done right.

jckarter · 2020-05-18T16:00:07Z

If we base the presence of the ObjC compatibility fields on the Runtime template parameter, then the correct MetadataReader behavior ought to fall out automatically.

rjmccall · 2020-05-18T17:48:00Z

For this, at least. Dave is right that there are existing #ifs in that file which ought to be converted to uses of information from Runtime if we're going to support running cross-configuration properly.

davezarzycki · 2020-05-20T10:13:17Z

@swift-ci please test

swift-ci · 2020-05-20T10:16:05Z

Build failed
Swift Test Linux Platform
Git Sha - 0cf24e8750e0db307578f91c6f484a4c3a1df204

swift-ci · 2020-05-20T10:16:09Z

Build failed
Swift Test OS X Platform
Git Sha - 0cf24e8750e0db307578f91c6f484a4c3a1df204

davezarzycki · 2020-05-20T10:20:16Z

@swift-ci please test windows

davezarzycki · 2020-05-20T10:41:29Z

A good followup for non-Darwin optimization would be to start treating class objects as being directly referenceable as metadata, like struct/enum metadata is, since there's no ObjC realization to worry about.

Hi @jckarter – If you point me at a few lines of code to see the difference you're talking about, then I can make an attempt at fixing it.

davezarzycki · 2020-05-20T11:04:57Z

@swift-ci please test macos

davezarzycki · 2020-05-20T11:22:56Z

Hi @compnerd – Does FileCheck not execute RUN directives via a POSIX compliant shell on Windows? I'm trying to use $(( 1 + 2 * 3 )) style shell arithmetic in a RUN line.

swift-ci · 2020-05-20T12:17:37Z

Build failed
Swift Test OS X Platform
Git Sha - 37250e1ca6e14f006861f91d125d348b05a5427a

davezarzycki · 2020-05-20T13:22:55Z

@swift-ci please test

swift-ci · 2020-05-20T13:23:19Z

Build failed
Swift Test Linux Platform
Git Sha - 37250e1ca6e14f006861f91d125d348b05a5427a

swift-ci · 2020-05-20T13:25:37Z

Build failed
Swift Test OS X Platform
Git Sha - 37250e1ca6e14f006861f91d125d348b05a5427a

davezarzycki · 2020-05-21T11:02:27Z

@swift-ci please clean test macos

swift-ci · 2020-05-21T12:56:16Z

Build failed
Swift Test OS X Platform
Git Sha - 21af78586a549cb5134c418308e93e4005d32c7b

davezarzycki · 2020-05-21T13:38:25Z

@swift-ci please test

davezarzycki · 2020-05-21T13:38:41Z

@swift-ci please test windows

swift-ci · 2020-05-21T13:41:13Z

Build failed
Swift Test OS X Platform
Git Sha - 21af78586a549cb5134c418308e93e4005d32c7b

swift-ci · 2020-05-21T13:41:18Z

Build failed
Swift Test Linux Platform
Git Sha - 21af78586a549cb5134c418308e93e4005d32c7b

davezarzycki · 2020-05-21T13:47:18Z

@swift-ci please test windows

davezarzycki · 2020-05-21T16:24:14Z

@swift-ci please test

swift-ci · 2020-05-21T16:27:09Z

Build failed
Swift Test Linux Platform
Git Sha - 4dd6ae4e0607bd5033da74f07745fe8e7ee320fb

swift-ci · 2020-05-21T16:27:30Z

Build failed
Swift Test OS X Platform
Git Sha - 4dd6ae4e0607bd5033da74f07745fe8e7ee320fb

jckarter · 2020-05-21T16:28:52Z

@davezarzycki If you want to look into making class metadata statically addressable, a good place to start looking would be shouldCacheTypeMetadataAccess and shouldTypeMetadataAccessUseAccessor in IRGen. You could also look over the *ClassMetadataBuilder* classes in IRGen and the various swift_init*Class* functions in the runtime to see if there are opportunities for optimizing the initialization process for generic or resilient classes when ObjC realization is not necessary.

compnerd · 2020-05-22T16:22:31Z

@swift-ci please test Windows platform

Adjust the test to accommodate environments which do indirect imports, which use the singleton strategy, relying on the runtime initialization of fields which are imported.

compnerd · 2020-05-22T22:20:43Z

#31981 has the test adjustments needed for this. Could you please grab the top commit and put it into this PR?

davezarzycki · 2020-05-23T09:38:42Z

@swift-ci please test Windows platform

davezarzycki · 2020-05-23T09:39:10Z

@swift-ci please test

swift-ci · 2020-05-23T09:41:54Z

Build failed
Swift Test Linux Platform
Git Sha - 28bdd9831bee7115e7526a92c6cc07794f69db77

swift-ci · 2020-05-23T09:42:01Z

Build failed
Swift Test OS X Platform
Git Sha - 28bdd9831bee7115e7526a92c6cc07794f69db77

davezarzycki · 2020-05-23T09:51:44Z

@swift-ci please test Windows platform

davezarzycki · 2020-05-23T12:22:47Z

@swift-ci please test Windows platform

davezarzycki · 2020-05-23T15:12:11Z

@compnerd The windows failure looks unrelated. Agreed? Ready to merge?

@swift-ci please test Windows platform

compnerd · 2020-05-23T17:05:14Z

Yeap, that looks unrelated to this change. sigh time to go track that down.

davezarzycki requested review from mikeash, gottesmm, rjmccall, eeckstein and jckarter May 15, 2020 09:35

mikeash approved these changes May 15, 2020

View reviewed changes

compnerd reviewed May 15, 2020

View reviewed changes

davezarzycki mentioned this pull request May 16, 2020

[IRGen testing] Fix bogus FileCheck check-prefix "-SAME" usage #31839

Merged

jckarter mentioned this pull request May 18, 2020

[WebAssembly] Use absolute pointers #31676

Closed

davezarzycki force-pushed the pr31811 branch from 0cf24e8 to 37250e1 Compare May 20, 2020 10:13

davezarzycki force-pushed the pr31811 branch from 37250e1 to eb4b41d Compare May 20, 2020 13:22

davezarzycki marked this pull request as ready for review May 20, 2020 13:22

davezarzycki force-pushed the pr31811 branch from 21af785 to 4dd6ae4 Compare May 21, 2020 13:36

davezarzycki force-pushed the pr31811 branch from 4dd6ae4 to 28bdd98 Compare May 21, 2020 16:24

Remove three ObjC fields from non-ObjC runtime

97c89d8

test: account for indirect imports

2c17600

Adjust the test to accommodate environments which do indirect imports, which use the singleton strategy, relying on the runtime initialization of fields which are imported.

davezarzycki force-pushed the pr31811 branch from 28bdd98 to 2c17600 Compare May 23, 2020 09:38

davezarzycki mentioned this pull request May 23, 2020

[DNM] IRGen tests #31981

Merged

compnerd merged commit 435b239 into swiftlang:master May 23, 2020

davezarzycki deleted the pr31811 branch May 23, 2020 20:02

belkadan mentioned this pull request Oct 1, 2020

[SR-6718] Linux Interoperability: Eliminate items in class metadata layout that aren't relevant to non-objc builds #49267

Closed

		@@ -65,8 +65,10 @@ template <class Impl> class ClassMetadataVisitor
		// FIXME: Figure out what can be removed altogether in non-objc-interop
		// mode and remove it. rdar://problem/18801263

Remove three ObjC metadata fields from non-ObjC runtime #31811

Remove three ObjC metadata fields from non-ObjC runtime #31811

Uh oh!

Conversation

davezarzycki commented May 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikeash left a comment

Choose a reason for hiding this comment

Uh oh!

mikeash May 15, 2020

Choose a reason for hiding this comment

Uh oh!

rjmccall commented May 15, 2020

Uh oh!

jckarter commented May 15, 2020

Uh oh!

rjmccall commented May 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

compnerd May 15, 2020

Choose a reason for hiding this comment

Uh oh!

davezarzycki May 16, 2020

Choose a reason for hiding this comment

Uh oh!

jckarter May 18, 2020

Choose a reason for hiding this comment

Uh oh!

davezarzycki commented May 16, 2020

Uh oh!

rjmccall commented May 16, 2020

Uh oh!

jckarter commented May 18, 2020

Uh oh!

rjmccall commented May 18, 2020

Uh oh!

davezarzycki commented May 20, 2020

Uh oh!

swift-ci commented May 20, 2020

Uh oh!

swift-ci commented May 20, 2020

Uh oh!

davezarzycki commented May 20, 2020

Uh oh!

davezarzycki commented May 20, 2020

Uh oh!

davezarzycki commented May 20, 2020

Uh oh!

davezarzycki commented May 20, 2020

Uh oh!

swift-ci commented May 20, 2020

Uh oh!

davezarzycki commented May 20, 2020

Uh oh!

swift-ci commented May 20, 2020

Uh oh!

swift-ci commented May 20, 2020

Uh oh!

davezarzycki commented May 21, 2020

Uh oh!

swift-ci commented May 21, 2020

Uh oh!

davezarzycki commented May 21, 2020

Uh oh!

davezarzycki commented May 21, 2020

Uh oh!

swift-ci commented May 21, 2020

Uh oh!

swift-ci commented May 21, 2020

Uh oh!

davezarzycki commented May 21, 2020

Uh oh!

davezarzycki commented May 21, 2020

Uh oh!

swift-ci commented May 21, 2020

Uh oh!

swift-ci commented May 21, 2020

Uh oh!

jckarter commented May 21, 2020

davezarzycki commented May 15, 2020 •

edited

Loading

rjmccall commented May 15, 2020 •

edited

Loading