Avoid moving around large suspended function states in the deferred definition worklist. (carbon-language#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>

Author: 2 people

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