Fixed layout classes should still have resilient vtables #20850

slavapestov · 2018-11-29T01:24:42Z

To achieve this, re-organize the metadata so that the field offset vector comes immediately after the generic parameter area, instead of after the vtable. This means the vtable can change resiliently.

Note that for non-generic @_fixed_layout classes without resiliently-sized fields, field offsets are constant and will be inlined directly, but we need the more general access pattern to work if the @_fixed_layout class has generic layout.

Also, use method dispatch thunks to call vtable methods instead of accessing the vtable directly, as with resilient classes.

Since the metadata size is not known at compile time, we use the most general metadata instantiation strategy now.

slavapestov · 2018-11-29T01:24:52Z

(This still needs some more tests and cleanup)

slavapestov · 2018-11-29T01:25:12Z

@swift-ci Please benchmark

swift-ci · 2018-11-29T02:08:51Z

Build comment file:

Performance: -O

TEST	OLD	NEW	DELTA	RATIO
Regression
UTF8Decode_InitDecoding_ascii	615	748	+21.6%	0.82x

Performance: -Osize

TEST	OLD	NEW	DELTA	RATIO
Regression
FlattenListLoop	3633	3968	+9.2%	0.92x (?)
Array2D	6201	6736	+8.6%	0.92x
Improvement
DropFirstSequence	68	52	-23.5%	1.31x
DropFirstSequenceLazy	68	52	-23.5%	1.31x
DropFirstAnyCollection	163	148	-9.2%	1.10x

Performance: -Onone

TEST	OLD	NEW	DELTA	RATIO
Regression
Breadcrumbs.IdxToUTF16.longASCII	623	705	+13.2%	0.88x
EqualSubstringSubstring	22	24	+9.1%	0.92x (?)

How to read the data

The tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.

If you see any unexpected regressions, you should consider fixing the
regressions before you merge the PR.

Noise: Sometimes the performance results (not code size!) contain false
alarms. Unexpected regressions which are marked with '(?)' are probably noise.
If you see regressions which you cannot explain you can try to run the
benchmarks again. If regressions still show up, please consult with the
performance team (@eeckstein).

Hardware Overview

  Model Name: Mac Pro
  Model Identifier: MacPro6,1
  Processor Name: 8-Core Intel Xeon E5
  Processor Speed: 3 GHz
  Number of Processors: 1
  Total Number of Cores: 8
  L2 Cache (per Core): 256 KB
  L3 Cache: 25 MB
  Memory: 64 GB

--------------

slavapestov · 2018-11-29T02:29:09Z

Let's try with one more tweak

@swift-ci Please benchmark

swift-ci · 2018-11-29T03:18:50Z

Build comment file:

Performance: -O

TEST	OLD	NEW	DELTA	RATIO
Regression
StringEqualPointerComparison	600	657	+9.5%	0.91x
Hanoi	3469	3785	+9.1%	0.92x

Performance: -Osize

TEST	OLD	NEW	DELTA	RATIO
Regression
StringEqualPointerComparison	571	628	+10.0%	0.91x
FlattenListLoop	4057	4429	+9.2%	0.92x (?)
Array2D	6910	7505	+8.6%	0.92x
Breadcrumbs.CopyUTF16CodeUnits.ASCII	50	54	+8.0%	0.93x
Improvement
DropFirstSequence	76	58	-23.7%	1.31x
DropFirstSequenceLazy	76	58	-23.7%	1.31x
DropFirstAnyCollection	183	164	-10.4%	1.12x
PrefixAnySeqCntRangeLazy	176	159	-9.7%	1.11x

Performance: -Onone

TEST	OLD	NEW	DELTA	RATIO
Improvement
ArrayOfGenericPOD2	1180	1065	-9.7%	1.11x (?)
ArrayOfPOD	859	776	-9.7%	1.11x

How to read the data

The tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.

If you see any unexpected regressions, you should consider fixing the
regressions before you merge the PR.

Noise: Sometimes the performance results (not code size!) contain false
alarms. Unexpected regressions which are marked with '(?)' are probably noise.
If you see regressions which you cannot explain you can try to run the
benchmarks again. If regressions still show up, please consult with the
performance team (@eeckstein).

Hardware Overview

  Model Name: Mac Pro
  Model Identifier: MacPro6,1
  Processor Name: 12-Core Intel Xeon E5
  Processor Speed: 2.7 GHz
  Number of Processors: 1
  Total Number of Cores: 12
  L2 Cache (per Core): 256 KB
  L3 Cache: 30 MB
  Memory: 64 GB

--------------

slavapestov · 2018-11-29T03:22:06Z

@swift-ci Please test source compatibility

DougGregor · 2018-11-29T05:30:02Z

@swift-ci Please test source compatibility

DougGregor · 2018-11-29T06:00:26Z

@swift-ci Please test source compatibility

jrose-apple · 2018-11-29T21:31:48Z

lib/SIL/SILDeclRef.cpp

