Fix EscapeAnalysis::mayReleaseContent #36046

atrick · 2021-02-19T03:02:54Z

The previous implementation made extremely subtle and specific
assumptions about how the API is used which doesn't apply
everywhere. It was trying very hard to avoid regressing performance
relative to an even older implementation that didn't even try to consider deinitializer side effects.

The aggressive logic was based on the idea that a release must have a
corresponding retain somewhere in the same function and we don't care
if the last release happens early if there are no more aliasing
uses. All the unit tests I wrote previously were based on release
hoisting, which happens to work given the way the API is used.

But this logic is incorrect for retain sinking. In that case sinking
past an "unrelated" release could cause the object to be freed
early. See test/SILOptimizer/arc_crash.swift.

With SemanticARC and other SIL improvements being made, I'm afraid
bugs like this will begin to surface.

To fix it, just remove the subtle logic to leave behind a simple and
sound EscapeAnalysis API. To do better, we will need to rewrite the
AliasAnalysis logic for release side effects, which is currently
a tangled web. In the meantime, SemanticARC can handle many cases without EscapeAnalysis.

Fixes rdar://74469299 (ARC miscompile:
EscapeAnalysis::mayReleaseContent; potential use-after-free)

While fixing this, add support for address-type queries too:

Fixes rdar://74360041 (Assertion failed:
(!releasedReference->getType().isAddress() && "an address is never a
reference"), function mayReleaseContent

Deinitializer side effects severely handicap the connection-graph
based EscapeAnalysis. Because it's not flow-sensitive, this approach
falls apart whenever an object is released in the current function,
which makes it seem to have escaped everywhere (it generally doesn't
matter if it doesn't escape until the release point, but the analysis
can't discern that).

This can be slightly mitigated by realizing that releasing an object
can only cause things it points to to escape if the object itself has
references or pointers.

Fix: Don't create a escaping content node when releasing an object
that can't contain any references or pointers. The previous commit,
"Fix EscapeAnalysis::mayReleaseContent" would defeat release-hoisting
in important cases without doing anything else. Adding this extra
precision to the connection graph avoids some regressions.

The previous implementation made extremely subtle and specific assumptions about how the API is used which doesn't apply everywhere. It was trying very hard to avoid regressing performance relative to an even older implementation that didn't even try to consider deinitializer side effects. The aggressive logic was based on the idea that a release must have a corresponding retain somewhere in the same function and we don't care if the last release happens early if there are no more aliasing uses. All the unit tests I wrote previously were based on release hoisting, which happens to work given the way the API is used. But this logic is incorrect for retain sinking. In that case sinking past an "unrelated" release could cause the object to be freed early. See test/SILOptimizer/arc_crash.swift. With SemanticARC and other SIL improvements being made, I'm afraid bugs like this will begin to surface. To fix it, just remove the subtle logic to leave behind a simple and sound EscapeAnalysis API. To do better, we will need to rewrite the AliasAnalysis logic for release side effects, which is currently a tangled web. In the meantime, SemanticARC can handle many cases without EscapeAnalysis. Fixes rdar://74469299 (ARC miscompile: EscapeAnalysis::mayReleaseContent; potential use-after-free) While fixing this, add support for address-type queries too: Fixes rdar://74360041 (Assertion failed: (!releasedReference->getType().isAddress() && "an address is never a reference"), function mayReleaseContent

atrick · 2021-02-19T03:03:29Z

@slavapestov In this commit:
ba23805

For EscapeAnalysis::findObjectKind. I spent a long time trying to figure out how to prove that a class type doesn't have any pointer or reference fields and handle resilience. I finally cobbled something together but still don't know what I'm doing.

atrick · 2021-02-19T03:51:02Z

@swift-ci test

atrick · 2021-02-19T03:51:11Z

@swift-ci test source compatibility

swift-ci · 2021-02-19T03:51:34Z

Build failed
Swift Test OS X Platform
Git Sha - 976aaf0f8532bc7a745dbb7a4d5e41bdf1ed8d1a

atrick · 2021-02-19T03:52:12Z

@swift-ci test

swift-ci · 2021-02-19T04:18:00Z

Build failed
Swift Test OS X Platform
Git Sha - 976aaf0f8532bc7a745dbb7a4d5e41bdf1ed8d1a

swift-ci · 2021-02-19T05:18:28Z

Build failed
Swift Test Linux Platform
Git Sha - 976aaf0f8532bc7a745dbb7a4d5e41bdf1ed8d1a

lib/SILOptimizer/Analysis/EscapeAnalysis.cpp

slavapestov · 2021-02-19T05:52:31Z

@atrick Is it worth running the benchmark suite too?

atrick · 2021-02-19T06:32:03Z

@swift-ci benchmark

atrick · 2021-02-19T06:32:10Z

@swift-ci test

atrick · 2021-02-19T06:32:19Z

@swift-ci test source compatibility

swift-ci · 2021-02-19T07:31:27Z

Build failed before running benchmark.

swift-ci · 2021-02-19T08:00:31Z

Build failed
Swift Test Linux Platform
Git Sha - 073ff217d15931b875eca55b2c3a3b42c43abf24

swift-ci · 2021-02-19T08:21:32Z

Build failed
Swift Test OS X Platform
Git Sha - 073ff217d15931b875eca55b2c3a3b42c43abf24

eeckstein · 2021-02-19T14:17:08Z

lib/SILOptimizer/Analysis/AliasAnalysis.cpp

@@ -744,7 +754,7 @@ bool AliasAnalysis::canBuiltinDecrementRefCount(BuiltinInst *BI, SILValue Ptr) {
    // to be an owned reference and disallows addresses. Conservatively handle
    // address type arguments as and conservatively treat all other values
    // potential owned references.
-    if (Arg->getType().isAddress() || EA->mayReleaseContent(Arg, Ptr))
+    if (Arg->getType().isAddress() || EA->mayReleaseReferenceContent(Arg, Ptr))


Are we sure we don't have any builtins with indirect arguments?

I think it's safe to enable this now for builtins taking addresses

eeckstein · 2021-02-19T14:34:27Z

lib/SILOptimizer/Analysis/EscapeAnalysis.cpp

+// to point to any other objects.
+EscapeAnalysis::PointerKind
+EscapeAnalysis::findObjectKind(SILType Ty, const SILFunction &F) const {
+  if (Ty.isAddress())


Can this ever be an address? Operands to release instructions are not addresses and types of stored properties are not addresses either.

I'm ambivalent about whether this should be an assert. It's ok right now to call it on any type and it gives you a conservative answer for non-class types.

fine with me. Maybe you can mention in a comment that it's (currently) never called for an address type.

eeckstein · 2021-02-19T14:34:59Z

lib/SILOptimizer/Analysis/EscapeAnalysis.cpp

+    return AnyPointer;
+
+  auto *classDecl = Ty.getClassOrBoundGenericClass();
+  if (!classDecl)


Why not handle structs and tuples as well? That's a pretty common case.

Structs and tuples are handles by getPointerKind. References are always pointers in that case. This specifically looks at object properties.

eeckstein · 2021-02-19T14:37:43Z

lib/SILOptimizer/Analysis/EscapeAnalysis.cpp

+    for (VarDecl *property : classDecl->getStoredProperties()) {
+      SILType fieldTy =
+          objTy.getFieldType(property, M, expansion).getObjectType();
+      meetAggregateKind(findCachedPointerKind(fieldTy, F));


That logic looks wrong to me. In case a stored property is a reference, shouldn't it return ReferenceOnly? Instead it recurses into the referenced class type and will e.g. return NoPointer if the referenced class has only trivial properties.

I renamed the new helper to findClassPropertiesPointerKind with this comment

// For each field in the class, get the pointer kind for that field. For // reference-type properties, this will be ReferenceOnly. For aggregates, it // will be the meet over all aggregate fields.

Ah, I misread: it's calling findCachedPointerKind and not findCachedClassPropertiesKind

eeckstein · 2021-02-19T14:40:37Z

lib/SILOptimizer/Analysis/EscapeAnalysis.cpp

@@ -113,6 +156,17 @@ EscapeAnalysis::findCachedPointerKind(SILType Ty, const SILFunction &F) const {
  return pointerKind;
 }

+EscapeAnalysis::PointerKind
+EscapeAnalysis::findCachedObjectKind(SILType Ty, const SILFunction &F) const {
+  auto iter = objectKindCache.find(Ty);


Caching the pointer kind sounds dangerous. The pointer kind is computed per function and can be different per function (because the resilience expansion can be different). But it's cached globally for all functions.

Maybe always use ResilienceExpansion::Minimal?

BTW, it's the same situation with the already existing pointerKindCache.

Thanks, I changed that to Maximal after the last feedback without even thinking.

atrick · 2021-02-19T19:43:40Z

@swift-ci test

atrick · 2021-02-19T19:43:48Z

@swift-ci benchmark

atrick · 2021-02-19T19:44:06Z

@swift-ci test source compatibility

Deinitializer side effects severely handicap the connection-graph based EscapeAnalysis. Because it's not flow-sensitive, this approach falls apart whenever an object is released in the current function, which makes it seem to have escaped everywhere (it generally doesn't matter if it doesn't escape until the release point, but the analysis can't discern that). This can be slightly mitigated by realizing that releasing an object can only cause things it points to to escape if the object itself has references or pointers. Fix: Don't create a escaping content node when releasing an object that can't contain any references or pointers. The previous commit, "Fix EscapeAnalysis::mayReleaseContent" would defeat release-hoisting in important cases without doing anything else. Adding this extra precision to the connection graph avoids some regressions.

atrick · 2021-02-19T20:35:33Z

@swift-ci test

atrick · 2021-02-19T20:35:43Z

@swift-ci test source compatibility

swift-ci · 2021-02-19T20:36:15Z

Build failed
Swift Test Linux Platform
Git Sha - 5aa415ef889cab493477a9e8e334a8a3b6700deb

atrick · 2021-02-19T20:36:50Z

@swift-ci benchmark

swift-ci · 2021-02-19T20:39:49Z

Build failed
Swift Test OS X Platform
Git Sha - 5aa415ef889cab493477a9e8e334a8a3b6700deb

swift-ci · 2021-02-19T21:39:54Z

Performance: -O

Improvement	OLD	NEW	DELTA	RATIO
FlattenListFlatMap	6684	5764	-13.8%	1.16x (?)

Code size: -O

Performance: -Osize

Regression	OLD	NEW	DELTA	RATIO
FlattenListFlatMap	3906	5705	+46.1%	0.68x (?)
ObjectiveCBridgeToNSSet	16400	17950	+9.5%	0.91x (?)
ObjectiveCBridgeFromNSArrayAnyObjectForced	4720	5100	+8.1%	0.93x (?)

Code size: -Osize

Performance: -Onone

Improvement	OLD	NEW	DELTA	RATIO
NSError	831	702	-15.5%	1.18x (?)

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 · 2021-02-20T00:35:47Z

@swift-ci test

swift-ci · 2021-02-20T00:57:40Z

Build failed
Swift Test Linux Platform
Git Sha - f6624e3

atrick · 2021-02-20T01:05:02Z

@swift-ci smoke test linux

swift-ci · 2021-02-20T03:31:51Z

Build failed
Swift Test OS X Platform
Git Sha - f6624e3

atrick · 2021-02-20T06:51:58Z

The only SCK failure is SwiftDate, where I see lots of

error opening '.../Release-iphoneos/SwiftDate-iOS.build/Objects-normal/armv7/String+Parser.o' for output: No such file or directory

atrick · 2021-02-20T06:52:38Z

@swift-ci test

swift-ci · 2021-02-20T08:13:41Z

Build failed
Swift Test Linux Platform
Git Sha - f6624e3

swift-ci · 2021-02-20T09:10:08Z

Build failed
Swift Test OS X Platform
Git Sha - f6624e3

atrick · 2021-02-21T16:51:02Z

@swift-ci test

atrick requested review from eeckstein and slavapestov February 19, 2021 03:02

atrick force-pushed the fix-escape branch from ba23805 to 976aaf0 Compare February 19, 2021 03:50

slavapestov reviewed Feb 19, 2021

View reviewed changes

lib/SILOptimizer/Analysis/EscapeAnalysis.cpp Show resolved Hide resolved

atrick force-pushed the fix-escape branch from 976aaf0 to 073ff21 Compare February 19, 2021 06:30

eeckstein reviewed Feb 19, 2021

View reviewed changes

atrick force-pushed the fix-escape branch 2 times, most recently from 85e206f to 5aa415e Compare February 19, 2021 19:43

atrick force-pushed the fix-escape branch from 5aa415e to f6624e3 Compare February 19, 2021 20:35

LucianoPAlmeida mentioned this pull request Feb 19, 2021

[CSBindings] Avoid to infer transitive defaults for DefaultClosureType #36043

Merged

atrick merged commit 81c0d09 into swiftlang:main Feb 22, 2021

atrick deleted the fix-escape branch February 22, 2021 02:37

Fix EscapeAnalysis::mayReleaseContent #36046

Fix EscapeAnalysis::mayReleaseContent #36046

Uh oh!

Conversation

atrick commented Feb 19, 2021

Uh oh!

atrick commented Feb 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atrick commented Feb 19, 2021

Uh oh!

atrick commented Feb 19, 2021

Uh oh!

swift-ci commented Feb 19, 2021

Uh oh!

atrick commented Feb 19, 2021

Uh oh!

swift-ci commented Feb 19, 2021

Uh oh!

swift-ci commented Feb 19, 2021

Uh oh!

Uh oh!

slavapestov commented Feb 19, 2021

Uh oh!

atrick commented Feb 19, 2021

Uh oh!

atrick commented Feb 19, 2021

Uh oh!

atrick commented Feb 19, 2021

Uh oh!

swift-ci commented Feb 19, 2021

Uh oh!

swift-ci commented Feb 19, 2021

Uh oh!

swift-ci commented Feb 19, 2021

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 Feb 21, 2021 • 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!

atrick commented Feb 19, 2021

Uh oh!

atrick commented Feb 19, 2021

Uh oh!

atrick commented Feb 19, 2021

Uh oh!

atrick commented Feb 19, 2021

Uh oh!

atrick commented Feb 19, 2021

Uh oh!

swift-ci commented Feb 19, 2021

Uh oh!

atrick commented Feb 19, 2021

Uh oh!

swift-ci commented Feb 19, 2021

Uh oh!

swift-ci commented Feb 19, 2021

Performance: -O

atrick commented Feb 19, 2021 •

edited

Loading

eeckstein Feb 21, 2021 •

edited

Loading