Avoid moving around large suspended function states in the deferred definition worklist. (#5608)

We already go to some effort to avoid moving these, but we end up still
moving them twice: once when adding to the worklist and again when
reversing a chunk of the worklist.

* To avoid a move when constructing the worklist, add an `EmplaceResult`
utility that allows the result of a function call to be emplaced into a
container.
* To avoid moves when reversing the list, stop reversing it. Instead of
reversing the list and popping tasks as we run them, we accumulate a
sequence of tasks for a deferred definition region, run them in the
order they were enqueued, then pop them all at the end. This will in
some cases increase the high-water-mark of the size of the worklist, but
not asymptotically. The same high-water-mark could be reached with the
old approach by reordering the declarations in the source file.

In passing, we no longer create `LeaveDeferredDefinitionRegion` tasks
for non-nested regions. We don't need them, because we can detect that
condition by our reaching the end of the worklist. This means that the
enter / leave region actions are now always in correspondence -- we only
create them for *nested* regions. The tasks have been renamed to convey
this.

We still move the suspended function states around if the worklist grows
to over 64 entries and gets reallocated. We could potentially address
that issue too by switching to a chunked allocation strategy as is used
by `ValueStore` and then make the tasks noncopyable, but I'm not
attempting that in this PR.

---------

Co-authored-by: Dana Jansens <danakj@orodu.net>

common/BUILD

@@ -116,6 +116,21 @@ cc_library(
     hdrs = ["concepts.h"],
 )
 
+cc_library(
+    name = "emplace_by_calling",
+    hdrs = ["emplace_by_calling.h"],
+)
+
+cc_test(
+    name = "emplace_by_calling_test",
+    srcs = ["emplace_by_calling_test.cpp"],
+    deps = [
+        ":emplace_by_calling",
+        "//testing/base:gtest_main",
+        "@googletest//:gtest",
+    ],
+)
+
 cc_library(
     name = "enum_base",
     hdrs = ["enum_base.h"],

common/emplace_by_calling.h

@@ -0,0 +1,66 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+#ifndef CARBON_COMMON_EMPLACE_BY_CALLING_H_
+#define CARBON_COMMON_EMPLACE_BY_CALLING_H_
+
+#include <type_traits>
+#include <utility>
+
+namespace Carbon {
+
+// A utility to use when calling an `emplace` function to emplace the result of
+// a function call. Expected usage is:
+//
+//   my_widget_vec.emplace_back(EmplaceByCalling([&] {
+//     return ConstructAWidget(...);
+//   }));
+//
+// In this example, the result of `ConstructAWidget` will be constructed
+// directly into the new element of `my_widget_vec`, without performing a copy
+// or move.
+//
+// Note that the type of the argument to `emplace_back` is an `EmplaceByCalling`
+// instance, not the type `DestT` stored in the container. When the `DestT`
+// instance is eventually initialized directly from the `EmplaceByCalling`, a
+// conversion function on `EmplaceByCalling` is used that converts to the type
+// `DestT` being emplaced. This `DestT` initialization does not call an
+// additional `DestT` copy or move constructor to initialize the result, and
+// instead initializes it in-place in the container's storage, per the C++17
+// guaranteed copy elision rules. Similarly, within the conversion function, the
+// result is initialized directly by calling `make_fn`, again relying on
+// guaranteed copy elision.
+//
+// Because the make function is called from the conversion function,
+// `EmplaceByCalling` should only be used in contexts where it will be used to
+// initialize a `DestT` object exactly once. This is generally true of `emplace`
+// functions. Also, because the `make_fn` callback will be called after the
+// container has made space for the new element, it should not inspect or modify
+// the container that is being emplaced into.
+template <typename MakeFnT>
+class EmplaceByCalling {
+ public:
+  explicit(false) EmplaceByCalling(MakeFnT make_fn)
+      : make_fn_(std::move(make_fn)) {}
+
+  // Convert to the exact return type of the make function, by calling the make
+  // function to construct the result. No implicit conversions are permitted
+  // here, as that would mean we are not constructing the result in place.
+  template <typename DestT>
+    requires std::same_as<DestT, std::invoke_result_t<MakeFnT&&>>
+  // NOLINTNEXTLINE(google-explicit-constructor)
+  explicit(false) operator DestT() && {
+    return std::move(make_fn_)();
+  }
+
+ private:
+  MakeFnT make_fn_;
+};
+
+template <typename MakeFnT>
+EmplaceByCalling(MakeFnT) -> EmplaceByCalling<MakeFnT>;
+
+}  // namespace Carbon
+
+#endif  // CARBON_COMMON_EMPLACE_BY_CALLING_H_

common/emplace_by_calling_test.cpp

@@ -0,0 +1,62 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+#include "common/emplace_by_calling.h"
+
+#include <gtest/gtest.h>
+
+#include <list>
+
+namespace Carbon {
+namespace {
+
+struct NoncopyableType {
+  NoncopyableType() = default;
+  NoncopyableType(const NoncopyableType&) = delete;
+  auto operator=(const NoncopyableType&) -> NoncopyableType& = delete;
+};
+
+auto Make() -> NoncopyableType { return NoncopyableType(); }
+
+TEST(EmplaceByCalling, Noncopyable) {
+  std::list<NoncopyableType> list;
+  // This should compile.
+  list.emplace_back(EmplaceByCalling(Make));
+}
+
+TEST(EmplaceByCalling, NoncopyableInAggregate) {
+  struct Aggregate {
+    int a, b, c;
+    NoncopyableType noncopyable;
+  };
+
+  std::list<Aggregate> list;
+  // This should compile.
+  list.emplace_back(EmplaceByCalling(
+      [] { return Aggregate{.a = 1, .b = 2, .c = 3, .noncopyable = Make()}; }));
+}
+
+class CopyCounter {
+ public:
+  explicit CopyCounter(int* counter) : counter_(counter) {}
+  CopyCounter(const CopyCounter& other) : counter_(other.counter_) {
+    ++*counter_;
+  }
+
+ private:
+  int* counter_;
+};
+
+TEST(EmplaceByCalling, NoCopies) {
+  std::vector<CopyCounter> vec;
+  vec.reserve(10);
+  int copies = 0;
+  for (int i = 0; i != 10; ++i) {
+    vec.emplace_back(EmplaceByCalling([&] { return CopyCounter(&copies); }));
+  }
+  EXPECT_EQ(0, copies);
+}
+
+}  // namespace
+}  // namespace Carbon

toolchain/check/BUILD

@@ -177,6 +177,7 @@ cc_library(
         ":diagnostic_emitter",
         ":dump",
         "//common:check",
+        "//common:emplace_by_calling",
         "//common:error",
         "//common:find",
         "//common:map",

toolchain/check/deferred_definition_worklist.cpp

@@ -6,7 +6,9 @@
 
 #include <algorithm>
 #include <optional>
+#include <variant>
 
+#include "common/emplace_by_calling.h"
 #include "common/vlog.h"
 #include "toolchain/base/kind_switch.h"
 #include "toolchain/check/handle.h"
@@ -27,8 +29,10 @@ auto DeferredDefinitionWorklist::SuspendFunctionAndPush(
     Parse::FunctionDefinitionStartId node_id) -> void {
   // TODO: Investigate factoring out `HandleFunctionDefinitionSuspend` to make
   // `DeferredDefinitionWorklist` reusable.
-  worklist_.push_back(CheckSkippedDefinition{
-      index, HandleFunctionDefinitionSuspend(context, node_id)});
+  worklist_.emplace_back(EmplaceByCalling([&] {
+    return CheckSkippedDefinition{
+        index, HandleFunctionDefinitionSuspend(context, node_id)};
+  }));
   CARBON_VLOG("{0}Push CheckSkippedDefinition {1}\n", VlogPrefix, index.index);
 }
 
@@ -37,96 +41,68 @@ auto DeferredDefinitionWorklist::PushEnterDeferredDefinitionScope(
   bool nested = !entered_scopes_.empty() &&
                 entered_scopes_.back().scope_index ==
                     context.decl_name_stack().PeekInitialScopeIndex();
-  entered_scopes_.push_back({.worklist_start_index = worklist_.size(),
+  entered_scopes_.push_back({.nested = nested,
+                             .worklist_start_index = worklist_.size(),
                              .scope_index = context.scope_stack().PeekIndex()});
-  worklist_.push_back(EnterDeferredDefinitionScope{
-      .suspended_name = std::nullopt, .in_deferred_definition_scope = nested});
-  CARBON_VLOG("{0}Push EnterDeferredDefinitionScope {1}\n", VlogPrefix,
-              nested ? "(nested)" : "(non-nested)");
+  if (nested) {
+    worklist_.emplace_back(EmplaceByCalling([&] {
+      return EnterNestedDeferredDefinitionScope{.suspended_name = std::nullopt};
+    }));
+    CARBON_VLOG("{0}Push EnterDeferredDefinitionScope (nested)\n", VlogPrefix);
+  } else {
+    // Don't push a task to re-enter a non-nested scope. Instead,
+    // SuspendFinishedScopeAndPush will remain in the scope when executing the
+    // worklist tasks.
+    CARBON_VLOG("{0}Entered non-nested deferred definition scope\n",
+                VlogPrefix);
+  }
   return !nested;
 }
 
 auto DeferredDefinitionWorklist::SuspendFinishedScopeAndPush(Context& context)
     -> FinishedScopeKind {
-  auto start_index = entered_scopes_.pop_back_val().worklist_start_index;
+  auto [nested, start_index, _] = entered_scopes_.pop_back_val();
 
-  // If we've not found any deferred definitions in this scope, clean up the
-  // stack.
-  if (start_index == worklist_.size() - 1) {
+  // If we've not found any tasks to perform in this scope, clean up the stack.
+  // For non-nested scope, there will be no tasks on the worklist for this scope
+  // in this case; for a nested scope, there will just be a task to re-enter the
+  // nested scope.
+  if (!nested && start_index == worklist_.size()) {
     context.decl_name_stack().PopScope();
-    auto enter_scope =
-        get<EnterDeferredDefinitionScope>(worklist_.pop_back_val());
-    CARBON_VLOG("{0}Pop EnterDeferredDefinitionScope (empty)\n", VlogPrefix);
-    return enter_scope.in_deferred_definition_scope
-               ? FinishedScopeKind::Nested
-               : FinishedScopeKind::NonNestedEmpty;
+    CARBON_VLOG("{0}Left non-nested empty deferred definition scope\n",
+                VlogPrefix);
+    return FinishedScopeKind::NonNestedEmpty;
+  }
+  if (nested && start_index == worklist_.size() - 1) {
+    CARBON_CHECK(std::holds_alternative<EnterNestedDeferredDefinitionScope>(
+        worklist_.back()));
+    worklist_.pop_back();
+    context.decl_name_stack().PopScope();
+    CARBON_VLOG("{0}Pop EnterNestedDeferredDefinitionScope (empty)\n",
+                VlogPrefix);
+    return FinishedScopeKind::Nested;
   }
 
   // If we're finishing a nested deferred definition scope, keep track of that
   // but don't type-check deferred definitions now.
-  if (auto& enter_scope =
-          get<EnterDeferredDefinitionScope>(worklist_[start_index]);
-      enter_scope.in_deferred_definition_scope) {
+  if (nested) {
+    auto& enter_scope =
+        get<EnterNestedDeferredDefinitionScope>(worklist_[start_index]);
     // This is a nested deferred definition scope. Suspend the inner scope so we
     // can restore it when we come to type-check the deferred definitions.
-    enter_scope.suspended_name = context.decl_name_stack().Suspend();
+    enter_scope.suspended_name.emplace(
+        EmplaceByCalling([&] { return context.decl_name_stack().Suspend(); }));
 
     // Enqueue a task to leave the nested scope.
-    worklist_.push_back(
-        LeaveDeferredDefinitionScope{.in_deferred_definition_scope = true});
-    CARBON_VLOG("{0}Push LeaveDeferredDefinitionScope (nested)\n", VlogPrefix);
+    worklist_.emplace_back(LeaveNestedDeferredDefinitionScope{});
+    CARBON_VLOG("{0}Push LeaveNestedDeferredDefinitionScope\n", VlogPrefix);
     return FinishedScopeKind::Nested;
   }
 
-  // We're at the end of a non-nested deferred definition scope. Prepare to
-  // start checking deferred definitions. Enqueue a task to leave this outer
-  // scope and end checking deferred definitions.
-  worklist_.push_back(
-      LeaveDeferredDefinitionScope{.in_deferred_definition_scope = false});
-  CARBON_VLOG("{0}Push LeaveDeferredDefinitionScope (non-nested)\n",
-              VlogPrefix);
-
-  // We'll process the worklist in reverse index order, so reverse the part of
-  // it we're about to execute so we run our tasks in the order in which they
-  // were pushed.
-  std::reverse(worklist_.begin() + start_index, worklist_.end());
-
-  // Pop the `EnterDeferredDefinitionScope` that's now on the end of the
-  // worklist. We stay in that scope rather than suspending then immediately
-  // resuming it.
-  CARBON_CHECK(
-      holds_alternative<EnterDeferredDefinitionScope>(worklist_.back()),
-      "Unexpected task in worklist.");
-  worklist_.pop_back();
-  CARBON_VLOG("{0}Handle EnterDeferredDefinitionScope (non-nested)\n",
-              VlogPrefix);
+  // We're at the end of a non-nested deferred definition scope. Start checking
+  // deferred definitions.
+  CARBON_VLOG("{0}Starting deferred definition processing\n", VlogPrefix);
   return FinishedScopeKind::NonNestedWithWork;
 }
 
-auto DeferredDefinitionWorklist::Pop(
-    llvm::function_ref<auto(Task&&)->void> handle_fn) -> void {
-  if (vlog_stream_) {
-    CARBON_KIND_SWITCH(worklist_.back()) {
-      case CARBON_KIND(const CheckSkippedDefinition& definition):
-        CARBON_VLOG("{0}Handle CheckSkippedDefinition {1}\n", VlogPrefix,
-                    definition.definition_index.index);
-        break;
-      case CARBON_KIND(const EnterDeferredDefinitionScope& enter):
-        CARBON_CHECK(enter.in_deferred_definition_scope);
-        CARBON_VLOG("{0}Handle EnterDeferredDefinitionScope (nested)\n",
-                    VlogPrefix);
-        break;
-      case CARBON_KIND(const LeaveDeferredDefinitionScope& leave): {
-        bool nested = leave.in_deferred_definition_scope;
-        CARBON_VLOG("{0}Handle LeaveDeferredDefinitionScope {1}\n", VlogPrefix,
-                    nested ? "(nested)" : "(non-nested)");
-        break;
-      }
-    }
-  }
-
-  handle_fn(std::move(worklist_.back()));
-  worklist_.pop_back();
-}
-
 }  // namespace Carbon::Check

toolchain/check/deferred_definition_worklist.h

@@ -30,40 +30,35 @@ class DeferredDefinitionWorklist {
   };
 
   // A worklist task that indicates we should enter a nested deferred definition
-  // scope.
-  struct EnterDeferredDefinitionScope {
+  // scope. We delay processing the contents of nested deferred definition
+  // scopes until we reach the end of the parent scope. For example:
+  //
+  // ```
+  // class A {
+  //   class B {
+  //     fn F() -> A { return {}; }
+  //   }
+  // } // A.B.F is type-checked here, with A complete.
+  //
+  // fn F() {
+  //   class C {
+  //     fn G() {}
+  //   } // C.G is type-checked here.
+  // }
+  // ```
+  struct EnterNestedDeferredDefinitionScope {
     // The suspended scope. This is only set once we reach the end of the scope.
     std::optional<DeclNameStack::SuspendedName> suspended_name;
-    // Whether this scope is itself within an outer deferred definition scope.
-    // If so, we'll delay processing its contents until we reach the end of the
-    // parent scope. For example:
-    //
-    // ```
-    // class A {
-    //   class B {
-    //     fn F() -> A { return {}; }
-    //   }
-    // } // A.B.F is type-checked here, with A complete.
-    //
-    // fn F() {
-    //   class C {
-    //     fn G() {}
-    //   } // C.G is type-checked here.
-    // }
-    // ```
-    bool in_deferred_definition_scope;
   };
 
-  // A worklist task that indicates we should leave a deferred definition scope.
-  struct LeaveDeferredDefinitionScope {
-    // Whether this scope is within another deferred definition scope.
-    bool in_deferred_definition_scope;
-  };
+  // A worklist task that indicates we should leave a nested deferred definition
+  // scope.
+  struct LeaveNestedDeferredDefinitionScope {};
 
   // A pending type-checking task.
   using Task =
-      std::variant<CheckSkippedDefinition, EnterDeferredDefinitionScope,
-                   LeaveDeferredDefinitionScope>;
+      std::variant<CheckSkippedDefinition, EnterNestedDeferredDefinitionScope,
+                   LeaveNestedDeferredDefinitionScope>;
 
   explicit DeferredDefinitionWorklist(llvm::raw_ostream* vlog_stream);
 
@@ -94,8 +89,14 @@ class DeferredDefinitionWorklist {
   // deferred definitions should be type-checked immediately.
   auto SuspendFinishedScopeAndPush(Context& context) -> FinishedScopeKind;
 
-  // Pop and handle the next task on the worklist.
-  auto Pop(llvm::function_ref<auto(Task&&)->void> handle_fn) -> void;
+  // Returns the current size of the worklist.
+  auto size() const -> size_t { return worklist_.size(); }
+
+  // Truncates the worklist to the given size.
+  auto truncate(int new_size) -> void { worklist_.truncate(new_size); }
+
+  // Gets the given item on the worklist.
+  auto operator[](int index) -> Task& { return worklist_[index]; }
 
   // CHECK that the work list has no further work.
   auto VerifyEmpty() {
@@ -106,7 +107,11 @@ class DeferredDefinitionWorklist {
  private:
   // A deferred definition scope that is currently still open.
   struct EnteredScope {
-    // The index in worklist_ of the EnterDeferredDefinitionScope task.
+    // Whether this scope is nested immediately within the enclosing scope. If
+    // so, deferred definitions are not processed at the end of this scope.
+    bool nested;
+    // The index in worklist_ of the first task in this scope. For a nested
+    // scope, this is a EnterNestedDeferredDefinitionScope task.
     size_t worklist_start_index;
     // The corresponding lexical scope index.
     ScopeIndex scope_index;

toolchain/check/node_id_traversal.cpp

@@ -31,14 +31,22 @@ auto NodeIdTraversal::Next() -> std::optional<Parse::NodeId> {
     // should check, restore its suspended state, and add a corresponding
     // `Chunk` to the top of the chunk list.
     if (chunks_.back().checking_deferred_definitions) {
-      worklist_.Pop([&](DeferredDefinitionWorklist::Task&& task) {
-        std::visit(
-            [&](auto&& task) {
-              PerformTask(std::forward<decltype(task)>(task));
-            },
-            std::move(task));
-      });
-      continue;
+      if (chunks_.back().next_worklist_index < worklist_.size()) {
+        std::visit([&](auto& task) { PerformTask(std::move(task)); },
+                   worklist_[chunks_.back().next_worklist_index++]);
+        continue;
+      }
+
+      // Worklist is empty: discard the worklist items associated with this
+      // chunk, and leave the scope.
+      worklist_.truncate(chunks_.back().first_worklist_index);
+      // We reach here when
+      // `DeferredDefinitionScope::SuspendFinishedScopeAndPush` returns
+      // `NonNestedWithWork`. In this case it's our responsibility to pop the
+      // scope left behind by the `Handle*Definition` function for the
+      // non-nested definition.
+      context_->decl_name_stack().PopScope();
+      chunks_.back().checking_deferred_definitions = false;
     }
 
     // If we're not checking deferred definitions, produce the next parse node
@@ -143,23 +151,22 @@ auto NodeIdTraversal::Handle(Parse::NodeKind parse_kind) -> void {
     if (scope_kind ==
         DeferredDefinitionWorklist::FinishedScopeKind::NonNestedWithWork) {
       chunks_.back().checking_deferred_definitions = true;
+      chunks_.back().next_worklist_index = chunks_.back().first_worklist_index;
     }
   }
 }
 
 auto NodeIdTraversal::PerformTask(
-    DeferredDefinitionWorklist::EnterDeferredDefinitionScope&& enter) -> void {
+    DeferredDefinitionWorklist::EnterNestedDeferredDefinitionScope&& enter)
+    -> void {
   CARBON_CHECK(enter.suspended_name,
                "Entering a scope with no suspension information.");
   context_->decl_name_stack().Restore(std::move(*enter.suspended_name));
 }
 
 auto NodeIdTraversal::PerformTask(
-    DeferredDefinitionWorklist::LeaveDeferredDefinitionScope&& leave) -> void {
-  if (!leave.in_deferred_definition_scope) {
-    // We're done with checking deferred definitions.
-    chunks_.back().checking_deferred_definitions = false;
-  }
+    DeferredDefinitionWorklist::LeaveNestedDeferredDefinitionScope&& /*leave*/)
+    -> void {
   context_->decl_name_stack().PopScope();
 }
 
@@ -175,7 +182,10 @@ auto NodeIdTraversal::PerformTask(
       definition_info.start_id, definition_info.definition_id);
   chunks_.push_back({.it = range.begin() + 1,
                      .end = range.end(),
-                     .next_definition = next_deferred_definition_.index()});
+                     .next_definition = next_deferred_definition_.index(),
+                     .checking_deferred_definitions = false,
+                     .first_worklist_index = worklist_.size(),
+                     .next_worklist_index = worklist_.size()});
   ++definition_index.index;
   next_deferred_definition_.SkipTo(definition_index);
 }

toolchain/check/node_id_traversal.h

@@ -62,15 +62,23 @@ class NodeIdTraversal {
     // them until we're done with this batch of deferred definitions. Otherwise,
     // we'll pull node IDs from `*it` until it reaches `end`.
     bool checking_deferred_definitions = false;
+    // If we're checking deferred definitions, the index of the first task to
+    // execute from `worklist`.
+    size_t first_worklist_index;
+    // If we're checking deferred definitions, the index of the next task to
+    // execute from `worklist`.
+    size_t next_worklist_index;
   };
 
   // Re-enter a nested deferred definition scope.
   auto PerformTask(
-      DeferredDefinitionWorklist::EnterDeferredDefinitionScope&& enter) -> void;
+      DeferredDefinitionWorklist::EnterNestedDeferredDefinitionScope&& enter)
+      -> void;
 
-  // Leave a nested or top-level deferred definition scope.
+  // Leave a nested deferred definition scope.
   auto PerformTask(
-      DeferredDefinitionWorklist::LeaveDeferredDefinitionScope&& leave) -> void;
+      DeferredDefinitionWorklist::LeaveNestedDeferredDefinitionScope&& leave)
+      -> void;
 
   // Resume checking a deferred definition.
   auto PerformTask(