Improve lookup performance. (#7798)

* Improve lookup performance.

* Cleanup.

* Improve autocompletion latency.

10 files changed, 152 insertions, 92 deletions

diff --git a/source/slang/slang-ast-decl.cpp b/source/slang/slang-ast-decl.cpp
index 4d9b8718f..1e2d11600 100644
--- a/source/slang/slang-ast-decl.cpp
+++ b/source/slang/slang-ast-decl.cpp
@@ -405,25 +405,6 @@ void ContainerDecl::_ensureLookupAcceleratorsAreValid()
 }
 
 void ContainerDecl::
-    _invalidateLookupAcceleratorsBecauseUnscopedEnumAttributeWillBeTurnedIntoTransparentModifier(
-        UnscopedEnumAttribute* unscopedEnumAttr,
-        TransparentModifier* transparentModifier)
-{
-    if (isUsingOnDemandDeserializationForDirectMembers())
-    {
-        SLANG_UNEXPECTED("this operation shouldn't be performed on deserialized declarations");
-    }
-
-    SLANG_ASSERT(unscopedEnumAttr);
-    SLANG_ASSERT(transparentModifier);
-
-    SLANG_UNUSED(unscopedEnumAttr);
-    SLANG_UNUSED(transparentModifier);
-
-    _invalidateLookupAccelerators();
-}
-
-void ContainerDecl::
     _removeDirectMemberConstructorDeclBecauseSynthesizedAnotherDefaultConstructorInstead(
         ConstructorDecl* decl)
 {
diff --git a/source/slang/slang-ast-decl.h b/source/slang/slang-ast-decl.h
index 123ac5390..7b2cc4007 100644
--- a/source/slang/slang-ast-decl.h
+++ b/source/slang/slang-ast-decl.h
@@ -248,29 +248,6 @@ class ContainerDecl : public Decl
     // examples of how to interact with the Slang AST.
     //
 
-    /// Invalidate the acceleration structures used for declaration lookup,
-    /// because the code is about to replace `unscopedEnumAttr` with `transparentModifier`
-    /// as part of semantic checking of an `EnumDecl` nested under this `ContainerDecl`.
-    ///
-    /// Cannot be expressed in terms of the rest of the public API because the
-    /// existing assumption has been that any needed `TransparentModifier`s would
-    /// be manifestly obvious just from the syntax being parsed, so that they would
-    /// already be in place on parsed ASTs. the `UnscopedEnumAttribute` is a `TransparentModifier`
-    /// in all but name, but the two don't share a common base class such that code
-    /// could check for them together.
-    ///
-    /// TODO: In the long run, the obvious fix is to eliminate `UnscopedEnumAttribute`,
-    /// becuase it only exists to enable legacy code to be expressed in Slang, rather than
-    /// representing anything we want/intend to support long-term.
-    ///
-    /// TODO: In the even *longer* run, we should eliminate `TransparentModifier` as well,
-    /// because it only exists to support legacy `cbuffer` declarations and similar syntax,
-    /// and those should be deprecated over time.
-    ///
-    void _invalidateLookupAcceleratorsBecauseUnscopedEnumAttributeWillBeTurnedIntoTransparentModifier(
-        UnscopedEnumAttribute* unscopedEnumAttr,
-        TransparentModifier* transparentModifier);
-
     /// Remove a constructor declaration from the direct member declarations of this container.
     ///
     /// This operation is seemingly used when a default constructor declaration has been synthesized
diff --git a/source/slang/slang-check-decl.cpp b/source/slang/slang-check-decl.cpp
index a3662702a..46876131a 100644
--- a/source/slang/slang-check-decl.cpp
+++ b/source/slang/slang-check-decl.cpp
@@ -8870,6 +8870,8 @@ void SemanticsDeclBodyVisitor::visitEnumCaseDecl(EnumCaseDecl* decl)
     auto parentEnumDecl = as<EnumDecl>(decl->parentDecl);
     SLANG_ASSERT(parentEnumDecl);
 
+    ensureDecl(parentEnumDecl, DeclCheckState::ReadyForLookup);
+
     decl->type.type = DeclRefType::create(m_astBuilder, makeDeclRef(parentEnumDecl));
 
     // The tag type should have already been set by
diff --git a/source/slang/slang-check-impl.h b/source/slang/slang-check-impl.h
index 6b36031ee..86d0b42fe 100644
--- a/source/slang/slang-check-impl.h
+++ b/source/slang/slang-check-impl.h
@@ -458,6 +458,12 @@ struct FacetImpl
     ///
     Facet::Origin origin;
 
+    // DeclRef to the container that represent this facet for member lookup.
+    // If the facet is for an interface decl, this declref will be a specialized `LookupDeclRef`
+    // containing the subtype witness that the type this facet belongs to is a subtype of interface
+    // represented by this facet.
+    DeclRef<ContainerDecl> declRefForMemberLookup;
+
     Type* getType() const { return origin.type; }
     DeclRef<Decl> getDeclRef() const { return origin.declRef; }
 
@@ -470,9 +476,18 @@ struct FacetImpl
     /// The next facet in the linearized inheritance list of the entity.
     Facet next;
 
+    void setSubtypeWitness(ASTBuilder* builder, SubtypeWitness* witness)
+    {
+        subtypeWitness = witness;
+        init(builder);
+    }
+
+    void init(ASTBuilder* builder);
+
     FacetImpl() {}
 
     FacetImpl(
+        ASTBuilder* astBuilder,
         Facet::Kind kind,
         Facet::Directness directness,
         DeclRef<Decl> declRef,
@@ -480,6 +495,7 @@ struct FacetImpl
         SubtypeWitness* subtypeWitness)
         : kind(kind), directness(directness), origin(declRef, type), subtypeWitness(subtypeWitness)
     {
+        init(astBuilder);
     }
 };
 
