core-jit:

runtime/vm/compilation_trace.cc

Index: runtime/vm/compilation_trace.cc
diff --git a/runtime/vm/compilation_trace.cc b/runtime/vm/compilation_trace.cc
index f972dfa1f9d86aadd048a4543f7b1a5ac302014c..8e7146c0de5ee672d850f2ca80ffe8124e3f1d0a 100644
--- a/runtime/vm/compilation_trace.cc
+++ b/runtime/vm/compilation_trace.cc
@@ -22,6 +22,11 @@ CompilationTraceSaver::CompilationTraceSaver(Zone* zone)
 
 void CompilationTraceSaver::Visit(const Function& function) {
   if (!function.HasCode()) {
+    return;  // Not compiled.
+  }
+  if (function.parent_function() != Function::null()) {
+    // Lookup works poorly for local functions. We compile all local functions
+    // in a compiled function instead.
     return;
   }
 
@@ -52,20 +57,33 @@ CompilationTraceLoader::CompilationTraceLoader(Thread* thread)
       error_(Object::Handle(zone_)) {}
 
 
-RawObject* CompilationTraceLoader::CompileTrace(char* buffer) {
+static char* FindCharacter(char* str, char goal, char* limit) {
+  while (str < limit) {
+    if (*str == goal) {
+      return str;
+    }
+    str++;
+  }
+  return NULL;
+}
+
+
+RawObject* CompilationTraceLoader::CompileTrace(uint8_t* buffer,
+                                                intptr_t size) {
   // First compile functions named in the trace.
-  char* cursor = buffer;
-  while (cursor != NULL) {
+  char* cursor = reinterpret_cast<char*>(buffer);
+  char* limit = cursor + size;
+  while (cursor < limit) {
     char* uri = cursor;
-    char* comma1 = strchr(uri, ',');
+    char* comma1 = FindCharacter(uri, ',', limit);

[zra, 2017/06/08 03:10:30]

Is memchr() portable?

[rmacnak, 2017/06/08 22:15:09]

I think so, but it uses a length that would need to be updated at each step
instead of a constant limit.

     if (comma1 == NULL) break;

[zra, 2017/06/08 03:10:30]

Curly braces for single-line ifs.

[rmacnak, 2017/06/08 22:15:09]

Done.

     *comma1 = 0;
     char* cls_name = comma1 + 1;
-    char* comma2 = strchr(cls_name, ',');
+    char* comma2 = FindCharacter(cls_name, ',', limit);
     if (comma2 == NULL) break;
     *comma2 = 0;
     char* func_name = comma2 + 1;
-    char* newline = strchr(func_name, '\n');
+    char* newline = FindCharacter(func_name, '\n', limit);
     if (newline == NULL) break;
     *newline = 0;
     error_ = CompileTriple(uri, cls_name, func_name);
@@ -94,16 +112,22 @@ RawObject* CompilationTraceLoader::CompileTrace(char* buffer) {
     }
   }
 
-  // Finally, compile closures in all compiled functions.
+  // Finally, compile closures in all compiled functions. Compiling inner
+  // functions can cause inner inner functions to be registered, so iterate
+  // to a fixed point.
   const GrowableObjectArray& closure_functions = GrowableObjectArray::Handle(
       zone_, thread_->isolate()->object_store()->closure_functions());
-  for (intptr_t i = 0; i < closure_functions.Length(); i++) {
-    function_ ^= closure_functions.At(i);
-    function2_ = function_.parent_function();
-    if (function2_.HasCode()) {
-      error_ = CompileFunction(function_);
-      if (error_.IsError()) {
-        return error_.raw();
+  intptr_t previous_length = 0;
+  while (closure_functions.Length() != previous_length) {
+    previous_length = closure_functions.Length();
+    for (intptr_t i = 0; i < closure_functions.Length(); i++) {

[zra, 2017/06/08 03:10:30]

How is closure_functions modified by CompileFunction()? The way this loop is
written it looks like a lot of compilation is being repeated (the loop always
starts from 0 instead of where you left off the last time). Maybe add a comment
to clarify.

[rmacnak, 2017/06/08 22:15:09]

CompileFunction may append to this list, since we register closure functions in
the after building its parent's flow graph.
Undid this because we're safe as long as we don't cache the length for the loop;
added comment to this effect.

+      function_ ^= closure_functions.At(i);
+      function2_ = function_.parent_function();
+      if (function2_.HasCode()) {
+        error_ = CompileFunction(function_);
+        if (error_.IsError()) {
+          return error_.raw();
+        }
       }
     }
   }
@@ -158,8 +182,8 @@ RawObject* CompilationTraceLoader::CompileTriple(const char* uri_cstr,
 
     function_ = cls_.LookupFunctionAllowPrivate(function_name_);
     field_ = cls_.LookupFieldAllowPrivate(function_name_);
-    if (field_.IsNull() && is_getter) {
-      // Maybe this is a tear off.
+    if (function_.IsNull() && is_getter) {
+      // Maybe this was a tear off.
       add_closure = true;
       function_name2_ = Field::NameFromGetter(function_name_);
       function_ = cls_.LookupFunctionAllowPrivate(function_name2_);