-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[Runtime] Don't use the low bit of a WitnessTable pointer in the conformance cache #80844
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -537,47 +537,80 @@ namespace { | |
|
||
struct ConformanceCacheEntry { | ||
private: | ||
ConformanceCacheKey Key; | ||
/// Storage used when we have global actor isolation on the conformance. | ||
struct ExtendedStorage { | ||
/// The protocol to which the type conforms. | ||
const ProtocolDescriptor *Proto; | ||
|
||
/// The global actor to which this conformance is isolated, or NULL for | ||
/// a nonisolated conformances. | ||
const Metadata *globalActorIsolationType = nullptr; | ||
|
||
/// When the conformance is global-actor-isolated, this is the conformance | ||
/// of globalActorIsolationType to GlobalActor. | ||
const WitnessTable *globalActorIsolationWitnessTable = nullptr; | ||
}; | ||
|
||
const Metadata *Type; | ||
llvm::PointerUnion<const ProtocolDescriptor *, const ExtendedStorage *> | ||
ProtoOrStorage; | ||
|
||
/// The witness table or along with a bit that indicates whether the | ||
/// conformance is isolated to a global actor. | ||
llvm::PointerUnion<const WitnessTable *, const ConformanceLookupResult *> | ||
WitnessTableOrLookupResult; | ||
/// The witness table. | ||
const WitnessTable *Witness; | ||
|
||
public: | ||
ConformanceCacheEntry(ConformanceCacheKey key, | ||
ConformanceLookupResult result) | ||
: Key(key) { | ||
: Type(key.Type), Witness(result.witnessTable) | ||
{ | ||
if (result.globalActorIsolationType) { | ||
WitnessTableOrLookupResult = new ConformanceLookupResult(result); | ||
ProtoOrStorage = new ExtendedStorage{ | ||
key.Proto, result.globalActorIsolationType, | ||
result.globalActorIsolationWitnessTable | ||
}; | ||
} else { | ||
WitnessTableOrLookupResult = result.witnessTable; | ||
ProtoOrStorage = key.Proto; | ||
} | ||
} | ||
|
||
bool matchesKey(const ConformanceCacheKey &key) const { | ||
return Key.Type == key.Type && Key.Proto == key.Proto; | ||
return Type == key.Type && getProtocol() == key.Proto; | ||
} | ||
|
||
friend llvm::hash_code hash_value(const ConformanceCacheEntry &entry) { | ||
return hash_value(entry.Key); | ||
return hash_value(entry.getKey()); | ||
} | ||
|
||
/// Get the protocol. | ||
const ProtocolDescriptor *getProtocol() const { | ||
if (auto proto = ProtoOrStorage.dyn_cast<const ProtocolDescriptor *>()) | ||
return proto; | ||
|
||
if (auto storage = ProtoOrStorage.dyn_cast<const ExtendedStorage *>()) | ||
return storage->Proto; | ||
|
||
return nullptr; | ||
} | ||
|
||
/// Get the conformance cache key. | ||
ConformanceCacheKey getKey() const { | ||
return ConformanceCacheKey(Type, getProtocol()); | ||
} | ||
|
||
/// Get the cached witness table, or null if we cached failure. | ||
const WitnessTable *getWitnessTable() const { | ||
if (auto witnessTable = WitnessTableOrLookupResult.dyn_cast<const WitnessTable *>()) | ||
return witnessTable; | ||
|
||
return WitnessTableOrLookupResult.get<const ConformanceLookupResult *>() | ||
->witnessTable; | ||
return Witness; | ||
} | ||
|
||
ConformanceLookupResult getResult() const { | ||
if (auto witnessTable = WitnessTableOrLookupResult.dyn_cast<const WitnessTable *>()) | ||
return ConformanceLookupResult { witnessTable, nullptr, nullptr }; | ||
if (ProtoOrStorage.is<const ProtocolDescriptor *>()) | ||
return ConformanceLookupResult { Witness, nullptr, nullptr }; | ||
|
||
if (auto lookupResult = WitnessTableOrLookupResult.dyn_cast<const ConformanceLookupResult *>()) | ||
return *lookupResult; | ||
if (auto storage = ProtoOrStorage.dyn_cast<const ExtendedStorage *>()) { | ||
return ConformanceLookupResult( | ||
Witness, storage->globalActorIsolationType, | ||
storage->globalActorIsolationWitnessTable); | ||
} | ||
|
||
return nullptr; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Nothing to do with this PR) This line made me raise my eyebrows a bit —
Not sure what I think about that :-) It works, I suppose. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, so it used to be that this was just a pointer ( |
||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to use
swift_cxx_newObject()
here? (Also, this will leak whenever we dynamically load a new image, since we clear the conformance cache at that point and there's no destructor forConformanceCacheEntry
.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can switch to
swift_cxx_newObject()
, sure. And... I had not realized that we clear the conformance cache, ever. If we do... well, let's figure out a way to free this memory. I don't want to add a destructor toConformanceCacheEntry
, though, because only a tiny fraction of protocol conformances actual have this extra storage. Maybe an intrusive linked list of the extended storage pointers instead.