Fix use of history timers in background threads.

src/wasm/module-compiler.cc

 // Copyright 2017 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/wasm/module-compiler.h>
 
 #include <atomic>
 
 #include "src/asmjs/asm-js.h"
 #include "src/assembler-inl.h"
@@ skipping 79 matching lines @@
       executed_units_(
           isolate->random_number_generator(),
           (isolate->heap()->memory_allocator()->code_range()->valid()
                ? isolate->heap()->memory_allocator()->code_range()->size()
                : isolate->heap()->code_space()->Capacity()) /
               2),
       num_background_tasks_(
           Min(static_cast<size_t>(FLAG_wasm_num_compilation_tasks),
               V8::GetCurrentPlatform()->NumberOfAvailableBackgroundThreads())),
       stopped_compilation_tasks_(num_background_tasks_) {
-  counters_ = counters_shared_.get();
 }
 
 bool ModuleCompiler::GetNextUncompiledFunctionId(size_t* index) {
   DCHECK_NOT_NULL(index);
   // - 1 because AtomicIncrement returns the value after the atomic increment.
   *index = next_unit_.Increment(1) - 1;
   return *index < compilation_units_.size();
 }
 
 // The actual runnable task that performs compilations in the background.
@@ skipping 66 matching lines @@
 
 size_t ModuleCompiler::InitializeParallelCompilation(
     const std::vector<WasmFunction>& functions, ModuleBytesEnv& module_env) {
   uint32_t start = module_env.module_env.module->num_imported_functions +
                    FLAG_skip_compiling_wasm_funcs;
   uint32_t num_funcs = static_cast<uint32_t>(functions.size());
   uint32_t funcs_to_compile = start > num_funcs ? 0 : num_funcs - start;
   compilation_units_.reserve(funcs_to_compile);
   for (uint32_t i = start; i < num_funcs; ++i) {
     const WasmFunction* func = &functions[i];
-    constexpr bool is_sync = true;
     compilation_units_.push_back(std::unique_ptr<compiler::WasmCompilationUnit>(
-        new compiler::WasmCompilationUnit(isolate_, &module_env, func,
-                                          !is_sync)));
+        new compiler::WasmCompilationUnit(isolate_, &module_env, func)));
   }
   return funcs_to_compile;
 }
 
 void ModuleCompiler::RestartCompilationTasks() {
   base::LockGuard<base::Mutex> guard(&tasks_mutex_);
   for (; stopped_compilation_tasks_ > 0; --stopped_compilation_tasks_) {
     V8::GetCurrentPlatform()->CallOnBackgroundThread(
         new CompilationTask(this),
         v8::Platform::ExpectedRuntime::kShortRunningTask);
@@ skipping 157 matching lines @@
   Handle<FixedArray> function_tables =
       factory->NewFixedArray(function_table_count, TENURED);
   Handle<FixedArray> signature_tables =
       factory->NewFixedArray(function_table_count, TENURED);
   for (int i = 0; i < function_table_count; ++i) {
     temp_instance.function_tables[i] = factory->NewFixedArray(1, TENURED);
     temp_instance.signature_tables[i] = factory->NewFixedArray(1, TENURED);
     function_tables->set(i, *temp_instance.function_tables[i]);
     signature_tables->set(i, *temp_instance.signature_tables[i]);
   }
-
-  if (is_sync_) {
-    // TODO(karlschimpf): Make this work when asynchronous.
-    // https://bugs.chromium.org/p/v8/issues/detail?id=6361
-    HistogramTimerScope wasm_compile_module_time_scope(
-        module_->is_wasm()
-            ? isolate_->counters()->wasm_compile_wasm_module_time()
-            : isolate_->counters()->wasm_compile_asm_module_time());
-    return CompileToModuleObjectInternal(
-        thrower, wire_bytes, asm_js_script, asm_js_offset_table_bytes, factory,
-        &temp_instance, &function_tables, &signature_tables);
-  }
+  TimedHistogramScope wasm_compile_module_time_scope(
+      module_->is_wasm() ? counters()->wasm_compile_wasm_module_time()

[Mircea Trofin, 2017/06/22 18:42:24]

can this test be encapsulated in a method, it appears in a few places.

[kschimpf, 2017/06/22 20:38:31]

When I tried that in an earlier review, there was quite a bit of push back from
the reviewers. So I left it in its current form.

+                         : counters()->wasm_compile_asm_module_time());
   return CompileToModuleObjectInternal(
       thrower, wire_bytes, asm_js_script, asm_js_offset_table_bytes, factory,
       &temp_instance, &function_tables, &signature_tables);
 }
 
 namespace {
 bool compile_lazy(const WasmModule* module) {
   return FLAG_wasm_lazy_compilation ||
          (FLAG_asm_wasm_lazy_compilation && module->is_asm_js());
 }
@@ skipping 218 matching lines @@
                                   ? isolate_->builtins()->WasmCompileLazy()
                                   : isolate_->builtins()->Illegal();
   for (int i = 0, e = static_cast<int>(module_->functions.size()); i < e; ++i) {
     code_table->set(i, *init_builtin);
     temp_instance->function_code[i] = init_builtin;
   }
 
   if (is_sync_)
     // TODO(karlschimpf): Make this work when asynchronous.
     // https://bugs.chromium.org/p/v8/issues/detail?id=6361
-    (module_->is_wasm() ? isolate_->counters()->wasm_functions_per_wasm_module()
-                        : isolate_->counters()->wasm_functions_per_asm_module())
+    (module_->is_wasm() ? counters()->wasm_functions_per_wasm_module()
+                        : counters()->wasm_functions_per_asm_module())
         ->AddSample(static_cast<int>(module_->functions.size()));
 
   if (!lazy_compile) {
     size_t funcs_to_compile =
         module_->functions.size() - module_->num_imported_functions;
     bool compile_parallel =
         !FLAG_trace_wasm_decoder && FLAG_wasm_num_compilation_tasks > 0 &&
         funcs_to_compile > 1 &&
         V8::GetCurrentPlatform()->NumberOfAvailableBackgroundThreads() > 0;
     if (compile_parallel) {
@@ skipping 13 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(code, counters_);
+    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 45 matching lines @@
   JSToWasmWrapperCache js_to_wasm_cache;
   int func_index = 0;
   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(*wrapper_code, counters_);
+    RecordStats(*wrapper_code, counters());
     func_index++;
   }
 
   return WasmModuleObject::New(isolate_, compiled_module);
 }
 
 Handle<Code> JSToWasmWrapperCache::CloneOrCompileJSToWasmWrapper(
     Isolate* isolate, const wasm::WasmModule* module, Handle<Code> wasm_code,
     uint32_t index) {
   const wasm::WasmFunction* func = &module->functions[index];
@@ skipping 32 matching lines @@
     WeakCallbackInfo<void>::Callback instance_finalizer_callback)
     : 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()),
       instance_finalizer_callback_(instance_finalizer_callback) {
-  counters_ = counters_shared_.get();
 }
 
 // Build an instance, in all of its glory.
 MaybeHandle<WasmInstanceObject> InstanceBuilder::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 {};
   }
 
   // Record build time into correct bucket, then build instance.
   HistogramTimerScope wasm_instantiate_module_time_scope(
-      module_->is_wasm()
-          ? isolate_->counters()->wasm_instantiate_wasm_module_time()
-          : isolate_->counters()->wasm_instantiate_asm_module_time());
+      module_->is_wasm() ? counters()->wasm_instantiate_wasm_module_time()
+                         : counters()->wasm_instantiate_asm_module_time());
   Factory* factory = isolate_->factory();
 
   //--------------------------------------------------------------------------
   // Reuse the compiled module (if no owner), otherwise clone.
   //--------------------------------------------------------------------------
   Handle<FixedArray> code_table;
   // We keep around a copy of the old code table, because we'll be replacing
   // imports for the new instance, and then we need the old imports to be
   // able to relocate.
   Handle<FixedArray> old_code_table;
