[Compiler] Skip Ignition for asm.js code.

src/compiler.cc

Index: src/compiler.cc
diff --git a/src/compiler.cc b/src/compiler.cc
index d99f8d181b2e53d3407d0eb75ec005777718e749..e0b18a94ca3df2e7a835061c2ba6489d478b8ec7 100644
--- a/src/compiler.cc
+++ b/src/compiler.cc
@@ -939,6 +939,13 @@ MaybeHandle<Code> GetBaselineCode(Handle<JSFunction> function) {
   return info.code();
 }
 
+bool ShouldOptimizeForAsm(Isolate* isolate, Handle<JSFunction> function) {
+  // If the debugger is active, do not compile with turbofan unless we can
+  // deopt from turbofan code.
+  return FLAG_turbo_asm && function->shared()->asm_function() &&
+         (FLAG_turbo_asm_deoptimization || !isolate->debug()->is_active());

[Michael Starzinger, 2016/05/20 11:45:10]

What would you thing about guarding this by FLAG_ignition as well. Because for
the shipping configuration (with OSR support) we abandoned the eager
optimization of asm.js functions.

[rmcilroy, 2016/05/20 12:42:03]

Is there any particularly reason we abandoned it? Given asmJS is meant to be
ahead-of-time style code isn't it better just to optimize first without running
unoptimized code at all (and possibly causing jank mid-way through the app when
we recompile for OSR)?

As I mentioned on the chat, there is an issue with the sqlite tests which are
much slower after this change. Is there type feedback you are relying on in TF
to optmize asmJS code? If so, we should probably just skip Ignition and go
straight to FCG here rather than trying to optimize straight to TF.

[Michael Starzinger, 2016/05/23 14:45:38]

As discussed offline: I am afraid this will tank our shipping configuration. If
we want to change the shipping configuration then I would vote for doing so in a
separate CL (i.e. land this CL with the additional check for FLAG_ignition, then
land a second CL that removes the FLAG_ignition). This way we can at least
revert it easily without having the remove the Ignition path.

+Benedikt, +Ben: WDYT?

[rmcilroy, 2016/05/23 15:07:53]

Sounds good, I've updated this CL to only do this for FLAG_ignition and will
upload another CL for the shipping configuration.

[titzer, 2016/05/23 15:48:34]

It's not type feedback, it's just that TF is too heavyweight for the startup
phase of asm.js code. But OSR is required for the asm.js benchmarks to do well.
So OSR from ignition->TF for asm.js code is probably not in the cards, so maybe
best to just do as you say and do FCG.

+}
+
 MaybeHandle<Code> GetLazyCode(Handle<JSFunction> function) {
   Isolate* isolate = function->GetIsolate();
   DCHECK(!isolate->has_pending_exception());
@@ -971,7 +978,7 @@ MaybeHandle<Code> GetLazyCode(Handle<JSFunction> function) {
   Handle<Code> result;
   ASSIGN_RETURN_ON_EXCEPTION(isolate, result, GetUnoptimizedCode(&info), Code);
 
-  if (FLAG_always_opt) {
+  if (FLAG_always_opt || ShouldOptimizeForAsm(isolate, function)) {

[Michael Starzinger, 2016/05/20 11:45:10]

Can we keep the optimization path for production above the unoptimized compile?
That way we don't needlessly compile unoptimized code without deoptimization
support. For the --always-opt case the trigger is down here to increase code
coverage.

[rmcilroy, 2016/05/20 12:42:03]

Done.

     Handle<Code> opt_code;
     if (GetOptimizedCode(function, Compiler::NOT_CONCURRENT)
             .ToHandle(&opt_code)) {
@@ -1764,9 +1771,11 @@ void Compiler::FinalizeCompilationJob(CompilationJob* raw_job) {
 
 void Compiler::PostInstantiation(Handle<JSFunction> function,
                                  PretenureFlag pretenure) {
+  Isolate* isolate = function->GetIsolate();
   Handle<SharedFunctionInfo> shared(function->shared());
 
-  if (FLAG_always_opt && shared->allows_lazy_compilation()) {
+  if ((FLAG_always_opt || ShouldOptimizeForAsm(isolate, function)) &&

[Michael Starzinger, 2016/05/20 11:45:10]

I am not entirely sure why this is needed. For functions where SFI is not yet
compiled we should hit the GetLazy code path above. For functions where SFI is
already compiled, the optimized code map should hit. Could you elaborate in
this?

[rmcilroy, 2016/05/20 12:42:03]

This is for the eager compilation pipeline (--ignition-eager) without it we
don't optimize on first-run (there is nothing in the optimized code map the
first time we run) and so we run the unoptimized bytecode. 

[rmcilroy, 2016/05/23 09:28:16]

I just realized, this would mark all new functions for optimization even if
there was something already in the code cache (I mistakenly thought --always-opt
would do the right thing but forgot it was a purely testing flag). I've moved
this down to below the optimized code map check. I still have issues with sqlite
timing out though :/.

[Michael Starzinger, 2016/05/23 14:45:38]

Acknowledged. New version in patch set #5 works for me.

+      shared->allows_lazy_compilation()) {
     function->MarkForOptimization();
   }
 
@@ -1782,7 +1791,6 @@ void Compiler::PostInstantiation(Handle<JSFunction> function,
   if (cached.literals != nullptr) {
     function->set_literals(cached.literals);
   } else {
-    Isolate* isolate = function->GetIsolate();
     int number_of_literals = shared->num_literals();
     Handle<LiteralsArray> literals =
         LiteralsArray::New(isolate, handle(shared->feedback_vector()),