Avoid moving around large suspended function states in the deferred definition worklist.

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