Collect only optimizable function samples....

src/runtime-profiler.cc

Index: src/runtime-profiler.cc
===================================================================
--- src/runtime-profiler.cc	(revision 5946)
+++ src/runtime-profiler.cc	(working copy)
@@ -88,7 +88,14 @@
 static int sampler_threshold = kSamplerThresholdInit;
 static int sampler_threshold_size_factor = kSamplerThresholdSizeFactorInit;
 
+static const int kStateWindowSize = 128;
+static bool state_window[kStateWindowSize];

[Kasper Lund, 2010/12/08 14:54:15]

To me the name is of this is somewhat weird. If it's a state window it should
contain states. Maybe it would be nicer with an enum SampleState { JS, NON_JS }
and have a window of those? It's not clear what true means - and what false
means.

+static int state_window_position = 0;
+static int in_JS_state = 0;

[Kasper Lund, 2010/12/08 14:54:15]

static int state_counts[2] = { 0, } and then use state_counts[state] to get to
either in_JS_state or not_in_JS_state.

+static int not_in_JS_state = 0;
 
+static int sampler_ticks = kSamplerTicksDelta;

[Kasper Lund, 2010/12/08 14:54:15]

This name doesn't really relay much information. It's the number of ticks? Hmm -
how about something ala 'sampler_ticks_until_threshold_adjustment'?

+
 // The JSFunctions in the sampler window are not GC safe. Old-space
 // pointers are not cleared during mark-sweep collection and therefore
 // the window might contain stale pointers. The window is updated on
@@ -257,6 +264,9 @@
   }
   optimize_soon_list = NULL;
 
+  float js_ratio = in_JS_state / static_cast<float>(

[Kasper Lund, 2010/12/08 14:54:15]

Maybe comment on what this ratio means or change the variable name to make it
(even) more obvious.

+      in_JS_state + not_in_JS_state);
+
   // Run through the JavaScript frames and collect them. If we already
   // have a sample of the function, we mark it for optimizations
   // (eagerly or lazily).
@@ -267,16 +277,39 @@
        it.Advance()) {
     JavaScriptFrame* frame = it.frame();
     JSFunction* function = JSFunction::cast(frame->function());
+
+    if (sampler_ticks > 0) {

[Kasper Lund, 2010/12/08 14:54:15]

Add a comment that explains what this does.

+      sampler_ticks--;
+      if (sampler_ticks == 0) {
+        if (sampler_threshold > kSamplerThresholdMin) {
+          sampler_threshold -= kSamplerThresholdDelta;
+        }
+        sampler_ticks = kSamplerTicksDelta;
+      }
+    }
+
+    if (!IsOptimizable(function) && !function->IsMarkedForLazyRecompilation()) {

[Kasper Lund, 2010/12/08 14:54:15]

Please explain what you're trying to filter out here through a comment.

+      continue;
+    }
+
+    samples[count++] = function;
+
     int function_size = function->shared()->SourceSize();
     int threshold_size_factor;
-    if (function_size > kSizeLimit) {
-      threshold_size_factor = sampler_threshold_size_factor;
-    } else {
-      threshold_size_factor = 1;
+    threshold_size_factor = (function_size > kSizeLimit)

[Kasper Lund, 2010/12/08 14:54:15]

Why not initialize threshold_size_factor directly in the declaration of the
variable?

+        ? sampler_threshold_size_factor
+        : 1;
+
+    int threshold = sampler_threshold * threshold_size_factor;
+
+    if (js_ratio < .2) {

[fschneider, 2010/12/08 14:33:33]

These constants need some comments about the rationale behind.

[Kasper Lund, 2010/12/08 14:54:15]

0.2, 0.5, 0.75. I agree that you should explain the rationale behind the
threshold adjustments. Why does it make sense?

+      threshold = kStateWindowSize;
+    } else if (js_ratio < .5) {
+      threshold *= 3;
+    } else if (js_ratio < .75) {
+      threshold *= 2;
     }
 
-    int threshold = sampler_threshold * threshold_size_factor;
-    samples[count++] = function;
     if (function->IsMarkedForLazyRecompilation()) {
       Code* unoptimized = function->shared()->code();
       int nesting = unoptimized->allow_osr_at_loop_nesting_level();
@@ -284,10 +317,8 @@
       int new_nesting = Min(nesting + 1, Code::kMaxLoopNestingMarker);
       unoptimized->set_allow_osr_at_loop_nesting_level(new_nesting);
     } else if (LookupSample(function) >= threshold) {
-      if (IsOptimizable(function)) {
-        Optimize(function, false, 0);
-        CompilationCache::MarkForEagerOptimizing(Handle<JSFunction>(function));
-      }
+      Optimize(function, false, 0);
+      CompilationCache::MarkForEagerOptimizing(Handle<JSFunction>(function));
     }
   }
 
@@ -332,6 +363,11 @@
 
 
 void RuntimeProfiler::Setup() {
+  for (int i = 0; i < kStateWindowSize; i++) {
+    state_window[i] = false;
+  }
+  in_JS_state = 0;
+  not_in_JS_state = kStateWindowSize;
   ClearSampleBuffer();
   // If the ticker hasn't already started, make sure to do so to get
   // the ticks for the runtime profiler.
@@ -341,6 +377,7 @@
 
 void RuntimeProfiler::Reset() {
   sampler_threshold = kSamplerThresholdInit;
+  sampler_ticks = kSamplerTicksDelta;
   sampler_threshold_size_factor = kSamplerThresholdSizeFactorInit;
 }
 
@@ -360,6 +397,23 @@
 }
 
 
+void AddStateSample(bool in_js_state) {

[Kasper Lund, 2010/12/08 14:54:15]

Use enum SampleState for the parameter.

[Kasper Lund, 2010/12/08 14:54:15]

Make this function static.

+  if (state_window[state_window_position]) {

[Kasper Lund, 2010/12/08 14:54:15]

state_counts[state_window[state_window_position]]--; No branches.

+    in_JS_state--;

[fschneider, 2010/12/08 14:33:33]

is_js_state vs in_JS_state: These variable names are confusing. Please choose
something that does not only differ in capitalization.

+  } else {
+    not_in_JS_state--;
+  }
+  state_window[state_window_position] = in_js_state;
+  if (in_js_state) {

[Kasper Lund, 2010/12/08 14:54:15]

state_counts[state]++; No branches.

+    in_JS_state++;
+  } else {
+    not_in_JS_state++;
+  }
+  state_window_position = (state_window_position + 1) &

[Kasper Lund, 2010/12/08 14:54:15]

ASSERT(IsPowerOf2(kStateWindowSize));

+      (kStateWindowSize - 1);
+}
+
+
 bool RuntimeProfilerRateLimiter::SuspendIfNecessary() {
   static const int kNonJSTicksThreshold = 100;
   // We suspend the runtime profiler thread when not running
@@ -369,8 +423,10 @@
       !CpuProfiler::is_profiling() &&
       !(FLAG_prof && FLAG_prof_auto)) {
     if (Top::IsInJSState()) {
+      AddStateSample(true);
       non_js_ticks_ = 0;
     } else {
+      AddStateSample(false);
       if (non_js_ticks_ < kNonJSTicksThreshold) {
         ++non_js_ticks_;
       } else {