@@ skipping 53 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(code_table, counters_);
+      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 58 matching lines @@
   //--------------------------------------------------------------------------
   // Set up the indirect function tables for the new instance.
   //--------------------------------------------------------------------------
   if (function_table_count > 0)
     InitializeTables(instance, &code_specialization);
 
   //--------------------------------------------------------------------------
   // Set up the memory for the new instance.
   //--------------------------------------------------------------------------
   uint32_t min_mem_pages = module_->min_mem_pages;
-  (module_->is_wasm() ? isolate_->counters()->wasm_wasm_min_mem_pages_count()
-                      : isolate_->counters()->wasm_asm_min_mem_pages_count())
+  (module_->is_wasm() ? counters()->wasm_wasm_min_mem_pages_count()
+                      : counters()->wasm_asm_min_mem_pages_count())
       ->AddSample(min_mem_pages);
 
   if (!memory_.is_null()) {
     // Set externally passed ArrayBuffer non neuterable.
     memory_->set_is_neuterable(false);
     memory_->set_is_wasm_buffer(true);
 
     DCHECK_IMPLIES(EnableGuardRegions(),
                    module_->is_asm_js() || memory_->has_guard_region());
   } else if (min_mem_pages > 0) {
@@ skipping 181 matching lines @@
     HandleScope scope(isolate_);
     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(*startup_code, counters_);
+    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 176 matching lines @@
         Handle<Code> import_wrapper = UnwrapOrCompileImportWrapper(
             isolate_, index, module_->functions[import.index].sig,
             Handle<JSReceiver>::cast(value), module_name, import_name,
             module_->get_origin(), &imported_wasm_instances);
         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(*import_wrapper, counters_);
+        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 564 matching lines @@
                                  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 AsyncCompileJob::Start() {
   DoAsync<DecodeModule>();  // --
 }
 
 AsyncCompileJob::~AsyncCompileJob() {
   background_task_manager_.CancelAndWait();
   for (auto d : deferred_handles_) delete d;
 }
@@ skipping 104 matching lines @@
  public:
   DecodeModule() : CompileStep(1) {}
 
   void RunInBackground() override {
     ModuleResult result;
     {
       DisallowHandleAllocation no_handle;
       DisallowHeapAllocation no_allocation;
       // Decode the module bytes.
       TRACE_COMPILE("(1) Decoding module...\n");
-      constexpr bool is_sync = true;
-      result = DecodeWasmModule(job_->isolate_, job_->wire_bytes_.start(),
-                                job_->wire_bytes_.end(), false, kWasmOrigin,
-                                !is_sync);
+      result = AsyncDecodeWasmModule(job_->isolate_, job_->wire_bytes_.start(),
+                                     job_->wire_bytes_.end(), false,
+                                     kWasmOrigin, job_->counters_shared_);

[Mircea Trofin, 2017/06/22 18:42:24]

why not the accessor call (job_->counters()) ?

[kschimpf, 2017/06/22 20:38:31]

Because the API expected a shared_ptr<Counters>. However, I changed the API
since this (now) the only calling place.

     }
     if (result.failed()) {
       // Decoding failure; reject the promise and clean up.
       job_->DoSync<DecodeFail>(std::move(result));
     } else {
       // Decode passed.
       job_->DoSync<PrepareAndStartCompile>(std::move(result.val));
     }
   }
 };
