Fix counting ignition dispatches

The flag --trace-ignition-dispatches has been broken for a long time,
since it was not designed to work with bytecode handlers that are
generated ahead of time by mksnapshot. This splits the existing
--trace-ignition-dispatches logic into two separate parts:

1. A gn argument which instructs mksnapshot to include dispatch counting
   in the bytecode handlers, and ensures that the Interpreter allocates
   the array of counters, and
2. A runtime flag which enables the ignition-statistics extension which
   implements the JS-accessible function getIgnitionDispatchCounters().

Change-Id: I89323425697f5641451f67b9ddcc0303b8ca209f
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2937564
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Commit-Queue: Seth Brenith <seth.brenith@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#74992}

BUILD.gn

@@ -150,6 +150,15 @@ declare_args() {
   # v8_enable_concurrent_marking_state. See the default setting code below.
   v8_enable_concurrent_marking = true
 
+  # Sets -dV8_IGNITION_DISPATCH_COUNTING.
+  # Enables counting frequencies of bytecode dispatches. After building in this
+  # configuration, subsequent runs of d8 can output frequencies for each pair
+  # of (current, next) bytecode instructions executed if you specify
+  # --trace-ignition-dispatches-output-file, or can generate a JS object with
+  # those frequencies if you run with --expose-ignition-statistics and call the
+  # extension function getIgnitionDispatchCounters().
+  v8_enable_ignition_dispatch_counting = false
+
   # Runs mksnapshot with --turbo-profiling. After building in this
   # configuration, any subsequent run of d8 will output information about usage
   # of basic blocks in builtins.
@@ -825,6 +834,9 @@ config("features") {
   if (v8_enable_atomic_marking_state) {
     defines += [ "V8_ATOMIC_MARKING_STATE" ]
   }
+  if (v8_enable_ignition_dispatch_counting) {
+    defines += [ "V8_IGNITION_DISPATCH_COUNTING" ]
+  }
   if (v8_enable_lazy_source_positions) {
     defines += [ "V8_ENABLE_LAZY_SOURCE_POSITIONS" ]
   }

src/d8/d8.cc

@@ -5175,8 +5175,7 @@ int Shell::Main(int argc, char* argv[]) {
         RunShell(isolate);
       }
 
-      if (i::FLAG_trace_ignition_dispatches &&
-          i::FLAG_trace_ignition_dispatches_output_file != nullptr) {
+      if (i::FLAG_trace_ignition_dispatches_output_file != nullptr) {
         WriteIgnitionDispatchCountersFile(isolate);
       }
 

src/extensions/ignition-statistics-extension.cc

@@ -27,7 +27,6 @@ const char* const IgnitionStatisticsExtension::kSource =
 
 void IgnitionStatisticsExtension::GetIgnitionDispatchCounters(
     const v8::FunctionCallbackInfo<v8::Value>& args) {
-  DCHECK(FLAG_trace_ignition_dispatches);
   args.GetReturnValue().Set(reinterpret_cast<Isolate*>(args.GetIsolate())
                                 ->interpreter()
                                 ->GetDispatchCountersObject());

src/flags/flag-definitions.h

@@ -624,12 +624,10 @@ DEFINE_BOOL(
 #endif
 DEFINE_BOOL(trace_ignition_codegen, false,
             "trace the codegen of ignition interpreter bytecode handlers")
-DEFINE_BOOL(trace_ignition_dispatches, false,
-            "traces the dispatches to bytecode handlers by the ignition "
-            "interpreter")
-DEFINE_STRING(trace_ignition_dispatches_output_file, nullptr,
-              "the file to which the bytecode handler dispatch table is "
-              "written (by default, the table is not written to a file)")
+DEFINE_STRING(
+    trace_ignition_dispatches_output_file, nullptr,
+    "write the bytecode handler dispatch table to the specified file (d8 only) "
+    "(requires building with v8_enable_ignition_dispatch_counting)")
 
 DEFINE_BOOL(trace_track_allocation_sites, false,
             "trace the tracking of allocation sites")
@@ -1403,6 +1401,9 @@ DEFINE_IMPLICATION(expose_gc_as, expose_gc)
 DEFINE_BOOL(expose_externalize_string, false,
             "expose externalize string extension")
 DEFINE_BOOL(expose_trigger_failure, false, "expose trigger-failure extension")
+DEFINE_BOOL(expose_ignition_statistics, false,
+            "expose ignition-statistics extension (requires building with "
+            "v8_enable_ignition_dispatch_counting)")
 DEFINE_INT(stack_trace_limit, 10, "number of stack frames to capture")
 DEFINE_BOOL(builtins_in_stack_traces, false,
             "show built-in functions in stack traces")

src/init/bootstrapper.cc

@@ -5095,7 +5095,7 @@ bool Genesis::InstallExtensions(Isolate* isolate,
           InstallExtension(isolate, "v8/statistics", &extension_states)) &&
          (!FLAG_expose_trigger_failure ||
           InstallExtension(isolate, "v8/trigger-failure", &extension_states)) &&
-         (!FLAG_trace_ignition_dispatches ||
+         (!FLAG_expose_ignition_statistics ||
           InstallExtension(isolate, "v8/ignition-statistics",
                            &extension_states)) &&
          (!isValidCpuTraceMarkFunctionName() ||

src/interpreter/interpreter-assembler.cc

@@ -1208,7 +1208,7 @@ void InterpreterAssembler::DispatchToBytecodeWithOptionalStarLookahead(
 
 void InterpreterAssembler::DispatchToBytecode(
     TNode<WordT> target_bytecode, TNode<IntPtrT> new_bytecode_offset) {
-  if (FLAG_trace_ignition_dispatches) {
+  if (V8_IGNITION_DISPATCH_COUNTING_BOOL) {
     TraceBytecodeDispatch(target_bytecode);
   }
 
@@ -1241,7 +1241,7 @@ void InterpreterAssembler::DispatchWide(OperandScale operand_scale) {
   TNode<IntPtrT> next_bytecode_offset = Advance(1);
   TNode<WordT> next_bytecode = LoadBytecode(next_bytecode_offset);
 
-  if (FLAG_trace_ignition_dispatches) {
+  if (V8_IGNITION_DISPATCH_COUNTING_BOOL) {
     TraceBytecodeDispatch(next_bytecode);
   }
 

src/interpreter/interpreter.cc

@@ -68,15 +68,19 @@ Interpreter::Interpreter(Isolate* isolate)
       interpreter_entry_trampoline_instruction_start_(kNullAddress) {
   memset(dispatch_table_, 0, sizeof(dispatch_table_));
 
-  if (FLAG_trace_ignition_dispatches) {
-    static const int kBytecodeCount = static_cast<int>(Bytecode::kLast) + 1;
-    bytecode_dispatch_counters_table_.reset(
-        new uintptr_t[kBytecodeCount * kBytecodeCount]);
-    memset(bytecode_dispatch_counters_table_.get(), 0,
-           sizeof(uintptr_t) * kBytecodeCount * kBytecodeCount);
+  if (V8_IGNITION_DISPATCH_COUNTING_BOOL) {
+    InitDispatchCounters();
   }
 }
 
+void Interpreter::InitDispatchCounters() {
+  static const int kBytecodeCount = static_cast<int>(Bytecode::kLast) + 1;
+  bytecode_dispatch_counters_table_.reset(
+      new uintptr_t[kBytecodeCount * kBytecodeCount]);
+  memset(bytecode_dispatch_counters_table_.get(), 0,
+         sizeof(uintptr_t) * kBytecodeCount * kBytecodeCount);
+}
+
 namespace {
 
 int BuiltinIndexFromBytecode(Bytecode bytecode, OperandScale operand_scale) {
@@ -375,6 +379,9 @@ const char* Interpreter::LookupNameOfBytecodeHandler(const Code code) {
 uintptr_t Interpreter::GetDispatchCounter(Bytecode from, Bytecode to) const {
   int from_index = Bytecodes::ToByte(from);
   int to_index = Bytecodes::ToByte(to);
+  CHECK_WITH_MSG(bytecode_dispatch_counters_table_ != nullptr,
+                 "Dispatch counters require building with "
+                 "v8_enable_ignition_dispatch_counting");
   return bytecode_dispatch_counters_table_[from_index * kNumberOfBytecodes +
                                            to_index];
 }

src/interpreter/interpreter.h

@@ -22,6 +22,7 @@ class BytecodeArray;
 class Callable;
 class UnoptimizedCompilationJob;
 class FunctionLiteral;
+class IgnitionStatisticsTester;
 class Isolate;
 class LocalIsolate;
 class ParseInfo;
@@ -95,8 +96,11 @@ class Interpreter {
  private:
   friend class SetupInterpreter;
   friend class v8::internal::SetupIsolateDelegate;
+  friend class v8::internal::IgnitionStatisticsTester;
 
-  uintptr_t GetDispatchCounter(Bytecode from, Bytecode to) const;
+  V8_EXPORT_PRIVATE void InitDispatchCounters();
+  V8_EXPORT_PRIVATE uintptr_t GetDispatchCounter(Bytecode from,
+                                                 Bytecode to) const;
 
   // Get dispatch table index of bytecode.
   static size_t GetDispatchTableIndex(Bytecode bytecode,
@@ -112,6 +116,12 @@ class Interpreter {
   Address interpreter_entry_trampoline_instruction_start_;
 };
 
+#ifdef V8_IGNITION_DISPATCH_COUNTING
+#define V8_IGNITION_DISPATCH_COUNTING_BOOL true
+#else
+#define V8_IGNITION_DISPATCH_COUNTING_BOOL false
+#endif
+
 }  // namespace interpreter
 }  // namespace internal
 }  // namespace v8

test/cctest/BUILD.gn

@@ -246,6 +246,7 @@ v8_source_set("cctest_sources") {
     "test-heap-profiler.cc",
     "test-icache.cc",
     "test-identity-map.cc",
+    "test-ignition-statistics-extension.cc",
     "test-inobject-slack-tracking.cc",
     "test-inspector.cc",
     "test-intl.cc",

test/cctest/test-ignition-statistics-extension.cc

@@ -0,0 +1,129 @@
+// Copyright 2021 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/interpreter/bytecodes.h"
+#include "src/interpreter/interpreter.h"
+#include "test/cctest/test-api.h"
+
+namespace v8 {
+namespace internal {
+
+class IgnitionStatisticsTester {
+ public:
+  explicit IgnitionStatisticsTester(Isolate* isolate) : isolate_(isolate) {
+    // In case the build specified v8_enable_ignition_dispatch_counting, the
+    // interpreter already has a dispatch counters table and the bytecode
+    // handlers will update it. To avoid crashes, we keep that array alive here.
+    // This file doesn't test the results in the real array since there is no
+    // automated testing on configurations with
+    // v8_enable_ignition_dispatch_counting.
+    original_bytecode_dispatch_counters_table_ =
+        std::move(isolate->interpreter()->bytecode_dispatch_counters_table_);
+
+    // This sets up the counters array, but does not rewrite the bytecode
+    // handlers to update it.
+    isolate->interpreter()->InitDispatchCounters();
+  }
+
+  void SetDispatchCounter(interpreter::Bytecode from, interpreter::Bytecode to,
+                          uintptr_t value) const {
+    int from_index = interpreter::Bytecodes::ToByte(from);
+    int to_index = interpreter::Bytecodes::ToByte(to);
+    isolate_->interpreter()->bytecode_dispatch_counters_table_
+        [from_index * interpreter::Bytecodes::kBytecodeCount + to_index] =
+        value;
+    CHECK_EQ(isolate_->interpreter()->GetDispatchCounter(from, to), value);
+  }
+
+ private:
+  Isolate* isolate_;
+  std::unique_ptr<uintptr_t[]> original_bytecode_dispatch_counters_table_;
+};
+
+TEST(IgnitionStatisticsExtension) {
+  FLAG_expose_ignition_statistics = true;
+  CcTest::InitializeVM();
+  v8::Isolate* isolate = CcTest::isolate();
+  v8::HandleScope scope(isolate);
+  IgnitionStatisticsTester tester(CcTest::i_isolate());
+
+  Local<Value> typeof_result =
+      CompileRun("typeof getIgnitionDispatchCounters === 'function'");
+  CHECK(typeof_result->BooleanValue(isolate));
+
+  // Get the list of all bytecode names into a JavaScript array.
+#define BYTECODE_NAME_WITH_COMMA(Name, ...) "'" #Name "', "
+  const char* kBytecodeNames =
+      "var bytecodeNames = [" BYTECODE_LIST(BYTECODE_NAME_WITH_COMMA) "];";
+#undef BYTECODE_NAME_WITH_COMMA
+  CompileRun(kBytecodeNames);
+
+  // Check that the dispatch counters object is a non-empty object of objects
+  // where each property name is a bytecode name, in order, and each inner
+  // object is empty.
+  const char* kEmptyTest = R"(
+    var emptyCounters = getIgnitionDispatchCounters();
+    function isEmptyDispatchCounters(counters) {
+      if (typeof counters !== "object") return false;
+      var i = 0;
+      for (var sourceBytecode in counters) {
+        if (sourceBytecode !== bytecodeNames[i]) return false;
+        var countersRow = counters[sourceBytecode];
+        if (typeof countersRow !== "object") return false;
+        for (var counter in countersRow) {
+          return false;
+        }
+        ++i;
+      }
+      return true;
+    }
+    isEmptyDispatchCounters(emptyCounters);)";
+  Local<Value> empty_result = CompileRun(kEmptyTest);
+  CHECK(empty_result->BooleanValue(isolate));
+
+  // Simulate running some code, which would update the counters.
+  tester.SetDispatchCounter(interpreter::Bytecode::kLdar,
+                            interpreter::Bytecode::kStar, 3);
+  tester.SetDispatchCounter(interpreter::Bytecode::kLdar,
+                            interpreter::Bytecode::kLdar, 4);
+  tester.SetDispatchCounter(interpreter::Bytecode::kMov,
+                            interpreter::Bytecode::kLdar, 5);
+
+  // Check that the dispatch counters object is a non-empty object of objects
+  // where each property name is a bytecode name, in order, and the inner
+  // objects reflect the new state.
+  const char* kNonEmptyTest = R"(
+    var nonEmptyCounters = getIgnitionDispatchCounters();
+    function isUpdatedDispatchCounters(counters) {
+      if (typeof counters !== "object") return false;
+      var i = 0;
+      for (var sourceBytecode in counters) {
+        if (sourceBytecode !== bytecodeNames[i]) return false;
+        var countersRow = counters[sourceBytecode];
+        if (typeof countersRow !== "object") return false;
+        switch (sourceBytecode) {
+          case "Ldar":
+            if (JSON.stringify(countersRow) !== '{"Ldar":4,"Star":3}')
+              return false;
+            break;
+          case "Mov":
+            if (JSON.stringify(countersRow) !== '{"Ldar":5}')
+              return false;
+            break;
+          default:
+            for (var counter in countersRow) {
+              return false;
+            }
+        }
+        ++i;
+      }
+      return true;
+    }
+    isUpdatedDispatchCounters(nonEmptyCounters);)";
+  Local<Value> non_empty_result = CompileRun(kNonEmptyTest);
+  CHECK(non_empty_result->BooleanValue(isolate));
+}
+
+}  // namespace internal
+}  // namespace v8

test/mjsunit/ignition/ignition-statistics-extension.js

@@ -1,62 +0,0 @@
-// 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.
-
-// Flags: --trace-ignition-dispatches
-
-assertEquals(typeof getIgnitionDispatchCounters, "function");
-
-var old_dispatch_counters = getIgnitionDispatchCounters();
-
-// Check that old_dispatch_counters is a non-empty object of objects, such that
-// the value of each property in the inner objects is a number.
-
-assertEquals(typeof old_dispatch_counters, "object");
-assertTrue(Object.getOwnPropertyNames(old_dispatch_counters).length > 0);
-for (var source_bytecode in old_dispatch_counters) {
-  var counters_row = old_dispatch_counters[source_bytecode];
-  assertEquals(typeof counters_row, "object");
-  for (var counter in counters_row) {
-    assertEquals(typeof counters_row[counter], "number");
-  }
-}
-
-// Do something
-function f(x) { return x*x; }
-f(42);
-
-var new_dispatch_counters = getIgnitionDispatchCounters();
-
-var old_source_bytecodes = Object.getOwnPropertyNames(old_dispatch_counters);
-var new_source_bytecodes = Object.getOwnPropertyNames(new_dispatch_counters);
-var common_source_bytecodes = new_source_bytecodes.filter(function (name) {
-  return old_source_bytecodes.indexOf(name) > -1;
-});
-
-// Check that the keys on the outer objects are the same
-assertEquals(common_source_bytecodes, old_source_bytecodes);
-assertEquals(common_source_bytecodes, new_source_bytecodes);
-
-common_source_bytecodes.forEach(function (source_bytecode) {
-  var new_counters_row = new_dispatch_counters[source_bytecode];
-  var old_counters_row = old_dispatch_counters[source_bytecode];
-
-  var old_destination_bytecodes = Object.getOwnPropertyNames(old_counters_row);
-  var new_destination_bytecodes = Object.getOwnPropertyNames(new_counters_row);
-
-  // Check that all the keys in old_ are in new_ too
-  old_destination_bytecodes.forEach(function (name) {
-    assertTrue(new_destination_bytecodes.indexOf(name) > -1);
-  });
-
-  // Check that for each source-destination pair, the counter has either
-  // appeared (was undefined before calling f()), is unchanged, or incremented.
-  new_destination_bytecodes.forEach(function (destination_bytecode) {
-    var new_counter = new_counters_row[destination_bytecode];
-    var old_counter = old_counters_row[destination_bytecode];
-    assertTrue(typeof new_counter === "number");
-    if (typeof old_counter === "number") {
-      assertTrue(new_counter >= old_counter);
-    }
-  });
-});