Skip to content

[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

Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 52 additions & 19 deletions stdlib/public/runtime/ProtocolConformance.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Copy link
Contributor

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 for ConformanceCacheEntry.)

Copy link
Member Author

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 to ConformanceCacheEntry, 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.

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;
Copy link
Contributor

Choose a reason for hiding this comment

The 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 — ConformanceLookupResult is not a pointer. But it turns out we have

  ConformanceLookupResult(std::nullptr_t) { }

Not sure what I think about that :-) It works, I suppose.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, so it used to be that this was just a pointer (const WitnessTable *) in a number of places, so I added this to keep return nullptr; working from the lookup routines that now return a ConformanceLookupResult.

}
Expand Down