Expose allocation tracing over service protocol

runtime/vm/service.cc

Index: runtime/vm/service.cc
diff --git a/runtime/vm/service.cc b/runtime/vm/service.cc
index 4cbc5ddba46f142d49aa4c71cb4551b87d1c3ebb..8e37ce0e0ae8837122232df068bfee1922d2b242 100644
--- a/runtime/vm/service.cc
+++ b/runtime/vm/service.cc
@@ -259,6 +259,64 @@ static bool GetCodeId(const char* s, int64_t* timestamp, uword* address) {
 }
 
 
+// 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:
@@ -342,6 +400,9 @@ class UIntParameter : public MethodParameter {
   }
 
   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
@@ -2232,6 +2293,7 @@ static Profile::TagOrder tags_enum_values[] = {
 static const MethodParameter* get_cpu_profile_params[] = {
   ISOLATE_PARAMETER,
   new EnumParameter("tags", true, tags_enum_names),
+  new IdParameter("classId", false),
   NULL,
 };
 
@@ -2239,7 +2301,13 @@ static const MethodParameter* get_cpu_profile_params[] = {
 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;
 }
 
@@ -2751,6 +2819,32 @@ static bool SetName(Isolate* isolate, JSONStream* js) {
 }
 
 
+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,
@@ -2837,6 +2931,8 @@ static ServiceMethodDescriptor service_methods_[] = {
     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 },
 };