[embedded handlers] Store the handlers without gaps

Previously the builtins table had a value for every single
OperandScale/Bytecode combination regardless of whether it was valid.
This change makes it so that only valid bytecode handlers are stored in
the builtins table. This prevents placeholders being serialized into the
snapshot (and embedded into the binary) saving 9KB in
CODE_SPACE/OLD_SPACE and 2.5KB in the embedded data as well as 66
entries in the builtins table.

To do this, it generates a new header file bytecodes-builtins-list.h
which is created from the BYTECODE_LIST and OPERAND_SCALE_LIST macros.
Since list macros cannot be used to conditionally generate elements in
the C-preprocessor, this is done by generator executable, compiled from
interpreter/generate-flat-headers.cc.

Additionally the generator creates the flat bytecode list so that it is
transposed from the previous result, i.e. the results are grouped by
bytecode and then operand scale rather than operand scale then bytecode.
This should give better locality for commonly used bytecodes and may
allow less commonly used ExtraWide bytecodes to never be mapped into
memory at all.

The cost to storing the handlers densely is that looking up a handler
now requires a binary search through the builtins table, but this should
only happen during debugging. It is also fixable at least for non-wide
handlers and could be improved for wide ones if the need arises.

Bug: v8:8068
Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
Change-Id: Iaad22a952e2858f508030c5ddc082f91bf59f667
Reviewed-on: https://chromium-review.googlesource.com/1209304
Commit-Queue: Dan Elphick <delphick@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#55757}

BUILD.gn

