Expose allocation tracing over service protocol

runtime/vm/service.cc

 // Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "vm/service.h"
 
 #include "include/dart_api.h"
 #include "platform/globals.h"
 
 #include "vm/compiler.h"
@@ skipping 241 matching lines @@
   // Skip the dash.
   s++;
   // Extract the PC.
   if (!GetUnsignedIntegerId(s, address, 16)) {
     return false;
   }
   return true;
 }
 
 
+// Verifies that |s| begins with |directory| and then calls |GetIntegerId| on
+// the remainder of |s|.
+// NOTE: Callers are required to include a trailing / in |directory|.
+static bool GetIntegerIdFromDirectory(const char* s,
+                                      const char* directory,
+                                      intptr_t* service_id) {

[turnidge, 2015/07/13 23:37:49]

The word Directory here bothers me some.

How about changing the function name, maybe to something like
GetPrefixedIntegerId?  And directory -> prefix.  Also, instead of requiring the
/, how about the function just knows to expect a / separator?

[Cutch, 2015/07/14 15:53:47]

Done.

+  ASSERT(s != NULL);
+  ASSERT(directory != NULL);
+  const intptr_t kInputLen = strlen(s);
+  const intptr_t kDirectoryLen = strlen(directory);
+  ASSERT(kDirectoryLen > 0);
+  ASSERT(directory[kDirectoryLen - 1] == '/');
+  if (kInputLen <= kDirectoryLen) {
+    return false;
+  }
+  if (strncmp(s, directory, kDirectoryLen) != 0) {
+    return false;
+  }
+  // Directory prefix satisfied. Move forward.
+  s += kDirectoryLen;
+  // Attempt to read integer id.
+  return GetIntegerId(s, service_id);
+}
+
+
+static bool ValidateClassId(Isolate* isolate, intptr_t cid) {

[turnidge, 2015/07/13 23:37:49]

Change name to IsValidClassId?

[Cutch, 2015/07/14 15:53:47]

Done.

+  ASSERT(isolate != NULL);
+  ClassTable* class_table = isolate->class_table();
+  ASSERT(class_table != NULL);
+  return class_table->IsValidIndex(cid) && class_table->HasValidClassAt(cid);
+}
+
+
+static bool ValidateClassId(Isolate* isolate, const char* class_id) {
+  intptr_t cid;
+  if (!GetIntegerIdFromDirectory(class_id, "classes/", &cid)) {
+    return false;
+  }
+  return ValidateClassId(isolate, cid);
+}

[turnidge, 2015/07/13 23:37:49]

I would like you to get rid of the overloaded versions of ValidateClassId and
GetClassForId that take a const char*, as they encourage duplicate parsing.  See
the note in GetCpuProfile, below.

[Cutch, 2015/07/14 15:53:47]

Done.

