RCIdentity: fix another case of not-RC-identity-preserving casts. #34383

eeckstein · 2020-10-21T20:25:57Z

When casting from existentials to class - and vice versa - it can happen that a cast is not RC identity preserving (because of potential bridging).
This also affects mayRelease() of such cast instructions.
For details see the comments in SILDynamicCastInst::isRCIdentityPreserving().

This change also includes some refactoring: I centralized the logic in SILDynamicCastInst::isRCIdentityPreserving().

rdar://problem/70454804

eeckstein · 2020-10-21T20:26:54Z

@swift-ci test

eeckstein · 2020-10-21T20:27:01Z

@swift-ci benchmark

swift-ci · 2020-10-21T21:12:51Z

Performance: -O

Regression	OLD	NEW	DELTA	RATIO
ReversedBidirectional	15581	17243	+10.7%	0.90x
Array2D	6688	7216	+7.9%	0.93x

Improvement	OLD	NEW	DELTA	RATIO
StringWordBuilderReservingCapacity	2470	2260	-8.5%	1.09x (?)
DropLastAnyCollection	12	11	-8.3%	1.09x (?)
StringWordBuilder	2500	2310	-7.6%	1.08x (?)
NSStringConversion.MutableCopy.Rebridge.Long	1254	1165	-7.1%	1.08x (?)

Code size: -O

Performance: -Osize

Regression	OLD	NEW	DELTA	RATIO
UTF8Decode_InitFromCustom_contiguous	256	355	+38.7%	0.72x
FlattenListLoop	1945	2501	+28.6%	0.78x (?)
StringFromLongWholeSubstring	4	5	+25.0%	0.80x
String.data.Medium	106	126	+18.9%	0.84x (?)
DropFirstSequenceLazy	58	68	+17.2%	0.85x (?)
FlattenListFlatMap	6104	6817	+11.7%	0.90x (?)
RandomShuffleLCG2	416	448	+7.7%	0.93x (?)

Improvement	OLD	NEW	DELTA	RATIO
DataAccessBytesMedium	97	88	-9.3%	1.10x
NSStringConversion.UTF8	1032	944	-8.5%	1.09x (?)
StringWordBuilder	2510	2310	-8.0%	1.09x (?)
NSStringConversion.MutableCopy.Medium	865	797	-7.9%	1.09x (?)
StringWordBuilderReservingCapacity	2460	2270	-7.7%	1.08x
NSStringConversion.MutableCopy.Rebridge.Medium	676	624	-7.7%	1.08x (?)
FloatingPointPrinting_Float_description_uniform	5800	5400	-6.9%	1.07x (?)

Code size: -Osize

Performance: -Onone

Regression	OLD	NEW	DELTA	RATIO
NSStringConversion.MutableCopy.Rebridge	1301	1435	+10.3%	0.91x (?)
ClassArrayGetter2	4540	4940	+8.8%	0.92x (?)
UTF8Decode_InitDecoding_ascii	320	347	+8.4%	0.92x (?)
StringUTF16SubstringBuilder	19680	21220	+7.8%	0.93x (?)

Improvement	OLD	NEW	DELTA	RATIO
DataToStringSmall	6750	5900	-12.6%	1.14x (?)
DictionaryBridgeToObjC_Access	1340	1224	-8.7%	1.09x (?)
UTF8Decode_InitFromData_ascii_as_ascii	808	750	-7.2%	1.08x (?)

Code size: -swiftlibs

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

atrick

This refactoring looks great, but I'm deeply confused about the logic behind which kinds of casts are allowed vs. disallowed, and how that applies to Archetypes.

atrick · 2020-10-21T23:26:06Z

lib/SIL/Utils/DynamicCasts.cpp

+  CanType target = getTargetFormalType();
+
+  // Be conservative with generic parameters.
+  if (source->is<ArchetypeType>() || target->is<ArchetypeType>())


The ArchetypeType check is confusing to me, and "be conservative" doesn't help. In this context, based on all the comments so far, the check implies that an archetype can be substituted with a Metatype. I don't think that true, is it? If so, what would that look like? Instead, the comment should indicate specifically what kind of cast we need to protect against here.

Or is it simply that one of the Archetypes may be a bridged type and one side is non-bridged? In that case, why don't we have the same problem with casting concrete types? (can a concrete bridgeable struct be cast to a bridged class?)

Why don't we treat Archetypes similar to existentials and allow the RC-preserving cast as long as requiresClass() is true for both sides?

I wonder if we could maybe have a different instruction or something like that. The problem here is as I understand it around bridging. Rather than using normal casts for bridging, lets use special casts. Then we have a nice split.

I think whether a cast is bridging depends entirely on the types and may depend on substituted types. I mainly want to know the rules for which type classifications may result in bridging (metatype to ObjC class, struct to ObjC object, ...), whether it's legal to substitute those types with archetypes. Maybe @slavapestov, @tbkka, or @Catfish-Man can help classify these dynamic casts.

For example, in a struct-to-ObjC-object cast, can we assume that the struct is always a trivial type? If so, we just need to comment the 'isTrivial' check to make it clear that it covers bridged structs too, not just metatypes. If not, it seems that we're still missing a case here.

Remind me what Swift considers trivial? Is String trivial? I’d assume not...

@Catfish-Man "trivial" basically means: no ref-counting involved, i.e. values can be copied with a simple memcpy. So yes, String is not trivial.

@atrick I pushed a new version, which should make things clearer about archetypes. The idea is that an archetype can be a class or existential - we don't know.