@@ -73,7 +73,7 @@ declare_args() {
 
   # Enable embedded builtins.
   # TODO(jgruber,v8:6666): Support ia32 and maybe MSVC.
-  v8_enable_embedded_builtins = v8_use_snapshot && v8_current_cpu != "x86" &&
+  v8_enable_embedded_builtins = v8_use_snapshot && v8_target_cpu != "x86" &&
                                 !is_aix && (!is_win || is_clang)
 
   # Enable code-generation-time checking of types in the CodeStubAssembler.
@@ -966,6 +966,7 @@ v8_source_set("torque_generated_initializers") {
   visibility = [ ":*" ]  # Only targets in this file can depend on this.
 
   deps = [
+    ":generate_bytecode_builtins_list",
     ":run_torque",
   ]
 
@@ -986,6 +987,24 @@ v8_source_set("torque_generated_initializers") {
   configs = [ ":internal_config" ]
 }
 
+action("generate_bytecode_builtins_list") {
+  script = "tools/run.py"
+  outputs = [
+    "$target_gen_dir/builtins-generated/bytecodes-builtins-list.h",
+  ]
+  deps = [
+    ":bytecode_builtins_list_generator($host_toolchain)",
+  ]
+  args = [
+    "./" + rebase_path(
+            get_label_info(":bytecode_builtins_list_generator($host_toolchain)",
+                           "root_out_dir") +
+                "/bytecode_builtins_list_generator",
+            root_build_dir),
+    rebase_path("$target_gen_dir/builtins-generated/bytecodes-builtins-list.h"),
+  ]
+}
+
 # Template to generate different V8 snapshots based on different runtime flags.
 # Can be invoked with run_mksnapshot(<name>). The target will resolve to
 # run_mksnapshot_<name>. If <name> is "default", no file suffixes will be used.
@@ -1502,6 +1521,7 @@ v8_source_set("v8_base") {
     "//base/trace_event/common/trace_event_common.h",
 
     ### gcmole(all) ###
+    "$target_gen_dir/builtins-generated/bytecodes-builtins-list.h",
     "include/v8-inspector-protocol.h",
     "include/v8-inspector.h",
     "include/v8-internal.h",
@@ -2882,6 +2902,7 @@ v8_source_set("v8_base") {
 
   defines = []
   deps = [
+    ":generate_bytecode_builtins_list",
     ":torque_generated_core",
     ":v8_headers",
     ":v8_libbase",
@@ -3263,6 +3284,31 @@ if (v8_monolithic) {
 # Executables
 #
 
+if (current_toolchain == host_toolchain) {
+  executable("bytecode_builtins_list_generator") {
+    visibility = [ ":*" ]  # Only targets in this file can depend on this.
+
+    include_dirs = [ "." ]
+
+    sources = [
+      "src/builtins/generate-bytecodes-builtins-list.cc",
+      "src/interpreter/bytecode-operands.cc",
+      "src/interpreter/bytecode-operands.h",
+      "src/interpreter/bytecodes.cc",
+      "src/interpreter/bytecodes.h",
+    ]
+
+    if (v8_enable_embedded_builtins) {
+      defines = [ "V8_EMBEDDED_BUILTINS" ]
+    }
+
+    deps = [
+      ":v8_libbase",
+      "//build/win:default_exe_manifest",
+    ]
+  }
+}
+
 if (v8_use_snapshot && current_toolchain == v8_snapshot_toolchain) {
   v8_executable("mksnapshot") {
     visibility = [ ":*" ]  # Only targets in this file can depend on this.
@@ -3662,6 +3708,7 @@ v8_source_set("wasm_module_runner") {
   ]
 
   deps = [
+    ":generate_bytecode_builtins_list",
     ":torque_generated_core",
   ]
 
@@ -3745,6 +3792,7 @@ v8_source_set("lib_wasm_fuzzer_common") {
   ]
 
   deps = [
+    ":generate_bytecode_builtins_list",
     ":torque_generated_core",
   ]
 

src/DEPS

@@ -30,6 +30,7 @@ include_rules = [
   "+testing/gtest/include/gtest/gtest_prod.h",
   "-src/libplatform",
   "-include/libplatform",
+  "+builtins-generated",
   "+torque-generated"
 ]
 

src/builtins/builtins-definitions.h

@@ -5,7 +5,7 @@
 #ifndef V8_BUILTINS_BUILTINS_DEFINITIONS_H_
 #define V8_BUILTINS_BUILTINS_DEFINITIONS_H_
 
-#include "src/interpreter/bytecodes.h"
+#include "builtins-generated/bytecodes-builtins-list.h"
 
 // include generated header
 #include "torque-generated/builtin-definitions-from-dsl.h"
@@ -26,7 +26,7 @@ namespace internal {
 // TFH: Handlers in Turbofan, with CodeStub linkage.
 //      Args: name, interface descriptor
 // BCH: Bytecode Handlers, with bytecode dispatch linkage.
-//      Args: name
+//      Args: name, Bytecode, OperandScale
 // ASM: Builtin in platform-dependent assembly.
 //      Args: name
 
@@ -1443,12 +1443,6 @@ namespace internal {
   CPP(StringPrototypeToUpperCase)
 #endif  // V8_INTL_SUPPORT
 
-#ifdef V8_EMBEDDED_BYTECODE_HANDLERS
-#define BUILTIN_LIST_BYTECODE_HANDLERS(BCH) BYTECODE_LIST(BCH)
-#else
-#define BUILTIN_LIST_BYTECODE_HANDLERS(BCH)
-#endif  // V8_EMBEDDED_BYTECODE_HANDLERS
-
 #define BUILTIN_LIST(CPP, API, TFJ, TFC, TFS, TFH, BCH, ASM) \
   BUILTIN_LIST_BASE(CPP, API, TFJ, TFC, TFS, TFH, ASM)       \
   BUILTIN_LIST_FROM_DSL(CPP, API, TFJ, TFC, TFS, TFH, ASM)   \

src/builtins/builtins.cc

@@ -51,9 +51,7 @@ struct BuiltinMetadata {
 #define DECL_TFC(Name, ...) { #Name, Builtins::TFC, {} },
 #define DECL_TFS(Name, ...) { #Name, Builtins::TFS, {} },
 #define DECL_TFH(Name, ...) { #Name, Builtins::TFH, {} },
-#define DECL_BCH(Name, ...) { #Name "Handler", Builtins::BCH, {} }, \
-                            { #Name "WideHandler", Builtins::BCH, {} }, \
-                            { #Name "ExtraWideHandler", Builtins::BCH, {} },
+#define DECL_BCH(Name, ...) { #Name, Builtins::BCH, {} },
 #define DECL_ASM(Name, ...) { #Name, Builtins::ASM, {} },
 const BuiltinMetadata builtin_metadata[] = {
   BUILTIN_LIST(DECL_CPP, DECL_API, DECL_TFJ, DECL_TFC, DECL_TFS, DECL_TFH,
@@ -147,17 +145,6 @@ Handle<Code> Builtins::builtin_handle(int index) {
       reinterpret_cast<Code**>(isolate_->heap()->builtin_address(index)));
 }
 
-#ifdef V8_EMBEDDED_BYTECODE_HANDLERS
-Code* Builtins::GetBytecodeHandler(interpreter::Bytecode bytecode,
-                                   interpreter::OperandScale operand_scale) {
-  return builtin(
-      kFirstBytecodeHandler +
-      static_cast<int>(bytecode) *
-          interpreter::BytecodeOperands::kOperandScaleCount +
-      interpreter::BytecodeOperands::OperandScaleAsIndex(operand_scale));
-}
-#endif
-
 // static
 int Builtins::GetStackParameterCount(Name name) {
   DCHECK(Builtins::KindOf(name) == TFJ);

src/builtins/builtins.h

@@ -45,16 +45,13 @@ class Builtins {
 
   enum Name : int32_t {
 #define DEF_ENUM(Name, ...) k##Name,
-#define DEF_ENUM_BYTECODE_HANDLER(Name, ...) \
-  k##Name##Handler, k##Name##WideHandler, k##Name##ExtraWideHandler,
     BUILTIN_LIST(DEF_ENUM, DEF_ENUM, DEF_ENUM, DEF_ENUM, DEF_ENUM, DEF_ENUM,
-                 DEF_ENUM_BYTECODE_HANDLER, DEF_ENUM)
+                 DEF_ENUM, DEF_ENUM)
 #undef DEF_ENUM
-#undef DEF_ENUM_BYTECODE_HANDLER
         builtin_count,
 
 #ifdef V8_EMBEDDED_BYTECODE_HANDLERS
-#define EXTRACT_NAME(Name, ...) k##Name##Handler,
+#define EXTRACT_NAME(Name, ...) k##Name,
     // Define kFirstBytecodeHandler,
     kFirstBytecodeHandler =
         FirstFromVarArgs(BUILTIN_LIST_BYTECODE_HANDLERS(EXTRACT_NAME) 0)
@@ -92,11 +89,6 @@ class Builtins {
   Code* builtin(int index);
   V8_EXPORT_PRIVATE Handle<Code> builtin_handle(int index);
 
-#ifdef V8_EMBEDDED_BYTECODE_HANDLERS
-  Code* GetBytecodeHandler(interpreter::Bytecode bytecode,
-                           interpreter::OperandScale operand_scale);
-#endif  // V8_EMBEDDED_BYTECODE_HANDLERS
-
   V8_EXPORT_PRIVATE static Callable CallableFor(Isolate* isolate, Name name);
 
   static int GetStackParameterCount(Name name);

src/builtins/generate-bytecodes-builtins-list.cc

+// Copyright 2018 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <fstream>
+#include <iostream>
+
+#include "src/interpreter/bytecodes.h"
+
+namespace v8 {
+namespace internal {
+namespace interpreter {
+
+void WriteBytecode(std::ofstream& out, Bytecode bytecode,
+                   OperandScale operand_scale) {
+  if (Bytecodes::BytecodeHasHandler(bytecode, operand_scale)) {
+    out << " \\\n  V(" << Bytecodes::ToString(bytecode, operand_scale, "")
+        << "Handler, interpreter::Bytecode::k" << Bytecodes::ToString(bytecode)
+        << ", interpreter::OperandScale::k" << operand_scale << ")";
+  }
+}
+
+void WriteHeader(const char* header_filename) {
+  std::ofstream out(header_filename);
+
+  out << "// Automatically generated from interpreter/bytecodes.h\n"
+      << "// The following list macro is used to populate the builtins list\n"
+      << "// with the bytecode handlers\n\n"
+      << "#ifndef V8_BUILTINS_GENERATED_BYTECODES_BUILTINS_LIST\n"
+      << "#define V8_BUILTINS_GENERATED_BYTECODES_BUILTINS_LIST\n"
+      << "#define BUILTIN_LIST_BYTECODE_HANDLERS(V)";
+#ifdef V8_EMBEDDED_BUILTINS
+#define ADD_BYTECODES(Name, ...) \
+  WriteBytecode(out, Bytecode::k##Name, operand_scale);
+  OperandScale operand_scale = OperandScale::kSingle;
+  BYTECODE_LIST(ADD_BYTECODES)
+  operand_scale = OperandScale::kDouble;
+  BYTECODE_LIST(ADD_BYTECODES)
+  operand_scale = OperandScale::kQuadruple;
+  BYTECODE_LIST(ADD_BYTECODES)
+#undef ADD_BYTECODES
+#endif
+  out << "\n#endif  // V8_BUILTINS_GENERATED_BYTECODES_BUILTINS_LIST\n";
+}
+
+}  // namespace interpreter
+}  // namespace internal
+}  // namespace v8
+
+int main(int argc, const char* argv[]) {
+  if (argc != 2) {
+    std::cerr << "Usage: " << argv[0] << " <output filename>\n";
+    std::exit(1);
+  }
+
+  v8::internal::interpreter::WriteHeader(argv[1]);
+
+  return 0;
+}

src/builtins/setup-builtins-internal.cc

@@ -252,11 +252,7 @@ namespace {
 Code* GenerateBytecodeHandler(Isolate* isolate, int builtin_index,
                               const char* name, interpreter::Bytecode bytecode,
                               interpreter::OperandScale operand_scale) {
-  if (!interpreter::Bytecodes::BytecodeHasHandler(bytecode, operand_scale)) {
-    // TODO(v8:8068): Consider returning something else to avoid placeholders
-    // being serialized with the snapshot.
-    return nullptr;
-  }
+  DCHECK(interpreter::Bytecodes::BytecodeHasHandler(bytecode, operand_scale));
 
   Handle<Code> code = interpreter::GenerateBytecodeHandler(
       isolate, bytecode, operand_scale, builtin_index,
@@ -312,19 +308,10 @@ void SetupIsolateDelegate::SetupBuiltinsInternal(Isolate* isolate) {
   AddBuiltin(builtins, index++, code);
 
 #ifdef V8_EMBEDDED_BYTECODE_HANDLERS
-#define BUILD_BCH_WITH_SCALE(Code, Scale)                               \
+#define BUILD_BCH(Code, Bytecode, OperandScale)                         \
   code = GenerateBytecodeHandler(isolate, index, Builtins::name(index), \
-                                 interpreter::Bytecode::k##Code,        \
-                                 interpreter::OperandScale::k##Scale);  \
-  if (code) {                                                           \
-    AddBuiltin(builtins, index, code);                                  \
-  }                                                                     \
-  ++index;
-
-#define BUILD_BCH(Code, ...)         \
-  BUILD_BCH_WITH_SCALE(Code, Single) \
-  BUILD_BCH_WITH_SCALE(Code, Double) \
-  BUILD_BCH_WITH_SCALE(Code, Quadruple)
+                                 Bytecode, OperandScale);               \
+  AddBuiltin(builtins, index++, code);
 #else
 #define BUILD_BCH(Code, ...) UNREACHABLE();
 #endif  // V8_EMBEDDED_BYTECODE_HANDLERS
@@ -344,7 +331,6 @@ void SetupIsolateDelegate::SetupBuiltinsInternal(Isolate* isolate) {
 #undef BUILD_TFS
 #undef BUILD_TFH
 #undef BUILD_BCH
-#undef BUILD_BCH_WITH_SCALE
 #undef BUILD_ASM
   CHECK_EQ(Builtins::builtin_count, index);
 

src/interpreter/bytecodes.cc

@@ -107,14 +107,13 @@ const char* Bytecodes::ToString(Bytecode bytecode) {
 }
 
 // static
-std::string Bytecodes::ToString(Bytecode bytecode, OperandScale operand_scale) {
-  static const char kSeparator = '.';
-
+std::string Bytecodes::ToString(Bytecode bytecode, OperandScale operand_scale,
+                                const char* separator) {
   std::string value(ToString(bytecode));
   if (operand_scale > OperandScale::kSingle) {
     Bytecode prefix_bytecode = OperandScaleToPrefixBytecode(operand_scale);
     std::string suffix = ToString(prefix_bytecode);
-    return value.append(1, kSeparator).append(suffix);
+    return value.append(separator).append(suffix);
   } else {
     return value;
   }

src/interpreter/bytecodes.h

@@ -469,8 +469,10 @@ class V8_EXPORT_PRIVATE Bytecodes final : public AllStatic {
   // Returns string representation of |bytecode|.
   static const char* ToString(Bytecode bytecode);
 
-  // Returns string representation of |bytecode|.
-  static std::string ToString(Bytecode bytecode, OperandScale operand_scale);
+  // Returns string representation of |bytecode| combined with |operand_scale|
+  // using the optionally provided |separator|.
+  static std::string ToString(Bytecode bytecode, OperandScale operand_scale,
+                              const char* separator = ".");
 
   // Returns byte value of bytecode.
   static uint8_t ToByte(Bytecode bytecode) {

src/interpreter/interpreter.cc

@@ -61,14 +61,14 @@ Interpreter::Interpreter(Isolate* isolate) : isolate_(isolate) {
 
 Code* Interpreter::GetAndMaybeDeserializeBytecodeHandler(
     Bytecode bytecode, OperandScale operand_scale) {
+#ifdef V8_EMBEDDED_BYTECODE_HANDLERS
+  return GetBytecodeHandler(bytecode, operand_scale);
+#else
   Code* code = GetBytecodeHandler(bytecode, operand_scale);
 
   // Already deserialized? Then just return the handler.
   if (!isolate_->heap()->IsDeserializeLazyHandler(code)) return code;
 
-#ifdef V8_EMBEDDED_BYTECODE_HANDLERS
-  UNREACHABLE();
-#else
   DCHECK(FLAG_lazy_handler_deserialization);
   DCHECK(Bytecodes::BytecodeHasHandler(bytecode, operand_scale));
   code = Snapshot::DeserializeHandler(isolate_, bytecode, operand_scale);
@@ -86,9 +86,11 @@ Code* Interpreter::GetAndMaybeDeserializeBytecodeHandler(
 Code* Interpreter::GetBytecodeHandler(Bytecode bytecode,
                                       OperandScale operand_scale) {
 #ifdef V8_EMBEDDED_BYTECODE_HANDLERS
-  // The dispatch table has pointers to the offheap instruction stream, so it's
-  // not possible to use that to get hold of the Code object.
-  return isolate_->builtins()->GetBytecodeHandler(bytecode, operand_scale);
+  size_t index = GetDispatchTableIndex(bytecode, operand_scale);
+  Address pc = dispatch_table_[index];
+  Code* builtin = InstructionStream::TryLookupCode(isolate_, pc);
+  DCHECK(builtin->IsCode());
+  return builtin;
 #else
   DCHECK(IsDispatchTableInitialized());
   DCHECK(Bytecodes::BytecodeHasHandler(bytecode, operand_scale));
@@ -253,12 +255,21 @@ void Interpreter::InitializeDispatchTable() {
 #ifdef V8_EMBEDDED_BYTECODE_HANDLERS
   Builtins* builtins = isolate_->builtins();
   Code* illegal = builtins->builtin(Builtins::kIllegalHandler);
-  ForEachBytecode([=](Bytecode bytecode, OperandScale operand_scale) {
-    Code* handler = Bytecodes::BytecodeHasHandler(bytecode, operand_scale)
-                        ? builtins->GetBytecodeHandler(bytecode, operand_scale)
-                        : illegal;
+  int builtin_id = Builtins::kFirstBytecodeHandler;
+  ForEachBytecode([&](Bytecode bytecode, OperandScale operand_scale) {
+    Code* handler = illegal;
+    if (Bytecodes::BytecodeHasHandler(bytecode, operand_scale)) {
+#ifdef DEBUG
+      std::string builtin_name(Builtins::name(builtin_id));
+      std::string expected_name =
+          Bytecodes::ToString(bytecode, operand_scale, "") + "Handler";
+      DCHECK_EQ(expected_name, builtin_name);
+#endif
+      handler = builtins->builtin(builtin_id++);
+    }
     SetBytecodeHandler(bytecode, operand_scale, handler);
   });
+  DCHECK(builtin_id == Builtins::builtin_count);
 #endif  // V8_EMBEDDED_BYTECODE_HANDLERS
 }
 

src/snapshot/startup-deserializer.cc

@@ -64,7 +64,10 @@ void StartupDeserializer::DeserializeInto(Isolate* isolate) {
 
   // Issue code events for newly deserialized code objects.
   LOG_CODE_EVENT(isolate, LogCodeObjects());
+#ifndef V8_EMBEDDED_BYTECODE_HANDLERS
+  // Log bytecode handlers if they haven't already been logged as builtins
   LOG_CODE_EVENT(isolate, LogBytecodeHandlers());
+#endif  // V8_EMBEDDED_BYTECODE_HANDLERS
   LOG_CODE_EVENT(isolate, LogCompiledFunctions());
 
   isolate->builtins()->MarkInitialized();

test/cctest/BUILD.gn

@@ -164,7 +164,6 @@ v8_source_set("cctest_sources") {
     "test-bignum-dtoa.cc",
     "test-bignum.cc",
     "test-bit-vector.cc",
-    "test-builtins.cc",
     "test-circular-queue.cc",
     "test-code-layout.cc",
     "test-code-stub-assembler.cc",

test/cctest/test-builtins.cc

-// Copyright 2016 the V8 project authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "src/builtins/builtins.h"
-#include "src/interpreter/bytecode-operands.h"
-#include "test/cctest/cctest.h"
-
-namespace {
-
-TEST(GetBytecodeHandler) {
-#ifdef V8_EMBEDDED_BYTECODE_HANDLERS
-  using Bytecode = i::interpreter::Bytecode;
-  using OperandScale = i::interpreter::OperandScale;
-  using Builtins = i::Builtins;
-
-  Builtins* builtins = CcTest::i_isolate()->builtins();
-
-  CHECK_EQ(builtins->GetBytecodeHandler(Bytecode::kWide, OperandScale::kSingle),
-           builtins->builtin(Builtins::kWideHandler));
-
-  CHECK_EQ(builtins->GetBytecodeHandler(Bytecode::kLdaImmutableContextSlot,
-                                        OperandScale::kSingle),
-           builtins->builtin(Builtins::kLdaImmutableContextSlotHandler));
-
-  CHECK_EQ(builtins->GetBytecodeHandler(Bytecode::kLdaImmutableContextSlot,
-                                        OperandScale::kDouble),
-           builtins->builtin(Builtins::kLdaImmutableContextSlotWideHandler));
-
-  CHECK_EQ(
-      builtins->GetBytecodeHandler(Bytecode::kLdaImmutableContextSlot,
-                                   OperandScale::kQuadruple),
-      builtins->builtin(Builtins::kLdaImmutableContextSlotExtraWideHandler));
-#endif  // V8_EMBEDDED_BYTECODE_HANDLERS
-}
-
-}  // namespace