Collect type profile for DevTools.

src/compiler.cc

 // Copyright 2012 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/compiler.h"
 
 #include <algorithm>
 #include <memory>
 
 #include "src/asmjs/asm-js.h"
@@ skipping 303 matching lines @@
   // when the function is not compiled (i.e. no code was serialized).
 
   // TODO(mvstanton): reintroduce is_empty() predicate to feedback_metadata().
   if (info->shared_info()->feedback_metadata()->length() == 0 ||
       !info->shared_info()->is_compiled()) {
     Handle<FeedbackMetadata> feedback_metadata = FeedbackMetadata::New(
         info->isolate(), info->literal()->feedback_vector_spec());
     info->shared_info()->set_feedback_metadata(*feedback_metadata);
   }
 
+  // CollectTypeProfile uses its own feedback slot. The decision
+  // whether we collect type profile or not must match the
+  // feedback metadata.
+  // If we have not decided whether we collect type feedback, we
+  // make a decision based on the FLAG. Otherwise we cannot change it.
+  if (!info->shared_info()->computed_collects_type_profile()) {

[Yang, 2017/03/09 10:28:27]

Michael, here's where I'd like you to take a closer look. What we want to
achieve here is:
- We can freely make the decision whether to collect type profile at the first
compile. This decision is made based on a command line flag.
- After the first compile, we need to persist the decision somehow, so that any
subsequent compiles do not conflict with existing feedback metadata.
- We can run into the situation of compiling with existing feedback metadata if
we compile a function from the snapshot, since snapshotting discards the code.

Currently this decision is persisted on the shared function info. So we are
assuming that this is the correct place to keep this decision in sync with the
feedback metadata.

But I wonder whether we persist this decision even closer to the feedback
metadata. For example, if we could figure out the previous decision from looking
at feedback metadata itself, that would be a lot nicer.

[mvstanton, 2017/03/10 12:12:24]

Yep, it makes sense to avoid these flags and just iterate the feedback metadata
looking for your special slot. If you find one, then the metadata was created at
a moment when you wanted type profiling, so now you know that.

[Franzi, 2017/03/10 14:33:49]

I deleted the constants and instead check the FeedbackMetadata. 

+    info->shared_info()->set_collects_type_profile(FLAG_type_profile);
+    info->shared_info()->set_computed_collects_type_profile(true);
+  }
+
   // It's very important that recompiles do not alter the structure of the type
   // feedback vector. Verify that the structure fits the function literal.
   CHECK(!info->shared_info()->feedback_metadata()->SpecDiffersFrom(
       info->literal()->feedback_vector_spec()));
 }
 
 bool UseTurboFan(Handle<SharedFunctionInfo> shared) {
   bool must_use_ignition_turbo = shared->must_use_ignition_turbo();
 
   // Check the enabling conditions for Turbofan.
@@ skipping 137 matching lines @@
   }
   if (literal->flags() & AstProperties::kMustUseIgnitionTurbo) {
     shared_info->set_must_use_ignition_turbo(true);
   }
 }
 
 bool Renumber(ParseInfo* parse_info,
               Compiler::EagerInnerFunctionLiterals* eager_literals) {
   RuntimeCallTimerScope runtimeTimer(parse_info->isolate(),
                                      &RuntimeCallStats::CompileRenumber);
+
+  // CollectTypeProfile uses its own feedback slot. The decision
+  // whether we collect type profile or not must match the
+  // feedback metadata.
+  // If we parse for the first time or if we have not decided whether we collect
+  // type feedback, we make a decision based on the FLAG. Otherwise we stick
+  // with the previous decision.
+  bool collect_type_profile =
+      (FLAG_type_profile &&
+       (parse_info->shared_info().is_null() ||
+        !parse_info->shared_info()->computed_collects_type_profile())) ||
+      (!parse_info->shared_info().is_null() &&
+       parse_info->shared_info()->computed_collects_type_profile() &&
+       parse_info->shared_info()->collects_type_profile());

[Yang, 2017/03/09 10:28:27]

This expression seems correct. But can we make this a bit more readable?
Something like

if (parse_info->shared_info().is_null() ||
!parse_info->shared_info()->computed_collects_type_profile())) {
  collect_type_profile = FLAG_type_profile
} else {
  collect_type_profile = parse_info->shared_info()->collects_type_profile();
}

[Franzi, 2017/03/10 12:14:28]

Thanks for the suggestions. Done. 

+
   if (!AstNumbering::Renumber(
           parse_info->isolate()->stack_guard()->real_climit(),
-          parse_info->zone(), parse_info->literal(), eager_literals)) {
+          parse_info->zone(), parse_info->literal(), eager_literals,
+          collect_type_profile)) {
     return false;
   }
   if (!parse_info->shared_info().is_null()) {
     SetSharedFunctionFlagsFromLiteral(parse_info->literal(),
                                       parse_info->shared_info());
   }
   return true;
 }
 
 bool GenerateUnoptimizedCode(CompilationInfo* info) {
@@ skipping 1444 matching lines @@
   }
 
   if (shared->is_compiled()) {
     // TODO(mvstanton): pass pretenure flag to EnsureLiterals.
     JSFunction::EnsureLiterals(function);
   }
 }
 
 }  // namespace internal
 }  // namespace v8