@@ skipping 65 matching lines @@
 
     // Initialize {code_table_} with the illegal builtin. All call sites
     // will be patched at instantiation.
     Handle<Code> illegal_builtin = job_->isolate_->builtins()->Illegal();
     // TODO(wasm): Fix this for lazy compilation.
     for (uint32_t i = 0; i < module_->functions.size(); ++i) {
       job_->code_table_->set(static_cast<int>(i), *illegal_builtin);
       job_->temp_instance_->function_code[i] = illegal_builtin;
     }
 
-    job_->isolate_->counters()->wasm_functions_per_wasm_module()->AddSample(
+    job_->counters()->wasm_functions_per_wasm_module()->AddSample(
         static_cast<int>(module_->functions.size()));
 
     // Transfer ownership of the {WasmModule} to the {ModuleCompiler}, but
     // keep a pointer.
     WasmModule* module = module_.get();
     constexpr bool is_sync = true;
     job_->compiler_.reset(
         new ModuleCompiler(job_->isolate_, std::move(module_), !is_sync));
     job_->compiler_->EnableThrottling();
 
@@ skipping 133 matching lines @@
 // Step 5 (sync): Finish heap-allocated data structures.
 //==========================================================================
 class AsyncCompileJob::FinishCompile : public CompileStep {
   void RunInForeground() override {
     TRACE_COMPILE("(5b) Finish compile...\n");
     HandleScope scope(job_->isolate_);
     // At this point, compilation has completed. Update the code table.
     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(code, job_->counters_);
+      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
     //      for asynchronous compilation and store it as an external one
@@ skipping 55 matching lines @@
     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(*wrapper_code, job_->counters_);
+      RecordStats(*wrapper_code, job_->counters());
       func_index++;
     }
 
     job_->DoSync<FinishModule>();
   }
 };
 
 //==========================================================================
 // Step 7 (sync): Finish the module and resolve the promise.
 //==========================================================================
 class AsyncCompileJob::FinishModule : public CompileStep {
   void RunInForeground() override {
     TRACE_COMPILE("(7) Finish module...\n");
     HandleScope scope(job_->isolate_);
     Handle<WasmModuleObject> result =
         WasmModuleObject::New(job_->isolate_, job_->compiled_module_);
     // {job_} is deleted in AsyncCompileSucceeded, therefore the {return}.
     return job_->AsyncCompileSucceeded(result);
   }
 };
 }  // namespace wasm
 }  // namespace internal
 }  // namespace v8