Create a thread safe version of StatsCounters and use.

src/wasm/wasm-module.cc

 // Copyright 2015 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 <functional>
 #include <memory>
 
 #include "src/asmjs/asm-js.h"
 #include "src/assembler-inl.h"
 #include "src/base/atomic-utils.h"
@@ skipping 38 matching lines @@
   } while (false)
 
 namespace {
 
 static const int kInvalidSigIndex = -1;
 
 byte* raw_buffer_ptr(MaybeHandle<JSArrayBuffer> buffer, int offset) {
   return static_cast<byte*>(buffer.ToHandleChecked()->backing_store()) + offset;
 }
 
-static void RecordStats(Isolate* isolate, Code* code, bool is_sync) {
-  if (is_sync) {
-    // TODO(karlschimpf): Make this work when asynchronous.
-    // https://bugs.chromium.org/p/v8/issues/detail?id=6361
-    isolate->counters()->wasm_generated_code_size()->Increment(
-        code->body_size());
-    isolate->counters()->wasm_reloc_size()->Increment(
-        code->relocation_info()->length());
+static void RecordStats(Code* code, Counters* counters) {
+  counters->wasm_generated_code_size()->Increment(code->body_size());
+  counters->wasm_reloc_size()->Increment(code->relocation_info()->length());
+}
+
+static void RecordStats(Handle<FixedArray> functions, Counters* counters) {
+  DisallowHeapAllocation no_gc;
+  for (int i = 0; i < functions->length(); ++i) {
+    RecordStats(Code::cast(functions->get(i)), counters);
   }
 }
 
-static void RecordStats(Isolate* isolate, Handle<FixedArray> functions,
-                        bool is_sync) {
-  DisallowHeapAllocation no_gc;
-  for (int i = 0; i < functions->length(); ++i) {
-    RecordStats(isolate, Code::cast(functions->get(i)), is_sync);
-  }
-}
-
 void* TryAllocateBackingStore(Isolate* isolate, size_t size,
                               bool enable_guard_regions, void*& allocation_base,
                               size_t& allocation_length) {
   // TODO(eholk): Right now enable_guard_regions has no effect on 32-bit
   // systems. It may be safer to fail instead, given that other code might do
   // things that would be unsafe if they expected guard pages where there
   // weren't any.
   if (enable_guard_regions && kGuardRegionsSupported) {
     // TODO(eholk): On Windows we want to make sure we don't commit the guard
     // pages yet.
@@ skipping 243 matching lines @@
 class CompilationHelper {
  public:
   // The compilation helper takes ownership of the {WasmModule}.
   // In {CompileToModuleObject}, it will transfer ownership to the generated
   // {WasmModuleWrapper}. If this method is not called, ownership may be
   // reclaimed by explicitely releasing the {module_} field.
   CompilationHelper(Isolate* isolate, std::unique_ptr<WasmModule> module,
                     bool is_sync)
       : isolate_(isolate),
         module_(std::move(module)),
+        counters_shared_(isolate->counters_shared()),
         is_sync_(is_sync),
-        executed_units_(isolate->random_number_generator()) {}
+        executed_units_(isolate->random_number_generator()) {
+    counters_ = counters_shared_.get();
+  }
 
   // The actual runnable task that performs compilations in the background.
   class CompilationTask : public CancelableTask {
    public:
     CompilationHelper* helper_;
     explicit CompilationTask(CompilationHelper* helper)
         : CancelableTask(helper->isolate_), helper_(helper) {}
 
     void RunInternal() override {
       while (helper_->FetchAndExecuteCompilationUnit()) {
       }
       helper_->module_->pending_tasks.get()->Signal();
     }
   };
 
   Isolate* isolate_;
   std::unique_ptr<WasmModule> module_;
+  std::shared_ptr<Counters> counters_shared_;
+  Counters* counters_;

[Clemens Hammacher, 2017/06/02 13:08:50]

Why is this field needed? {counters_shared_.get()} should be equally good. It's
just reading a field from our own shared_ptr instance, so no potential race
conditions here.

[kschimpf, 2017/06/02 15:01:02]

I was confusing the assignment operator with the get() method, where there is a
LOT of inline code. You appear to be correct that I should have used the get()
method. Fixing in separate CL.

[kschimpf, 2017/06/02 17:06:27]

Fixed in CL https://codereview.chromium.org/2919953003

   bool is_sync_;
   std::vector<std::unique_ptr<compiler::WasmCompilationUnit>>
       compilation_units_;
   CodeGenerationSchedule executed_units_;
   base::Mutex result_mutex_;
   base::AtomicNumber<size_t> next_unit_;
   size_t num_background_tasks_ = 0;
   // This flag should only be set while holding result_mutex_.
   bool finisher_is_running_ = false;
 
@@ skipping 313 matching lines @@
       // (lazy) compilation time.
       ValidateSequentially(&module_env, thrower);
     }
     if (thrower->error()) return {};
 
     // At this point, compilation has completed. Update the code table.
     for (size_t i = FLAG_skip_compiling_wasm_funcs;
          i < temp_instance->function_code.size(); ++i) {
       Code* code = *temp_instance->function_code[i];
       code_table->set(static_cast<int>(i), code);
-      RecordStats(isolate_, code, is_sync_);
+      RecordStats(code, counters_);
     }
 
     // Create heap objects for script, module bytes and asm.js offset table to
     // be stored in the shared module data.
     Handle<Script> script;
     Handle<ByteArray> asm_js_offset_table;
     if (asm_js_script.is_null()) {
       script = CreateWasmScript(isolate_, wire_bytes);
     } else {
       script = asm_js_script;
@@ skipping 47 matching lines @@
     for (auto exp : module->export_table) {
       if (exp.kind != kExternalFunction) continue;
       Handle<Code> wasm_code = EnsureExportedLazyDeoptData(
           isolate_, Handle<WasmInstanceObject>::null(), code_table, exp.index);
       Handle<Code> wrapper_code =
           js_to_wasm_cache.CloneOrCompileJSToWasmWrapper(isolate_, module,
                                                          wasm_code, exp.index);
       int export_index =
           static_cast<int>(module->functions.size() + func_index);
       code_table->set(export_index, *wrapper_code);
-      RecordStats(isolate_, *wrapper_code, is_sync_);
+      RecordStats(*wrapper_code, counters_);
       func_index++;
     }
 
     return WasmModuleObject::New(isolate_, compiled_module);
   }
 };
 
 static void MemoryInstanceFinalizer(Isolate* isolate,
                                     WasmInstanceObject* instance) {
   DisallowHeapAllocation no_gc;
@@ skipping 133 matching lines @@
 int ExtractDirectCallIndex(wasm::Decoder& decoder, const byte* pc) {
   DCHECK_EQ(static_cast<int>(kExprCallFunction), static_cast<int>(*pc));
   // Read the leb128 encoded u32 value (up to 5 bytes starting at pc + 1).
   decoder.Reset(pc + 1, pc + 6);
   uint32_t call_idx = decoder.consume_u32v("call index");
   DCHECK(decoder.ok());
   DCHECK_GE(kMaxInt, call_idx);
   return static_cast<int>(call_idx);
 }
 
-void RecordLazyCodeStats(Isolate* isolate, Code* code) {
-  isolate->counters()->wasm_lazily_compiled_functions()->Increment();
-  isolate->counters()->wasm_generated_code_size()->Increment(code->body_size());
-  isolate->counters()->wasm_reloc_size()->Increment(
-      code->relocation_info()->length());
+void RecordLazyCodeStats(Code* code, Counters* counters) {
+  counters->wasm_lazily_compiled_functions()->Increment();
+  counters->wasm_generated_code_size()->Increment(code->body_size());
+  counters->wasm_reloc_size()->Increment(code->relocation_info()->length());
 }
 
 }  // namespace
 
 Handle<JSArrayBuffer> wasm::SetupArrayBuffer(Isolate* isolate,
                                              void* allocation_base,
                                              size_t allocation_length,
                                              void* backing_store, size_t size,
                                              bool is_external,
                                              bool enable_guard_regions) {
@@ skipping 214 matching lines @@
 // It closes over the {Isolate}, the {ErrorThrower}, the {WasmCompiledModule},
 // etc.
 class InstantiationHelper {
  public:
   InstantiationHelper(Isolate* isolate, ErrorThrower* thrower,
                       Handle<WasmModuleObject> module_object,
                       MaybeHandle<JSReceiver> ffi,
                       MaybeHandle<JSArrayBuffer> memory)
       : isolate_(isolate),
         module_(module_object->compiled_module()->module()),
+        counters_shared_(isolate->counters_shared()),
         thrower_(thrower),
         module_object_(module_object),
         ffi_(ffi.is_null() ? Handle<JSReceiver>::null()
                            : ffi.ToHandleChecked()),
         memory_(memory.is_null() ? Handle<JSArrayBuffer>::null()
-                                 : memory.ToHandleChecked()) {}
+                                 : memory.ToHandleChecked()) {
+    counters_ = counters_shared_.get();
+  }
 
   // Build an instance, in all of its glory.
   MaybeHandle<WasmInstanceObject> Build() {
     // Check that an imports argument was provided, if the module requires it.
     // No point in continuing otherwise.
     if (!module_->import_table.empty() && ffi_.is_null()) {
       thrower_->TypeError(
           "Imports argument must be present and must be an object");
       return {};
     }
@@ skipping 69 matching lines @@
             case Code::JS_TO_WASM_FUNCTION:
             case Code::WASM_FUNCTION: {
               Handle<Code> code = factory->CopyCode(orig_code);
               code_table->set(i, *code);
               break;
             }
             default:
               UNREACHABLE();
           }
         }
-        RecordStats(isolate_, code_table, is_sync_);
+        RecordStats(code_table, counters_);
       } else {
         // There was no owner, so we can reuse the original.
         compiled_module_ = original;
         old_code_table =
             factory->CopyFixedArray(compiled_module_->code_table());
         code_table = compiled_module_->code_table();
         TRACE("Reusing existing instance %d\n",
               compiled_module_->instance_id());
       }
       compiled_module_->set_native_context(isolate_->native_context());
@@ skipping 256 matching lines @@
       int start_index = module_->start_function_index;
       Handle<Code> startup_code = EnsureExportedLazyDeoptData(
           isolate_, instance, code_table, start_index);
       FunctionSig* sig = module_->functions[start_index].sig;
       Handle<Code> wrapper_code =
           js_to_wasm_cache_.CloneOrCompileJSToWasmWrapper(
               isolate_, module_, startup_code, start_index);
       Handle<WasmExportedFunction> startup_fct = WasmExportedFunction::New(
           isolate_, instance, MaybeHandle<String>(), start_index,
           static_cast<int>(sig->parameter_count()), wrapper_code);
-      RecordStats(isolate_, *startup_code, is_sync_);
+      RecordStats(*startup_code, counters_);
       // Call the JS function.
       Handle<Object> undefined = factory->undefined_value();
       MaybeHandle<Object> retval =
           Execution::Call(isolate_, startup_fct, undefined, 0, nullptr);
 
       if (retval.is_null()) {
         DCHECK(isolate_->has_pending_exception());
         isolate_->OptionalRescheduleException(false);
         // It's unfortunate that the new instance is already linked in the
         // chain. However, we need to set up everything before executing the
@@ skipping 13 matching lines @@
   // Represents the initialized state of a table.
   struct TableInstance {
     Handle<WasmTableObject> table_object;    // WebAssembly.Table instance
     Handle<FixedArray> js_wrappers;          // JSFunctions exported
     Handle<FixedArray> function_table;       // internal code array
     Handle<FixedArray> signature_table;      // internal sig array
   };
 
   Isolate* isolate_;
   WasmModule* const module_;
-  constexpr static bool is_sync_ = true;
+  std::shared_ptr<Counters> counters_shared_;
+  Counters* counters_;

[Clemens Hammacher, 2017/06/02 13:08:50]

Same here.

[kschimpf, 2017/06/02 17:06:28]

Fixed in CL https://codereview.chromium.org/2919953003

   ErrorThrower* thrower_;
   Handle<WasmModuleObject> module_object_;
   Handle<JSReceiver> ffi_;        // TODO(titzer): Use MaybeHandle
   Handle<JSArrayBuffer> memory_;  // TODO(titzer): Use MaybeHandle
   Handle<JSArrayBuffer> globals_;
   Handle<WasmCompiledModule> compiled_module_;
   std::vector<TableInstance> table_instances_;
   std::vector<Handle<JSFunction>> js_wrappers_;
   JSToWasmWrapperCache js_to_wasm_cache_;
 
@@ skipping 189 matching lines @@
               isolate_, index, module_->functions[import.index].sig,
               Handle<JSReceiver>::cast(value), module_name, import_name,
               module_->get_origin());
           if (import_wrapper.is_null()) {
             ReportLinkError(
                 "imported function does not match the expected type", index,
                 module_name, import_name);
             return -1;
           }
           code_table->set(num_imported_functions, *import_wrapper);
-          RecordStats(isolate_, *import_wrapper, is_sync_);
+          RecordStats(*import_wrapper, counters_);
           num_imported_functions++;
           break;
         }
         case kExternalTable: {
           if (!WasmJs::IsWasmTableObject(isolate_, value)) {
             ReportLinkError("table import requires a WebAssembly.Table", index,
                             module_name, import_name);
             return -1;
           }
           WasmIndirectFunctionTable& table =
@@ skipping 952 matching lines @@
 // e.g. when we synchronizing tasks or when we delete the AyncCompileJob.
 class AsyncCompileJob {
   // TODO(ahaas): Fix https://bugs.chromium.org/p/v8/issues/detail?id=6263 to
   // make sure that d8 does not shut down before the AsyncCompileJob is
   // finished.
  public:
   explicit AsyncCompileJob(Isolate* isolate, std::unique_ptr<byte[]> bytes_copy,
                            size_t length, Handle<Context> context,
                            Handle<JSPromise> promise)
       : isolate_(isolate),
+        counters_shared_(isolate->counters_shared()),
         bytes_copy_(std::move(bytes_copy)),
         wire_bytes_(bytes_copy_.get(), bytes_copy_.get() + length) {
     // The handles for the context and promise must be deferred.
     DeferredHandleScope deferred(isolate);
     context_ = Handle<Context>(*context);
     module_promise_ = Handle<JSPromise>(*promise);
     deferred_handles_.push_back(deferred.Detach());
+    counters_ = counters_shared_.get();
   }
 
   void Start() {
     DoAsync<DecodeModule>();  // --
   }
 
   ~AsyncCompileJob() {
     for (auto d : deferred_handles_) delete d;
   }
 
  private:
   Isolate* isolate_;
+  std::shared_ptr<Counters> counters_shared_;
+  Counters* counters_;
   std::unique_ptr<byte[]> bytes_copy_;
   ModuleWireBytes wire_bytes_;
   Handle<Context> context_;
   Handle<JSPromise> module_promise_;
   std::unique_ptr<CompilationHelper> helper_;
   std::unique_ptr<ModuleBytesEnv> module_bytes_env_;
 
   bool failed_ = false;
   std::vector<DeferredHandles*> deferred_handles_;
   Handle<WasmModuleObject> module_object_;
@@ skipping 346 matching lines @@
   };
 
   //==========================================================================
   // Step 5b (sync): Finish heap-allocated data structures.
   //==========================================================================
   class FinishCompile : public SyncCompileTask {
     void RunImpl() override {
       TRACE_COMPILE("(5b) Finish compile...\n");
       HandleScope scope(job_->isolate_);
       // At this point, compilation has completed. Update the code table.
-      constexpr bool is_sync = true;
       for (size_t i = FLAG_skip_compiling_wasm_funcs;
            i < job_->temp_instance_->function_code.size(); ++i) {
         Code* code = Code::cast(job_->code_table_->get(static_cast<int>(i)));
-        RecordStats(job_->isolate_, code, !is_sync);
+        RecordStats(code, job_->counters_);
       }
 
       // Create heap objects for script and module bytes to be stored in the
       // shared module data. Asm.js is not compiled asynchronously.
       Handle<Script> script =
           CreateWasmScript(job_->isolate_, job_->wire_bytes_);
       Handle<ByteArray> asm_js_offset_table;
       // TODO(wasm): Improve efficiency of storing module wire bytes.
       //   1. Only store relevant sections, not function bodies
       //   2. Don't make a second copy of the bytes here; reuse the copy made
@@ skipping 45 matching lines @@
   //==========================================================================
   // Step 6 (sync): Compile JS->WASM wrappers.
   //==========================================================================
   class CompileWrappers : public SyncCompileTask {
     void RunImpl() override {
       TRACE_COMPILE("(6) Compile wrappers...\n");
       // Compile JS->WASM wrappers for exported functions.
       HandleScope scope(job_->isolate_);
       JSToWasmWrapperCache js_to_wasm_cache;
       int func_index = 0;
-      constexpr bool is_sync = true;
       WasmModule* module = job_->compiled_module_->module();
       for (auto exp : module->export_table) {
         if (exp.kind != kExternalFunction) continue;
         Handle<Code> wasm_code(Code::cast(job_->code_table_->get(exp.index)),
                                job_->isolate_);
         Handle<Code> wrapper_code =
             js_to_wasm_cache.CloneOrCompileJSToWasmWrapper(
                 job_->isolate_, module, wasm_code, exp.index);
         int export_index =
             static_cast<int>(module->functions.size() + func_index);
         job_->code_table_->set(export_index, *wrapper_code);
-        RecordStats(job_->isolate_, *wrapper_code, !is_sync);
+        RecordStats(*wrapper_code, job_->counters_);
         func_index++;
       }
 
       job_->DoSync<FinishModule>();
     }
   };
 
   //==========================================================================
   // Step 7 (sync): Finish the module and resolve the promise.
   //==========================================================================
@@ skipping 103 matching lines @@
     // do the patching redundantly.
     Handle<FixedArray> new_deopt_data =
         isolate->factory()->CopyFixedArrayUpTo(exp_deopt_data, 2, TENURED);
     lazy_compile_code->set_deoptimization_data(*new_deopt_data);
   }
 
   return compiled_code;
 }
 
 void LazyCompilationOrchestrator::CompileFunction(
-    Isolate* isolate, Handle<WasmInstanceObject> instance, int func_index) {
+    Isolate* isolate, Handle<WasmInstanceObject> instance, int func_index,
+    Counters* counters) {
   Handle<WasmCompiledModule> compiled_module(instance->compiled_module(),
                                              isolate);
   if (Code::cast(compiled_module->code_table()->get(func_index))->kind() ==
       Code::WASM_FUNCTION) {
     return;
   }
 
   size_t num_function_tables =
       compiled_module->module()->function_tables.size();
   // Store a vector of handles to be embedded in the generated code.
@@ skipping 67 matching lines @@
     DCHECK_IMPLIES(mem_size == 0, mem_start == nullptr);
     if (mem_size > 0) {
       code_specialization.RelocateMemoryReferences(nullptr, 0, mem_start,
                                                    mem_size);
     }
   }
   code_specialization.RelocateDirectCalls(instance);
   code_specialization.ApplyToWasmCode(*code, SKIP_ICACHE_FLUSH);
   Assembler::FlushICache(isolate, code->instruction_start(),
                          code->instruction_size());
-  RecordLazyCodeStats(isolate, *code);
+  RecordLazyCodeStats(*code, counters);
 }
 
 Handle<Code> LazyCompilationOrchestrator::CompileLazy(
     Isolate* isolate, Handle<WasmInstanceObject> instance, Handle<Code> caller,
     int call_offset, int exported_func_index, bool patch_caller) {
   struct NonCompiledFunction {
     int offset;
     int func_index;
   };
+  std::shared_ptr<Counters> counters_shared = isolate->counters_shared();
+  Counters* counters = counters_shared.get();
   std::vector<NonCompiledFunction> non_compiled_functions;
   int func_to_return_idx = exported_func_index;
   wasm::Decoder decoder(nullptr, nullptr);
   bool is_js_to_wasm = caller->kind() == Code::JS_TO_WASM_FUNCTION;
   Handle<WasmCompiledModule> compiled_module(instance->compiled_module(),
                                              isolate);
 
   if (is_js_to_wasm) {
     non_compiled_functions.push_back({0, exported_func_index});
   } else if (patch_caller) {
@@ skipping 24 matching lines @@
           ExtractDirectCallIndex(decoder, func_bytes + byte_pos);
       non_compiled_functions.push_back({offset, called_func_index});
       // Call offset one instruction after the call. Remember the last called
       // function before that offset.
       if (offset < call_offset) func_to_return_idx = called_func_index;
     }
   }
 
   // TODO(clemensh): compile all functions in non_compiled_functions in
   // background, wait for func_to_return_idx.
-  CompileFunction(isolate, instance, func_to_return_idx);
+  CompileFunction(isolate, instance, func_to_return_idx, counters);
 
   if (is_js_to_wasm || patch_caller) {
     DisallowHeapAllocation no_gc;
     // Now patch the code object with all functions which are now compiled.
     int idx = 0;
     for (RelocIterator it(*caller, RelocInfo::kCodeTargetMask); !it.done();
          it.next()) {
       Code* callee =
           Code::GetCodeFromTargetAddress(it.rinfo()->target_address());
       if (callee->builtin_index() != Builtins::kWasmCompileLazy) continue;
@@ skipping 22 matching lines @@
                                      callee_compiled->instruction_start());
     }
     DCHECK_EQ(non_compiled_functions.size(), idx);
   }
 
   Code* ret =
       Code::cast(compiled_module->code_table()->get(func_to_return_idx));
   DCHECK_EQ(Code::WASM_FUNCTION, ret->kind());
   return handle(ret, isolate);
 }