This check is there to be absolutely on the safe side. Currently we even cannot hit this case, because the only cast instruction which can have an archetype is checked_cast_addr_br - which has the MayRelease attribute. And for RCidentity we don't deal with addr-casts.
I added the check to be future prove, once we switch to opaque values.

For the remaining logic (existential <-> class casts) I consulted with @tbkka (who rewrote big parts of the runtime cast machinery). This should catch all the cases where we can end up with not having RC identity.

This change came out of some tests I'm developing to stress the cast machinery. My tests uncovered problems with casts where one side is AnyObject and the other is a pointer type (metatype, class type, existential, etc). These cases were failing because the code was implicitly assuming that such casts simply copied a pointer, but my tests demonstrate cases where the AnyObject references a container structure that in turn holds the pointer in question. In these cases, retain/release of the AnyObject is not equivalent to retain/release on the contained pointer.

In particular, the previous logic could do the wrong thing in cases like the following:

let source : AnyObject = .... // retain source for later use let result = source as? ClassType // Re-use source here

If source above is just a pointer to a class instance, then it is valid to sink the retain past the cast:

let source : AnyObject = ClassType(). // <<== Simple case let result = source as? ClassType // retain source (which is the same pointer as result). <== THIS IS OK // Re-use source

But this transformation is not valid if source holds a pointer to a runtime container structure that in turn holds the class reference. In that case, the transformation above releases the container prematurely:

let source : AnyObject = ... something gotten from somewhere ... // MUST retain `source` BEFORE the cast, since source may not be a simple class reference let result = source as? ClassType // Re-use source

This is primarily a concern when there are complex interactions with Obj-C code. The bridging support wraps Swift types into a variety of heap-allocated containers. The compiler cannot productively reason about these. They are being exchanged with Obj-C code that is not visible to the compiler, so their structure can really only be understood by dynamic inspection in the runtime casting code.

@eeckstein @tbkka The code is more clear now. Thanks for your patience. It's always helpful for me to see more explanation like this in comments.

Just one question: we seem to be assuming that AnyObject -> Metatype casts will never happen because we don't check for trivial targets. If so, can we add that assertion at least?

Oh? AnyObject -> Metatype casts can definitely happen. (Briefly: anything can be stored in an AnyObject via casting and can later be pulled back out with another cast.)

eeckstein · 2020-10-22T08:52:35Z

@swift-ci smoke test

eeckstein · 2020-10-23T10:53:39Z

@atrick yes, it makes sense to also check the target to be trivial. I pushed a new version where I check if the source and target mismatch in "triviality".

eeckstein · 2020-10-23T10:53:47Z

@swift-ci smoke test

eeckstein · 2020-10-23T10:54:24Z

@swift-ci smoke test

eeckstein · 2020-10-23T15:42:00Z

@swift-ci smoke test

tbkka · 2020-10-24T01:24:17Z

I tried testing this PR combined with my PR #33761 on Ubuntu 16.04 and it looks good so far. (There was something strange with the merge, though, so it's possible I wasn't exactly testing what I hoped.)

When casting from existentials to class - and vice versa - it can happen that a cast is not RC identity preserving (because of potential bridging). This also affects mayRelease() of such cast instructions. For details see the comments in SILDynamicCastInst::isRCIdentityPreserving(). This change also includes some refactoring: I centralized the logic in SILDynamicCastInst::isRCIdentityPreserving(). rdar://problem/70454804

eeckstein · 2020-10-28T07:19:05Z

@swift-ci smoke test and merge

eeckstein requested review from atrick and gottesmm October 21, 2020 20:26

atrick reviewed Oct 21, 2020

View reviewed changes

eeckstein force-pushed the fix_bridging_casts branch from 7acd48b to 0fb1b81 Compare October 22, 2020 08:30

tbkka mentioned this pull request Oct 22, 2020

[DynamicCast] Rely on runtime when casts can't be optimized #33761

Merged

eeckstein force-pushed the fix_bridging_casts branch from 0fb1b81 to 368257c Compare October 23, 2020 10:48

atrick approved these changes Oct 24, 2020

View reviewed changes

eeckstein force-pushed the fix_bridging_casts branch from 368257c to e8e613b Compare October 28, 2020 07:18

swift-ci merged commit 77363fe into swiftlang:main Oct 28, 2020

eeckstein deleted the fix_bridging_casts branch October 28, 2020 10:09

RCIdentity: fix another case of not-RC-identity-preserving casts. #34383

RCIdentity: fix another case of not-RC-identity-preserving casts. #34383

Uh oh!

Conversation

eeckstein commented Oct 21, 2020

Uh oh!

eeckstein commented Oct 21, 2020

Uh oh!

eeckstein commented Oct 21, 2020

Uh oh!

swift-ci commented Oct 21, 2020

Performance: -O

Code size: -O

Performance: -Osize

Code size: -Osize

Performance: -Onone

Code size: -swiftlibs

Uh oh!

atrick left a comment

Choose a reason for hiding this comment

Uh oh!

atrick Oct 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

eeckstein commented Oct 22, 2020

Uh oh!

eeckstein commented Oct 23, 2020

Uh oh!

eeckstein commented Oct 23, 2020

Uh oh!

eeckstein commented Oct 23, 2020

Uh oh!

eeckstein commented Oct 23, 2020

Uh oh!

tbkka commented Oct 24, 2020

Uh oh!

eeckstein commented Oct 28, 2020

Uh oh!

Uh oh!

atrick Oct 21, 2020 •

edited

Loading