+
+
+static RawClass* GetClassForId(Isolate* isolate, intptr_t cid) {
+  ASSERT(isolate == Isolate::Current());
+  ASSERT(isolate != NULL);
+  ClassTable* class_table = isolate->class_table();
+  ASSERT(class_table != NULL);
+  return class_table->At(cid);
+}
+
+
+static RawClass* GetClassForId(Isolate* isolate, const char* class_id) {
+  intptr_t cid;
+  ASSERT(GetIntegerIdFromDirectory(class_id, "classes/", &cid));

[turnidge, 2015/07/13 23:37:49]

Do ASSERTs get removed in release builds?  I don't think that required
functionality should be in an ASSERT.

[Cutch, 2015/07/14 15:53:47]

Good catch.

+  return GetClassForId(isolate, cid);
+}
+
+
 // TODO(johnmccutchan): Split into separate file and write unit tests.
 class MethodParameter {
  public:
   MethodParameter(const char* name, bool required)
       : name_(name), required_(required) {
   }
 
   virtual ~MethodParameter() { }
 
   virtual bool Validate(const char* value) const {
@@ skipping 63 matching lines @@
     }
     for (const char* cp = value; *cp != '\0'; cp++) {
       if (*cp < '0' || *cp > '9') {
         return false;
       }
     }
     return true;
   }
 
   static intptr_t Parse(const char* value) {
+    if (value == NULL) {
+      return -1;
+    }
     char* end_ptr = NULL;
     uintptr_t result = strtoul(value, &end_ptr, 10);
     ASSERT(*end_ptr == '\0');  // Parsed full string
     return result;
   }
 };
 
 
 class IdParameter : public MethodParameter {
  public:
@@ skipping 1870 matching lines @@
   Profile::kUser,
   Profile::kVMUser,
   Profile::kVM,
   Profile::kNoTags,  // Default value.
 };
 
 
 static const MethodParameter* get_cpu_profile_params[] = {
   ISOLATE_PARAMETER,
   new EnumParameter("tags", true, tags_enum_names),
+  new IdParameter("classId", false),
   NULL,
 };
 
 
 static bool GetCpuProfile(Isolate* isolate, JSONStream* js) {

[turnidge, 2015/07/13 23:37:49]

As mentioned elsewhere, consider separating cpu and class allocation profiling
rpcs or rename this to something more general.

[Cutch, 2015/07/14 15:53:47]

Done.

   Profile::TagOrder tag_order =
       EnumMapper(js->LookupParam("tags"), tags_enum_names, tags_enum_values);
-  ProfilerService::PrintJSON(js, tag_order);
+  const char* class_id = js->LookupParam("classId");
+  if (ValidateClassId(isolate, class_id)) {
+    const Class& cls = Class::Handle(GetClassForId(isolate, class_id));

[turnidge, 2015/07/13 23:37:49]

You are double-parsing the class id, once in ValidateClassId and once in
GetClassForId.

I would rather see something like this:

intptr_t cid = GetPrefixedIntegerId(..., "classes", class_id);
if (IsValidClassId(cid) {
  const Class& cls = Class::Handle(GetClassForId(isolate, cid);
}

Similarly in TraceClassAllocation

[Cutch, 2015/07/14 15:53:47]

Done.

+    ProfilerService::PrintAllocationJSON(js, tag_order, cls);
+  } else {
+    ProfilerService::PrintJSON(js, tag_order);
+  }
   return true;
 }
 
 
 static const MethodParameter* clear_cpu_profile_params[] = {
   ISOLATE_PARAMETER,
   NULL,
 };
 
 
@@ skipping 491 matching lines @@
   isolate->set_debugger_name(js->LookupParam("name"));
   if (Service::NeedsIsolateEvents()) {
     ServiceEvent event(isolate, ServiceEvent::kIsolateUpdate);
     Service::HandleEvent(&event);
   }
   PrintSuccess(js);
   return true;
 }
 
 
+static const MethodParameter* trace_class_allocation_params[] = {
+  ISOLATE_PARAMETER,
+  new IdParameter("classId", true),
+  new BoolParameter("enable", true),
+  NULL,
+};
+
+
+static bool TraceClassAllocation(Isolate* isolate, JSONStream* js) {
+  const char* class_id = js->LookupParam("classId");
+  const bool enable = BoolParameter::Parse(js->LookupParam("enable"));
+
+  if (!ValidateClassId(isolate, class_id)) {
+    PrintInvalidParamError(js, "cid");
+    return true;
+  }
+
+  const Class& cls = Class::Handle(GetClassForId(isolate, class_id));
+  ASSERT(!cls.IsNull());
+
+  cls.SetTraceAllocation(enable);
+  PrintSuccess(js);
+  return true;
+}
+
+
 static ServiceMethodDescriptor service_methods_[] = {
   { "_dumpIdZone", DumpIdZone, NULL },
   { "_echo", Echo,
     NULL },
   { "_respondWithMalformedJson", RespondWithMalformedJson,
     NULL },
   { "_respondWithMalformedObject", RespondWithMalformedObject,
     NULL },
   { "_triggerEchoEvent", TriggerEchoEvent,
     NULL },
@@ skipping 66 matching lines @@
   { "_requestHeapSnapshot", RequestHeapSnapshot,
     request_heap_snapshot_params },
   { "_setExceptionPauseInfo", SetExceptionPauseInfo,
     set_exception_pause_info_params },
   { "_setFlag", SetFlag,
     set_flags_params },
   { "setLibraryDebuggable", SetLibraryDebuggable,
     set_library_debuggable_params },
   { "setName", SetName,
     set_name_params },
+  { "_traceClassAllocation", TraceClassAllocation,

[turnidge, 2015/07/13 23:37:49]

Consider changing rpc name for consistency with setLibraryDebuggable -- perhaps
setTraceClassAllocation?

[Cutch, 2015/07/14 15:53:48]

Done.

+    trace_class_allocation_params },
 };
 
 
 ServiceMethodDescriptor* FindMethod(const char* method_name) {
   intptr_t num_methods = sizeof(service_methods_) /
                          sizeof(service_methods_[0]);
   for (intptr_t i = 0; i < num_methods; i++) {
     ServiceMethodDescriptor& method = service_methods_[i];
     if (strcmp(method_name, method.name) == 0) {
       return &method;
     }
   }
   return NULL;
 }
 
 
 }  // namespace dart