Added isolate + thread high watermark tracking to Observatory

runtime/vm/isolate.cc

Index: runtime/vm/isolate.cc
diff --git a/runtime/vm/isolate.cc b/runtime/vm/isolate.cc
index 8304532470999c81ea515843ad6496c8348db0f3..a27985253dc8738de6abb23e229d91ac0cb3828c 100644
--- a/runtime/vm/isolate.cc
+++ b/runtime/vm/isolate.cc
@@ -776,6 +776,7 @@ Isolate::Isolate(const Dart_IsolateFlags& api_flags)
       single_step_(false),
       thread_registry_(new ThreadRegistry()),
       safepoint_handler_(new SafepointHandler(this)),
+      isolate_memory_high_watermark_(0),
       message_notify_callback_(NULL),
       name_(NULL),
       debugger_name_(NULL),
@@ -2103,6 +2104,8 @@ void Isolate::PrintJSON(JSONStream* stream, bool ref) {
     }
   }
 
+  jsobj.AddProperty("isolateMemoryHighWatermark",

[Cutch, 2017/01/03 20:58:54]

Please make this private.

[bkonyi, 2017/01/03 22:00:38]

Done.

+                    isolate_memory_high_watermark_);
   jsobj.AddProperty("threads", thread_registry_);

[Cutch, 2017/01/03 20:58:54]

this too.

[bkonyi, 2017/01/03 22:00:38]

Done.

 }
 #endif
@@ -2653,6 +2656,8 @@ Thread* Isolate::ScheduleThread(bool is_mutator, bool bypass_safepoint) {
     thread = thread_registry()->GetFreeThreadLocked(this, is_mutator);
     ASSERT(thread != NULL);
 
+    thread->ResetHighWatermark();

[Cutch, 2017/01/03 20:58:54]

why would we be resetting the high watermark when it's scheduled? Doesn't this
lose information?

[bkonyi, 2017/01/03 22:00:38]

I was thinking once a thread was no longer being used and is part of the free
thread list, we would want to clear the watermark information before the thread
is reused later for a new task. If that's not correct, should I ever be clearing
the watermarks?

[Cutch, 2017/01/03 22:28:00]

No this was me remembering the way the threads *used* to be structured. I went
and read the code again and I believe your code here is correct.

+
     // Set up other values and set the TLS value.
     thread->isolate_ = this;
     ASSERT(heap() != NULL);
@@ -2699,6 +2704,7 @@ void Isolate::UnscheduleThread(Thread* thread,
     // Ensure that the thread reports itself as being at a safepoint.
     thread->EnterSafepoint();
   }
+  UpdateIsolateHighWatermark();
   OSThread* os_thread = thread->os_thread();
   ASSERT(os_thread != NULL);
   os_thread->DisableThreadInterrupts();
@@ -2719,6 +2725,15 @@ void Isolate::UnscheduleThread(Thread* thread,
 }
 
 
+void Isolate::UpdateIsolateHighWatermark() {
+  intptr_t thread_watermarks_total =

[Cutch, 2017/01/03 20:58:54]

const intptr_t ...

Also, try and use more descriptive names that are consistent with each other.

For example:

UpdateIsolateHighWatermark() and isolate_memory_high_watermark_

The field `isolate_memory_high_watermark_` is already inside the isolate so drop
the isolate_: `memory_high_watermark_`.

Similarly with the function name. It's already defined on the Isolate class so
maybe call it UpdateMemoryHighWatermark()

[bkonyi, 2017/01/03 22:00:38]

Done.

+      thread_registry()->ThreadHighWatermarksTotalLocked();
+  if (thread_watermarks_total > isolate_memory_high_watermark_) {
+    isolate_memory_high_watermark_ = thread_watermarks_total;
+  }
+}
+
+
 static RawInstance* DeserializeObject(Thread* thread,
                                       uint8_t* obj_data,
                                       intptr_t obj_len) {