Skip to content

Cache ConformanceAccessPaths in GSB::EquivalenceClass for big wins #18597

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.

Sign up for GitHub

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
merged 3 commits into from
Aug 14, 2018
Merged

Cache ConformanceAccessPaths in GSB::EquivalenceClass for big wins #18597

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f8fd85b
Cache ConformanceAccessPaths in GSB::EquivalenceClass for big wins
jrose-apple Aug 9, 2018
480af40
Move ConformanceAccessPaths to live completely on the ASTContext
jrose-apple Aug 10, 2018
5d7f07f
Extract local helper to GenericSignature::buildConformanceAccessPath
jrose-apple Aug 10, 2018
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
6 changes: 5 additions & 1 deletion include/swift/AST/GenericSignatureBuilder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

#include "swift/AST/Decl.h"
#include "swift/AST/DiagnosticEngine.h"
#include "swift/AST/GenericSignature.h"
#include "swift/AST/Identifier.h"
#include "swift/AST/ProtocolConformanceRef.h"
#include "swift/AST/Types.h"
Expand All @@ -45,7 +46,6 @@ namespace swift {
class DeclContext;
class DependentMemberType;
class GenericParamList;
class GenericSignature;
class GenericSignatureBuilder;
class GenericTypeParamType;
class LazyResolver;
Expand Down Expand Up @@ -278,6 +278,10 @@ class GenericSignatureBuilder {
/// Cached nested-type information, which contains the best declaration
/// for a given name.
llvm::SmallDenseMap<Identifier, CachedNestedType> nestedTypeNameCache;

/// Cached access paths.
llvm::SmallDenseMap<const ProtocolDecl *, ConformanceAccessPath, 8>
conformanceAccessPathCache;
};

friend class RequirementSource;
Expand Down
17 changes: 12 additions & 5 deletions lib/AST/GenericSignature.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -829,6 +829,10 @@ ConformanceAccessPath GenericSignature::getConformanceAccessPath(
type,
ArchetypeResolutionKind::CompleteWellFormed);

auto cached = equivClass->conformanceAccessPathCache.find(protocol);
if (cached != equivClass->conformanceAccessPathCache.end())
return cached->second;

// Dig out the conformance of this type to the given protocol, because we
// want its requirement source.
auto conforms = equivClass->conformsTo.find(protocol);
Expand All @@ -840,11 +844,12 @@ ConformanceAccessPath GenericSignature::getConformanceAccessPath(

// Local function to construct the conformance access path from the
// requirement.
std::function<void(ArrayRef<Requirement>, const RequirementSource *,
ProtocolDecl *, Type, ProtocolDecl *)> buildPath;
buildPath = [&](ArrayRef<Requirement> reqs, const RequirementSource *source,
ProtocolDecl *conformingProto, Type rootType,
ProtocolDecl *requirementSignatureProto) {
llvm::function_ref<void(ArrayRef<Requirement>, const RequirementSource *,
ProtocolDecl *, Type, ProtocolDecl *)> buildPath;
auto buildPathLambda = [&](ArrayRef<Requirement> reqs,
const RequirementSource *source,
ProtocolDecl *conformingProto, Type rootType,
ProtocolDecl *requirementSignatureProto) {
// Each protocol requirement is a step along the path.
if (source->isProtocolRequirement()) {
// If we're expanding for a protocol that had no requirement signature
Expand Down Expand Up @@ -976,6 +981,7 @@ ConformanceAccessPath GenericSignature::getConformanceAccessPath(
// Add the root of the path, which starts at this explicit requirement.
path.path.push_back({rootType, conformingProto});
};
buildPath = buildPathLambda;
Copy link
Contributor

@rjmccall rjmccall Aug 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I don't understand the purpose of binding buildPath specifically as a function_ref here.

Oh, I guess it uses itself recursively? Well, function_ref is better than std::function, at least.

...should this just be hoisted to file scope? This function is huge, and it mostly relies on its parameters, not captures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would probably work too. I didn't want to modify code I didn't understand too much, but the allocation was bothering me.


// Canonicalize the root type.
auto source = getBestRequirementSource(conforms->second);
Expand All @@ -985,6 +991,7 @@ ConformanceAccessPath GenericSignature::getConformanceAccessPath(
buildPath(getRequirements(), source, protocol, rootType, nullptr);

// Return the path; we're done!
equivClass->conformanceAccessPathCache[protocol] = path;
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're caching these instead of building them on-demand, can we reasonably return them by reference? Or make ConformanceAccessPath just hold a reference to external storage so that it's cheaper to pass one around?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That seems sensible too.

return path;
}

Expand Down