diff --git a/source/slang/slang-check-inheritance.cpp b/source/slang/slang-check-inheritance.cpp
index 2554553de..164828bac 100644
--- a/source/slang/slang-check-inheritance.cpp
+++ b/source/slang/slang-check-inheritance.cpp
@@ -218,8 +218,13 @@ InheritanceInfo SharedSemanticsContext::_calcInheritanceInfo(
     //
     TypeEqualityWitness* selfIsSelf =
         selfType ? visitor.createTypeEqualityWitness(selfType) : nullptr;
-    Facet selfFacet = new (arena)
-        Facet::Impl(selfFacetKind, Facet::Directness::Self, declRef, selfType, selfIsSelf);
+    Facet selfFacet = new (arena) Facet::Impl(
+        astBuilder,
+        selfFacetKind,
+        Facet::Directness::Self,
+        declRef,
+        selfType,
+        selfIsSelf);
     allFacets.add(selfFacet);
 
     // After the self facet will come a list of facets formed
@@ -253,8 +258,13 @@ InheritanceInfo SharedSemanticsContext::_calcInheritanceInfo(
         // the base in the linearized inheritance list
         // we are building.
         //
-        baseInfo->facetImpl =
-            FacetImpl(kind, Facet::Directness::Direct, baseDeclRef, baseType, selfIsBaseWitness);
+        baseInfo->facetImpl = FacetImpl(
+            astBuilder,
+            kind,
+            Facet::Directness::Direct,
+            baseDeclRef,
+            baseType,
+            selfIsBaseWitness);
         Facet baseFacet(&baseInfo->facetImpl);
         //
         // Second, we have a list of the facets in the
@@ -852,7 +862,7 @@ void SharedSemanticsContext::_mergeFacetLists(
                 selfIsSubtypeOfBase,
                 baseIsSubtypeOfFacet);
 
-            indirectFacet->subtypeWitness = selfIsSubtypeOfFacet;
+            indirectFacet->setSubtypeWitness(_getASTBuilder(), selfIsSubtypeOfFacet);
 
             ioMergedFacets.add(indirectFacet);
         }
@@ -1105,6 +1115,7 @@ InheritanceInfo SharedSemanticsContext::_calcInheritanceInfo(
         //
         DirectBaseInfo leftBaseInfo;
         leftBaseInfo.facetImpl = FacetImpl(
+            astBuilder,
             Facet::Kind::Type,
             Facet::Directness::Direct,
             DeclRef<Decl>(),
@@ -1114,6 +1125,7 @@ InheritanceInfo SharedSemanticsContext::_calcInheritanceInfo(
 
         DirectBaseInfo rightBaseInfo;
         rightBaseInfo.facetImpl = FacetImpl(
+            astBuilder,
             Facet::Kind::Type,
             Facet::Directness::Direct,
             DeclRef<Decl>(),
@@ -1143,6 +1155,7 @@ InheritanceInfo SharedSemanticsContext::_calcInheritanceInfo(
             getInheritanceInfo(eachType->getElementType(), circularityInfo);
         SemanticsVisitor visitor(this);
         auto directFacet = new (arena) Facet::Impl(
+            astBuilder,
             Facet::Kind::Type,
             Facet::Directness::Self,
             DeclRef<Decl>(),
@@ -1154,6 +1167,7 @@ InheritanceInfo SharedSemanticsContext::_calcInheritanceInfo(
             if (facet->directness == Facet::Directness::Direct)
             {
                 auto eachFacet = new (arena) Facet::Impl(
+                    astBuilder,
                     Facet::Kind::Type,
                     Facet::Directness::Direct,
                     facet->origin.declRef,
@@ -1182,6 +1196,7 @@ InheritanceInfo SharedSemanticsContext::_calcInheritanceInfo(
         //
         SemanticsVisitor visitor(this);
         auto directFacet = new (arena) Facet::Impl(
+            astBuilder,
             Facet::Kind::Type,
             Facet::Directness::Self,
             DeclRef<Decl>(),
diff --git a/source/slang/slang-check-modifier.cpp b/source/slang/slang-check-modifier.cpp
index 0561a7084..db477ac25 100644
--- a/source/slang/slang-check-modifier.cpp
+++ b/source/slang/slang-check-modifier.cpp
@@ -1656,19 +1656,6 @@ Modifier* SemanticsVisitor::checkModifier(
         //
 
         auto checkedAttr = checkAttribute(hlslUncheckedAttribute, syntaxNode);
-
-        if (auto unscopedEnumAttr = as<UnscopedEnumAttribute>(checkedAttr))
-        {
-            auto transparentModifier = getASTBuilder()->create<TransparentModifier>();
-            if (auto parentDecl = getParentDecl(as<Decl>(syntaxNode)))
-            {
-                parentDecl
-                    ->_invalidateLookupAcceleratorsBecauseUnscopedEnumAttributeWillBeTurnedIntoTransparentModifier(
-                        unscopedEnumAttr,
-                        transparentModifier);
-            }
-            return transparentModifier;
-        }
         return checkedAttr;
     }
 
diff --git a/source/slang/slang-language-server.cpp b/source/slang/slang-language-server.cpp
index ba2722fae..048fcb44b 100644
--- a/source/slang/slang-language-server.cpp
+++ b/source/slang/slang-language-server.cpp
@@ -565,6 +565,12 @@ HumaneSourceLoc getModuleLoc(SourceManager* manager, ContainerDecl* moduleDecl)
     return location;
 }
 
+void LanguageServer::removePendingModuleToUpdateDiagnostics(const String& uri)
+{
+    String canonicalPath = uriToCanonicalPath(uri);
+    m_pendingModulesToUpdateDiagnostics.remove(canonicalPath);
+}
+
 // When user code has `Foo(123)` where `Foo` is a `struct`, goto-definition on
 // `Foo` should redirect to the constructor of `Foo` instead of the type declaration of `Foo`.
 // This function will check if the `declRefExpr` is a reference to a type declaration,
@@ -601,6 +607,8 @@ SlangResult LanguageServer::hover(
     const JSONValue& responseId)
 {
     auto result = m_core.hover(args);
+    removePendingModuleToUpdateDiagnostics(args.textDocument.uri);
+
     if (SLANG_FAILED(result.returnCode) || result.isNull)
     {
         m_connection->sendResult(NullResponse::get(), responseId);
@@ -614,6 +622,7 @@ LanguageServerResult<LanguageServerProtocol::Hover> LanguageServerCore::hover(
     const LanguageServerProtocol::HoverParams& args)
 {
     String canonicalPath = uriToCanonicalPath(args.textDocument.uri);
+
     RefPtr<DocumentVersion> doc;
     if (!m_workspace->openedDocuments.tryGetValue(canonicalPath, doc))
     {
@@ -982,6 +991,8 @@ SlangResult LanguageServer::gotoDefinition(
     const JSONValue& responseId)
 {
     auto result = m_core.gotoDefinition(args);
+    removePendingModuleToUpdateDiagnostics(args.textDocument.uri);
+
     if (SLANG_FAILED(result.returnCode) || result.isNull)
     {
         m_connection->sendResult(NullResponse::get(), responseId);
@@ -1363,6 +1374,8 @@ SlangResult LanguageServer::semanticTokens(
     const JSONValue& responseId)
 {
     auto result = m_core.semanticTokens(args);
+    removePendingModuleToUpdateDiagnostics(args.textDocument.uri);
+
     if (SLANG_FAILED(result.returnCode) || result.isNull)
     {
         m_connection->sendResult(NullResponse::get(), responseId);
@@ -1536,6 +1549,8 @@ SlangResult LanguageServer::signatureHelp(
     const JSONValue& responseId)
 {
     auto result = m_core.signatureHelp(args);
+    removePendingModuleToUpdateDiagnostics(args.textDocument.uri);
+
     if (SLANG_FAILED(result.returnCode) || result.isNull)
     {
         m_connection->sendResult(NullResponse::get(), responseId);
@@ -1914,6 +1929,8 @@ SlangResult LanguageServer::inlayHint(
     const JSONValue& responseId)
 {
     auto result = m_core.inlayHint(args);
+    removePendingModuleToUpdateDiagnostics(args.textDocument.uri);
+
     if (SLANG_FAILED(result.returnCode) || result.isNull)
     {
         m_connection->sendResult(NullResponse::get(), responseId);
@@ -2121,6 +2138,13 @@ void LanguageServer::publishDiagnostics()
     auto version = m_core.m_workspace->getCurrentVersion();
     SLANG_AST_BUILDER_RAII(version->linkage->getASTBuilder());
 
+    // Make sure modules in pendingSet are being compiled.
+    for (auto canonicalPath : m_pendingModulesToUpdateDiagnostics)
+    {
+        version->getOrLoadModule(canonicalPath);
+    }
+    m_pendingModulesToUpdateDiagnostics.clear();
+
     // Send updates to clear diagnostics for files that no longer have any messages.
     List<String> filesToRemove;
     for (const auto& [filepath, _] : m_lastPublishedDiagnostics)
@@ -2752,6 +2776,18 @@ SlangResult LanguageServer::didChangeTextDocument(const DidChangeTextDocumentPar
 {
     resetDiagnosticUpdateTime();
     auto result = m_core.didChangeTextDocument(args);
+
+    // Register the module path for a diagnostics update.
+    // Note: we don't want to trigger a compile and generate the diagnostics immediately on
+    // every text change. Instead, we want to defer it to when it is the right time for an
+    // update. This is needed to reduce the latency of other requests, such as completion.
+    // For example, when user types `.`, there will be a text change event followed by a
+    // completion request. We don't want to run compile once to get the diagnostics, and then
+    // process the completion request, as this will double the latency to popup the completion
+    // list.
+    String canonicalPath = uriToCanonicalPath(args.textDocument.uri);
+    m_pendingModulesToUpdateDiagnostics.add(canonicalPath);
+
     if (!m_core.m_options.periodicDiagnosticUpdate)
     {
         publishDiagnostics();
@@ -2765,13 +2801,6 @@ SlangResult LanguageServerCore::didChangeTextDocument(const DidChangeTextDocumen
     for (auto change : args.contentChanges)
         m_workspace->changeDoc(canonicalPath, change.range, change.text);
 
-    auto version = m_workspace->getCurrentVersion();
-    Module* parsedModule = version->getOrLoadModule(canonicalPath);
-    if (!parsedModule)
-    {
-        return SLANG_FAIL;
-    }
-
     return SLANG_OK;
 }
 
diff --git a/source/slang/slang-language-server.h b/source/slang/slang-language-server.h
index 31b7114c0..397a93b26 100644
--- a/source/slang/slang-language-server.h
+++ b/source/slang/slang-language-server.h
@@ -191,6 +191,9 @@ public:
     TraceOptions m_traceOptions = TraceOptions::Off;
     std::chrono::time_point<std::chrono::system_clock> m_lastDiagnosticUpdateTime;
     Dictionary<String, String> m_lastPublishedDiagnostics;
+    HashSet<String> m_pendingModulesToUpdateDiagnostics;
+
+    void removePendingModuleToUpdateDiagnostics(const String& uri);
 
     LanguageServer(LanguageServerStartupOptions options)
         : m_core(options)
diff --git a/source/slang/slang-lookup.cpp b/source/slang/slang-lookup.cpp
index a9b472776..0fd66abc3 100644
--- a/source/slang/slang-lookup.cpp
+++ b/source/slang/slang-lookup.cpp
@@ -357,6 +357,32 @@ static Type* _maybeSpecializeSuperType(
     return superType;
 }
 
+void FacetImpl::init(ASTBuilder* astBuilder)
+{
+    if (directness != Facet::Directness::Self)
+    {
+        // Depending on the type of the facet, we may want to specialize the
+        // declRef that we are going to lookup in. If the facet represents
+        // an extension, we should just lookup in the extension decl.
+        //
+        // If the facet is an extension to an interface type, we should
+        // specialize the interface declRef to the concrete type that this
+        // extension applied to.
+        //
+        // If the facet represents an implementation of interface type,
+        // we should also specialize the interface declRef with the concrete
+        // type info.
+        //
+        declRefForMemberLookup =
+            _maybeSpecializeSuperTypeDeclRef(astBuilder, origin.declRef, getType(), subtypeWitness)
+                .as<ContainerDecl>();
+    }
+    else
+    {
+        declRefForMemberLookup = origin.declRef.as<ContainerDecl>();
+    }
+}
+
 static void _lookUpMembersInType(
     ASTBuilder* astBuilder,
     Name* name,
@@ -490,27 +516,9 @@ static void _lookupMembersInSuperTypeFacets(
 
         BreadcrumbInfo* newBreadcrumbs = inBreadcrumbs;
         BreadcrumbInfo subtypeInfo;
-        auto parentDeclRef = containerDeclRef;
+        auto parentDeclRef = facet->declRefForMemberLookup;
         if (facet->directness != Facet::Directness::Self)
         {
-            // Depending on the type of the facet, we may want to specialize the
-            // declRef that we are going to lookup in. If the facet represents
-            // an extension, we should just lookup in the extension decl.
-            //
-            // If the facet is an extension to an interface type, we should
-            // specialize the interface declRef to the concrete type that this
-            // extension applied to.
-            //
-            // If the facet represents an implementation of interface type,
-            // we should also specialize the interface declRef with the concrete
-            // type info.
-            //
-            parentDeclRef = _maybeSpecializeSuperTypeDeclRef(
-                                astBuilder,
-                                containerDeclRef,
-                                facet->getType(),
-                                facet->subtypeWitness)
-                                .as<ContainerDecl>();
             if (as<ThisTypeDecl>(parentDeclRef.getDecl()) && getText(name) == "This")
             {
                 // If we are going looking for `This` in a `ThisType`, we just need to return the
diff --git a/source/slang/slang-parser.cpp b/source/slang/slang-parser.cpp
index 590954bd2..a12f216b1 100644
--- a/source/slang/slang-parser.cpp
+++ b/source/slang/slang-parser.cpp
@@ -4861,6 +4861,31 @@ static void addSpecialGLSLModifiersBasedOnType(Parser* parser, Decl* decl, Modif
         }
     }
 }
+
+static EnumDecl* isUnscopedEnum(Decl* decl)
+{
+    EnumDecl* enumDecl = as<EnumDecl>(decl);
+    if (!enumDecl)
+        return nullptr;
+    for (auto mod : enumDecl->modifiers)
+    {
+        if (as<UnscopedEnumAttribute>(mod))
+        {
+            return enumDecl;
+        }
+        else if (auto uncheckedAttribute = as<UncheckedAttribute>(mod))
+        {
+            // We have to perform an ugly string comparison here, because the attributes
+            // haven't been checked during parsing.
+            if (getText(uncheckedAttribute->keywordName) == "UnscopedEnum")
+            {
+                return enumDecl;
+            }
+        }
+    }
+    return nullptr;
+}
+
 // Finish up work on a declaration that was parsed
 static void CompleteDecl(
     Parser* parser,
@@ -4936,6 +4961,24 @@ static void CompleteDecl(
         {
             // Make sure the decl is properly nested inside its lexical parent
             AddMember(containerDecl, decl);
+
+            // As a special case, if we are adding an unscoped enum to container, we should also
+            // create static const decls for each enum case and add them to the container.
+            if (auto enumDecl = isUnscopedEnum(decl))
+            {
+                for (auto enumCase : enumDecl->getMembersOfType<EnumCaseDecl>())
+                {
+                    auto staticConstDecl = parser->astBuilder->create<LetDecl>();
+                    staticConstDecl->nameAndLoc = enumCase->nameAndLoc;
+                    addModifier(staticConstDecl, parser->astBuilder->create<HLSLStaticModifier>());
+                    addModifier(staticConstDecl, parser->astBuilder->create<ConstModifier>());
+                    auto valueExpr = parser->astBuilder->create<VarExpr>();
+                    valueExpr->declRef = DeclRef<Decl>(enumCase);
+                    staticConstDecl->initExpr = valueExpr;
+                    staticConstDecl->loc = enumCase->loc;
+                    AddMember(containerDecl, staticConstDecl);
+                }
+            }
         }
     }
 
@@ -5556,17 +5599,16 @@ static Decl* parseEnumDecl(Parser* parser)
         decl->nameAndLoc = expectIdentifier(parser);
     }
 
-    // If the type needs to be unscoped, insert modifiers to make it so.
-    if (isUnscoped)
-    {
-        addModifier(decl, parser->astBuilder->create<UnscopedEnumAttribute>());
-        addModifier(decl, parser->astBuilder->create<TransparentModifier>());
-    }
 
     return parseOptGenericDecl(
         parser,
         [&](GenericDecl* genericParent)
         {
+            // If the type needs to be unscoped, insert modifiers to make it so.
+            if (isUnscoped && !genericParent)
+            {
+                addModifier(decl, parser->astBuilder->create<UnscopedEnumAttribute>());
+            }
             parseOptionalInheritanceClause(parser, decl);
             maybeParseGenericConstraints(parser, genericParent);
             parser->ReadToken(TokenType::LBrace);