@@ -928,6 +928,9 @@ SubclassScope SILDeclRef::getSubclassScope() const {
  assert(FD->getEffectiveAccess() <= classType->getEffectiveAccess() &&
         "class must be as visible as its members");

+  // FIXME: This is too narrow. Any class with resilient metadata should
+  // probably have this, at least for method overrides that don't add new
+  // vtable entries.


Yeah, this seems important for the correctness issue we're worried about.

Slava clarified privately that we can change this later as long as the fixed-layout classes we ship in Swift 5 don't break the rules that were there in Swift 5.

slavapestov · 2018-11-29T21:48:49Z

@swift-ci Please test

slavapestov · 2018-11-29T21:49:07Z

@swift-ci Please test

rjmccall

Alright, this seems ok.

As I mentioned in chat, this implies that we consider the generic signature section of the metadata to be stable when the metadata is fixed-layout, which may make some things with resiliently-added default generic arguments harder. But I think we can live with that.

swift-ci · 2018-11-29T22:59:06Z

Build failed
Swift Test Linux Platform
Git Sha - 3cac8288eff7df5d7d6a04278c56a3f1c9e67d6f

- We don't want to support changing a root class of a class, so don't pretend that works. Some of these tests got removed recently in d8104e7 but one still remained. - For the tests that insert a non-root superclass in the inheritance hierarchy, also test calling a method of the derived class. This works now that we no longer hardcode vtable offsets and instead use dispatch thunks.

We want to be able to define classes with a fixed storage layout, but a resilient (opaque) vtable. If the class is also generic, we still have to load field offsets from the metadata if they are dependent. So put the field offsets after the generic arguments and before the vtable. This is an ABI break for @_fixed_layout classes, which are defined by the stdlib.

…t-metadata from resilient-storage We want @_fixed_layout classes to have non-resilient storage, but still have resilient metadata.

@_fixed_layout classes have resilient vtables now, so we can add and re-order methods. Removing overrides is not legal though because they are subject to devirtualization.

The result type mismatch comes up when we're calling a covariant override of a class method resiliently as well.

slavapestov · 2018-11-30T04:21:50Z

@swift-ci Please test

slavapestov force-pushed the fixed-layout-classes branch from f389afd to c28379f Compare November 29, 2018 02:28

slavapestov force-pushed the fixed-layout-classes branch from c28379f to 549d1b6 Compare November 29, 2018 21:24

slavapestov requested review from rjmccall and jrose-apple November 29, 2018 21:24

jrose-apple reviewed Nov 29, 2018

View reviewed changes

slavapestov force-pushed the fixed-layout-classes branch from 549d1b6 to 3cac828 Compare November 29, 2018 21:48

rjmccall approved these changes Nov 29, 2018

View reviewed changes

slavapestov added 8 commits November 29, 2018 23:20

AST: Change isResilient() to do the cheaper check first

fcb5217

AST: Introduce ClassDecl::hasResilientMetadata()

4289f76

IRGen: Refactor ClassLayoutBuilder a bit to help distinguish resilien…

16a459d

…t-metadata from resilient-storage We want @_fixed_layout classes to have non-resilient storage, but still have resilient metadata.

IRGen: @_fixed_layout classes still have resilient metadata

ff09603

Evolution: Add some tests for @_fixed_layout classes

a5abb9d

@_fixed_layout classes have resilient vtables now, so we can add and re-order methods. Removing overrides is not legal though because they are subject to devirtualization.

IRGen: Relax an assertion

f8d2881

The result type mismatch comes up when we're calling a covariant override of a class method resiliently as well.

slavapestov force-pushed the fixed-layout-classes branch from 3cac828 to f8d2881 Compare November 30, 2018 04:21

slavapestov merged commit 67ec3b7 into swiftlang:master Nov 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fixed layout classes should still have resilient vtables #20850

Fixed layout classes should still have resilient vtables #20850

Uh oh!

slavapestov commented Nov 29, 2018

Uh oh!

slavapestov commented Nov 29, 2018

Uh oh!

slavapestov commented Nov 29, 2018

Uh oh!

swift-ci commented Nov 29, 2018

Uh oh!

slavapestov commented Nov 29, 2018

Uh oh!

swift-ci commented Nov 29, 2018

Uh oh!

slavapestov commented Nov 29, 2018

Uh oh!

DougGregor commented Nov 29, 2018

Uh oh!

DougGregor commented Nov 29, 2018

Uh oh!

jrose-apple Nov 29, 2018

Uh oh!

jrose-apple Nov 29, 2018

Uh oh!

slavapestov commented Nov 29, 2018

Uh oh!

slavapestov commented Nov 29, 2018

Uh oh!

rjmccall left a comment

Uh oh!

swift-ci commented Nov 29, 2018

Uh oh!

slavapestov commented Nov 30, 2018

Uh oh!

Uh oh!

Fixed layout classes should still have resilient vtables #20850

Fixed layout classes should still have resilient vtables #20850

Uh oh!

Conversation

slavapestov commented Nov 29, 2018

Uh oh!

slavapestov commented Nov 29, 2018

Uh oh!

slavapestov commented Nov 29, 2018

Uh oh!

swift-ci commented Nov 29, 2018

Build comment file:

Performance: -O

Performance: -Osize

Performance: -Onone

Uh oh!

slavapestov commented Nov 29, 2018

Uh oh!

swift-ci commented Nov 29, 2018

Build comment file:

Performance: -O

Performance: -Osize

Performance: -Onone

Uh oh!

slavapestov commented Nov 29, 2018

Uh oh!

DougGregor commented Nov 29, 2018

Uh oh!

DougGregor commented Nov 29, 2018

Uh oh!

jrose-apple Nov 29, 2018

Choose a reason for hiding this comment

Uh oh!

jrose-apple Nov 29, 2018

Choose a reason for hiding this comment

Uh oh!

slavapestov commented Nov 29, 2018

Uh oh!

slavapestov commented Nov 29, 2018

Uh oh!

rjmccall left a comment

Choose a reason for hiding this comment

Uh oh!

swift-ci commented Nov 29, 2018

Uh oh!

slavapestov commented Nov 30, 2018

Uh oh!

Uh oh!