[Interpreter] Add Ignition statistics JavaScript extension.

src/extensions/ignition-statistics-extension.cc

Index: src/extensions/ignition-statistics-extension.cc
diff --git a/src/extensions/ignition-statistics-extension.cc b/src/extensions/ignition-statistics-extension.cc
new file mode 100644
index 0000000000000000000000000000000000000000..9e9e99e26dc65569a717d928c4b48ade9499fc9f
--- /dev/null
+++ b/src/extensions/ignition-statistics-extension.cc
@@ -0,0 +1,78 @@
+// 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/extensions/ignition-statistics-extension.h"
+
+#include "src/interpreter/bytecodes.h"
+#include "src/interpreter/interpreter.h"
+#include "src/isolate.h"
+
+namespace v8 {
+namespace internal {
+
+v8::Local<v8::FunctionTemplate>
+IgnitionStatisticsExtension::GetNativeFunctionTemplate(
+    v8::Isolate* isolate, v8::Local<v8::String> name) {
+  DCHECK_EQ(strcmp(*v8::String::Utf8Value(name), "getIgnitionDispatchCounters"),
+            0);
+  return v8::FunctionTemplate::New(
+      isolate, IgnitionStatisticsExtension::DumpIgnitionDispatchCounters);
+}
+
+const char* const IgnitionStatisticsExtension::kSource =
+    "native function getIgnitionDispatchCounters();";
+
+void IgnitionStatisticsExtension::DumpIgnitionDispatchCounters(

[rmcilroy, 2016/04/21 14:17:29]

Call this the same as the javascript function (or vice versa).

[Stefano Sanfilippo, 2016/04/21 14:22:42]

Oops, forgot to rename the method as well. Done.

+    const v8::FunctionCallbackInfo<v8::Value>& args) {
+  DCHECK_EQ(args.Length(), 0);
+  DCHECK(FLAG_trace_ignition_dispatches);
+
+  using interpreter::Bytecode;

[oth, 2016/04/21 14:03:02]

Is this needed?

[Stefano Sanfilippo, 2016/04/21 14:22:42]

No, this is not needed, but using interpreter::Bytecode causes a few statements
to be laid out on two lines, which makes the code definitely less readable. 

[rmcilroy, 2016/04/21 14:33:30]

+1 to removing this.

[Stefano Sanfilippo, 2016/04/22 13:00:49]

Done contextually with extracting this piece of code to an Interpreter method.

+
+  static const int kBytecodeCount = static_cast<int>(Bytecode::kLast) + 1;

[oth, 2016/04/21 14:03:02]

This could be brittle as the code is reading directly from the dispatch table
array so there is no bounds checking.

If the code could decouple the dispatch counter table layout that would be
better. For instance, the interpreter could expose a get_dispatch_count(from,
to) method.

[rmcilroy, 2016/04/21 14:17:29]

+1

[Stefano Sanfilippo, 2016/04/21 14:22:42]

This is a very good point we might want to discuss further. The same pattern is
used elsewhere in the codebase, so I might save it for a future CL. WDYT?

[rmcilroy, 2016/04/21 14:33:30]

No I think we should do it here. The only other place which uses
bytecode_dispatch_count_table() is in interpreter.cc so we can refactor that
usage and remove bytecode_dispatch_count_table() form the interpreter API
entirely.

Actually, come to think of it, could you move this code into interpreter.cc and
then have the d8 version call it to get a JavaScript object then call a function
to output it as JSON, instead of manually building the JSON itself?

[Stefano Sanfilippo, 2016/04/22 13:00:49]

As discussed, we still need a getter for the raw pointer because of the
ExternalReference. 
This is a brilliant suggestion, done.

+
+  v8::Isolate* isolate = args.GetIsolate();
+  v8::Local<v8::Context> context = isolate->GetCurrentContext();
+  v8::Local<v8::Object> counters_map = v8::Object::New(isolate);
+
+  uintptr_t* bytecode_dispatch_count_table =
+      reinterpret_cast<Isolate*>(isolate)
+          ->interpreter()
+          ->bytecode_dispatch_count_table();
+
+  for (int from_index = 0; from_index < kBytecodeCount; ++from_index) {
+    v8::Local<v8::Object> counters_row = v8::Object::New(args.GetIsolate());
+
+    for (int to_index = 0; to_index < kBytecodeCount; ++to_index) {
+      uintptr_t counter =
+          bytecode_dispatch_count_table[from_index * kBytecodeCount + to_index];
+      if (counter > 0) {
+        Bytecode to_bytecode = interpreter::Bytecodes::FromByte(to_index);
+        std::string to_name = interpreter::Bytecodes::ToString(to_bytecode);
+        Local<v8::String> to_name_object =
+            v8::String::NewFromUtf8(isolate, to_name.c_str(),
+                                    NewStringType::kNormal)
+                .ToLocalChecked();
+        Local<v8::Number> counter_object =
+            v8::Number::New(isolate, static_cast<uint32_t>(counter));

[rmcilroy, 2016/04/21 14:17:29]

You are casting to 32bits here - did casting to doubles to preserve 64bit values
not work?

[Stefano Sanfilippo, 2016/04/21 14:22:42]

Fixed.

+        CHECK(counters_row->Set(context, to_name_object, counter_object)

[rmcilroy, 2016/04/21 14:17:29]

No need for the CHECK here I don't think. IsJust will crash itself if the return
is Nothing.

[Stefano Sanfilippo, 2016/04/21 14:22:42]

Done.

+                  .IsJust());
+      }
+    }
+
+    Bytecode from_bytecode = interpreter::Bytecodes::FromByte(from_index);
+    std::string from_name = interpreter::Bytecodes::ToString(from_bytecode);
+    Local<v8::String> from_name_object =
+        v8::String::NewFromUtf8(isolate, from_name.c_str(),
+                                NewStringType::kNormal)
+            .ToLocalChecked();
+
+    CHECK(counters_map->Set(context, from_name_object, counters_row).IsJust());

[rmcilroy, 2016/04/21 14:17:29]

ditto

[Stefano Sanfilippo, 2016/04/21 14:22:42]

Done.

+  }
+
+  args.GetReturnValue().Set(counters_map);
+}
+
+}  // namespace internal
+}  // namespace v8