Unflake CPU profiler tests.

test/cctest/profiler-extension.cc

Index: test/cctest/profiler-extension.cc
diff --git a/test/cctest/profiler-extension.cc b/test/cctest/profiler-extension.cc
index a917932978c128e27b044da453facbcdf8b4f506..5e4917c83f0c2e397fbaba4fc2a614ba0535e616 100644
--- a/test/cctest/profiler-extension.cc
+++ b/test/cctest/profiler-extension.cc
@@ -37,31 +37,33 @@ namespace internal {
 v8::CpuProfile* ProfilerExtension::last_profile = NULL;
 const char* ProfilerExtension::kSource =
     "native function startProfiling();"
-    "native function stopProfiling();";
+    "native function stopProfiling();"
+    "native function collectSample();";
+
+namespace {
+v8::Local<v8::String> v8_str(v8::Isolate* isolate, const char* string) {

[yurys, 2016/02/05 17:33:04]

Please use the one defined in v8/test/cctest/cctest.h instead.

[alph, 2016/02/05 17:52:26]

Done that, just forgot to upload. ;-)

+  return v8::String::NewFromUtf8(isolate, string, v8::NewStringType::kNormal)
+      .ToLocalChecked();
+}
+}  // namespace
 
 v8::Local<v8::FunctionTemplate> ProfilerExtension::GetNativeFunctionTemplate(
     v8::Isolate* isolate, v8::Local<v8::String> name) {
   v8::Local<v8::Context> context = isolate->GetCurrentContext();
-  if (name->Equals(context, v8::String::NewFromUtf8(isolate, "startProfiling",
-                                                    v8::NewStringType::kNormal)
-                                .ToLocalChecked())
-          .FromJust()) {
+  if (name->Equals(context, v8_str(isolate, "startProfiling")).FromJust()) {
     return v8::FunctionTemplate::New(isolate,
                                      ProfilerExtension::StartProfiling);
-  } else if (name->Equals(context,
-                          v8::String::NewFromUtf8(isolate, "stopProfiling",
-                                                  v8::NewStringType::kNormal)
-                              .ToLocalChecked())
-                 .FromJust()) {
-    return v8::FunctionTemplate::New(isolate,
-                                     ProfilerExtension::StopProfiling);
-  } else {
-    CHECK(false);
-    return v8::Local<v8::FunctionTemplate>();
   }
+  if (name->Equals(context, v8_str(isolate, "stopProfiling")).FromJust()) {
+    return v8::FunctionTemplate::New(isolate, ProfilerExtension::StopProfiling);
+  }
+  if (name->Equals(context, v8_str(isolate, "collectSample")).FromJust()) {
+    return v8::FunctionTemplate::New(isolate, ProfilerExtension::CollectSample);
+  }
+  CHECK(false);
+  return v8::Local<v8::FunctionTemplate>();
 }
 
-
 void ProfilerExtension::StartProfiling(
     const v8::FunctionCallbackInfo<v8::Value>& args) {
   last_profile = NULL;
@@ -71,7 +73,6 @@ void ProfilerExtension::StartProfiling(
       : v8::String::Empty(args.GetIsolate()));
 }
 
-
 void ProfilerExtension::StopProfiling(
     const v8::FunctionCallbackInfo<v8::Value>& args) {
   v8::CpuProfiler* cpu_profiler = args.GetIsolate()->GetCpuProfiler();
@@ -80,5 +81,10 @@ void ProfilerExtension::StopProfiling(
       : v8::String::Empty(args.GetIsolate()));
 }
 
+void ProfilerExtension::CollectSample(
+    const v8::FunctionCallbackInfo<v8::Value>& args) {
+  args.GetIsolate()->GetCpuProfiler()->CollectSample();

[yurys, 2016/02/05 17:33:04]

The whole idea of the tests that now call this native method was to test the
sampler behavior with various stack states. When deepest js function simply
calls native method, we will always have exit frame on top of the stack and no
js frames above it. This is just one deterministic case among those we wanted to
cover. Can we instead feed the sampler with an artificially constructed top
frame (as we discussed a while ago) and probe into different pc positions inside
js functions of interest?

[alph, 2016/02/05 17:52:26]

The tests I'm unflaking are mostly checking the stack collection for various
combinations of stack frames (e.g. js-native-js, call, apply, deep stack, etc).
And yes they can be done deterministically.
The one you're suggesting is another test that I agree is worth adding. However
it doesn't seem to belong to this patch, which intent is to enable important
profiler tests currently disabled.
wdyt?

[alph, 2016/02/05 18:03:33]

Ok, how about I drop the CheckChildrenNames which I believe is the main source
of flakiness, while keep the stochastic sampling as is?

[yurys, 2016/02/05 18:07:33]

Well, the tests like JsNative1JsNative2JsSample are becoming ones always ending
in a native call, i.e.
JsNative1JsNative2JsSampleNative. In that case it can be simplified even further
to be just JsNative1JsNative2, right?
I agree that it is important to find a way to fix and enable those test as
having them skipped adds little value.

+}
+
 }  // namespace internal
 }  // namespace v8