Initial prototype of vmstats support, based on Dart VM Stats draft design doc.

Initial prototype of vmstats support, based on Dart VM Stats draft design doc.
Committed: https://code.google.com/p/dart/source/detail?r=19091

[Tom Ball, 2013-02-05 21:50:08]

There's no need for a deep review, as this is more a progress report.
------------------------------
Updated VM stats service: simplified public API and moved it to
dart_debugger_api.h. Added plug-in service map, and route stats requests to
specified plug-in. Implemented isolate stats as a built-in plug-in in
isolate.cc, verifying routing works. Updated design doc to match this prototype.

Next I'll be working on the client-side Dart app.

[Tom Ball, 2013-02-06 21:19:55]

Added support for web server returning static files.

[Tom Ball, 2013-02-06 23:15:20]

Fixed socket flushing, returning of large static files.

[siva, 2013-02-09 01:00:57]

https://codereview.chromium.org/12221022/diff/13003/runtime/bin/bin.gypi
File runtime/bin/bin.gypi (right):

https://codereview.chromium.org/12221022/diff/13003/runtime/bin/bin.gypi#newc...
runtime/bin/bin.gypi:554: },
Does this have to be a separate static library, can we list vmstats.cc and
vmstats.h along with main.cc in the targets
'dart' and 'dart_no_snapshot'?

Do you see other executables wanting to link with this library?

https://codereview.chromium.org/12221022/diff/13003/runtime/bin/vmstats.cc
File runtime/bin/vmstats.cc (right):

https://codereview.chromium.org/12221022/diff/13003/runtime/bin/vmstats.cc#ne...
runtime/bin/vmstats.cc:1: // Copyright (c) 2012, the Dart project authors. 
Please see the AUTHORS file
2013

https://codereview.chromium.org/12221022/diff/13003/runtime/bin/vmstats.cc#ne...
runtime/bin/vmstats.cc:32: char** env = Platform::Environment(&num_vars);
Are you going to switch this to be a command line value so that people can
specify a root directory location?
If this is not specified then we pick the SDK-specific path to the root
directory.

https://codereview.chromium.org/12221022/diff/13003/runtime/bin/vmstats.cc#ne...
runtime/bin/vmstats.cc:48: ServerSocket::CreateBindListen("127.0.0.1", port,
BACKLOG);
fine for now but we may want to make 127.0.0.1 a parameter too. The command line
option could be specified as
--vmstats_port=8080 or
--vmstats_port=hostname:8080

https://codereview.chromium.org/12221022/diff/13003/runtime/bin/vmstats.cc#ne...
runtime/bin/vmstats.cc:72: Dart_Isolate isolate) {
.... indentation.

https://codereview.chromium.org/12221022/diff/13003/runtime/bin/vmstats.cc#ne...
runtime/bin/vmstats.cc:139: int len = read(socket, buffer, BUFSIZE);
How do you deal with partial reads here
(e.g: the read could have just read 5 bytes are returned)

The protocol states:
"The Request-Line begins with a method token, followed by the Request-URI and
the protocol version, and ending with CRLF. The elements are separated by SP
characters. No CR or LF is allowed except in the final CRLF sequence."

You should probably read here in a loop till you get the entire request line or
have a ReadRequest method that ensures that you get a complete request.

https://codereview.chromium.org/12221022/diff/13003/runtime/bin/vmstats.cc#ne...
runtime/bin/vmstats.cc:156: Socket::Write(socket, response, strlen(response));
In all these writes I would assert that the returned value is equal to the
number of bytes requested to be written.

You could have helper methods WriteResponse, WriteErrorResponse(error_code (403,
404 etc.)) and move all the HTTP stuff in there. Would make this loop code less
verbose.

https://codereview.chromium.org/12221022/diff/13003/runtime/bin/vmstats.cc#ne...
runtime/bin/vmstats.cc:174: instance_->MemoryUsed(&stream);
Instead of MemoryUsed you could probably call this
IsolateStatus or ToplevelIsolateStatus so that it can return
all the top level information available for the isolate which could potentially
include:
- script/library/class/method of the function used for spawning the isolate
- memory usage
- links to other information available for drilling down, this part was not
clear to me. How does a VM developer expose a new stat that he/she has added.

https://codereview.chromium.org/12221022/diff/13003/runtime/bin/vmstats.cc#ne...
runtime/bin/vmstats.cc:186: } else {
I assume this else section is present for serving some static pages that are
present in the root directory. Could you add a comment.

https://codereview.chromium.org/12221022/diff/13003/runtime/bin/vmstats.h
File runtime/bin/vmstats.h (right):

https://codereview.chromium.org/12221022/diff/13003/runtime/bin/vmstats.h#new...
runtime/bin/vmstats.h:1: // Copyright (c) 2012, the Dart project authors. 
Please see the AUTHORS file
2013

https://codereview.chromium.org/12221022/diff/13003/runtime/include/dart_debu...
File runtime/include/dart_debugger_api.h (right):

https://codereview.chromium.org/12221022/diff/13003/runtime/include/dart_debu...
runtime/include/dart_debugger_api.h:628: const char* status_type,
Dart_VmStatusCallback callback);
I guess I am not clear about why external entities need to register their status
plugins with the VM. Can't this be filtered out in the webserver loop that you
have.

The VM needs to deal only with the stuff that it can provide.

https://codereview.chromium.org/12221022/diff/13003/runtime/vm/dart.cc
File runtime/vm/dart.cc (right):

https://codereview.chromium.org/12221022/diff/13003/runtime/vm/dart.cc#newcod...
runtime/vm/dart.cc:102: VmStatusService::InitOnce();
I guess this InitOnce is needed to for the plug-in registration and the map that
you maintain for supporting that. We need to discuss about the need for external
entities registering their plugins with the VM.

https://codereview.chromium.org/12221022/diff/13003/runtime/vm/debugger.h
File runtime/vm/debugger.h (right):

https://codereview.chromium.org/12221022/diff/13003/runtime/vm/debugger.h#new...
runtime/vm/debugger.h:9: #include <map>
We have generally tried to avoid usage of map inside the VM sources.

https://codereview.chromium.org/12221022/diff/13003/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/12221022/diff/13003/runtime/vm/isolate.cc#new...
runtime/vm/isolate.cc:664: stream << "  }" << std::endl << "}";
Can this whole sequence be constructed using OS::SNPrint instead of this C++
sstream stuff. We have tried to avoid this sstream stuff in the VM also (trying
to minimize porting surprises).

[siva, 2013-02-11 19:14:55]

https://codereview.chromium.org/12221022/diff/13003/runtime/bin/vmstats.cc
File runtime/bin/vmstats.cc (right):

https://codereview.chromium.org/12221022/diff/13003/runtime/bin/vmstats.cc#ne...
runtime/bin/vmstats.cc:74: instance_->isolate_table_[isolate_data] = isolate;
This method needs to be thread safe right as we can potentially have multiple
isolates calling into it.

https://codereview.chromium.org/12221022/diff/13003/runtime/bin/vmstats.cc#ne...
runtime/bin/vmstats.cc:79: void VmStats::RemoveIsolate(IsolateData*
isolate_data) {
Ditto comment about this method being thread safe.

https://codereview.chromium.org/12221022/diff/13003/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/12221022/diff/13003/runtime/vm/debugger.cc#ne...
runtime/vm/debugger.cc:1759: const char* status_type, Dart_VmStatusCallback
callback) {
Do you expect multiple threads to register plugins? If yes then this needs to be
thread safe.

[Tom Ball, 2013-02-14 00:15:43]

Rewrote VM support to not use STL. Aadded flag for web server root directory,
which is only used when developing the tool's front-end.

[Tom Ball, 2013-02-14 23:45:16]

https://codereview.chromium.org/12221022/diff/13003/runtime/bin/bin.gypi
File runtime/bin/bin.gypi (right):

https://codereview.chromium.org/12221022/diff/13003/runtime/bin/bin.gypi#newc...
runtime/bin/bin.gypi:554: },
On 2013/02/09 01:00:57, siva wrote:
> Does this have to be a separate static library, can we list vmstats.cc and
> vmstats.h along with main.cc in the targets
> 'dart' and 'dart_no_snapshot'?
> 
> Do you see other executables wanting to link with this library?

I added to them to the targets that include main.cc as you suggested.

https://codereview.chromium.org/12221022/diff/13003/runtime/bin/vmstats.cc
File runtime/bin/vmstats.cc (right):

https://codereview.chromium.org/12221022/diff/13003/runtime/bin/vmstats.cc#ne...
runtime/bin/vmstats.cc:1: // Copyright (c) 2012, the Dart project authors. 
Please see the AUTHORS file
On 2013/02/09 01:00:57, siva wrote:
> 2013

Done.

https://codereview.chromium.org/12221022/diff/13003/runtime/bin/vmstats.cc#ne...
runtime/bin/vmstats.cc:32: char** env = Platform::Environment(&num_vars);
On 2013/02/09 01:00:57, siva wrote:
> Are you going to switch this to be a command line value so that people can
> specify a root directory location?
> If this is not specified then we pick the SDK-specific path to the root
> directory.

Done. I added a flag that optionally sets root directory, which only needs to be
used when working on the status UI code.

https://codereview.chromium.org/12221022/diff/13003/runtime/bin/vmstats.cc#ne...
runtime/bin/vmstats.cc:48: ServerSocket::CreateBindListen("127.0.0.1", port,
BACKLOG);
On 2013/02/09 01:00:57, siva wrote:
> fine for now but we may want to make 127.0.0.1 a parameter too. The command
line
> option could be specified as
> --vmstats_port=8080 or
> --vmstats_port=hostname:8080

Sounds great -- any security issues with making this accessible from remote
sites I need to watch out for?

https://codereview.chromium.org/12221022/diff/13003/runtime/bin/vmstats.cc#ne...
runtime/bin/vmstats.cc:72: Dart_Isolate isolate) {
On 2013/02/09 01:00:57, siva wrote:
> .... indentation.

Done.

https://codereview.chromium.org/12221022/diff/13003/runtime/bin/vmstats.cc#ne...
runtime/bin/vmstats.cc:74: instance_->isolate_table_[isolate_data] = isolate;
On 2013/02/11 19:14:55, siva wrote:
> This method needs to be thread safe right as we can potentially have multiple
> isolates calling into it.

Done.

https://codereview.chromium.org/12221022/diff/13003/runtime/bin/vmstats.cc#ne...
runtime/bin/vmstats.cc:79: void VmStats::RemoveIsolate(IsolateData*
isolate_data) {
On 2013/02/11 19:14:55, siva wrote:
> Ditto comment about this method being thread safe.

Done.

https://codereview.chromium.org/12221022/diff/13003/runtime/bin/vmstats.cc#ne...
runtime/bin/vmstats.cc:174: instance_->MemoryUsed(&stream);
On 2013/02/09 01:00:57, siva wrote:
> Instead of MemoryUsed you could probably call this
> IsolateStatus or ToplevelIsolateStatus so that it can return
> all the top level information available for the isolate which could
potentially
> include:
> - script/library/class/method of the function used for spawning the isolate
> - memory usage
> - links to other information available for drilling down, this part was not
> clear to me. How does a VM developer expose a new stat that he/she has added.

Done.

https://codereview.chromium.org/12221022/diff/13003/runtime/bin/vmstats.cc#ne...
runtime/bin/vmstats.cc:186: } else {
On 2013/02/09 01:00:57, siva wrote:
> I assume this else section is present for serving some static pages that are
> present in the root directory. Could you add a comment.

Done.

https://codereview.chromium.org/12221022/diff/13003/runtime/bin/vmstats.h
File runtime/bin/vmstats.h (right):

https://codereview.chromium.org/12221022/diff/13003/runtime/bin/vmstats.h#new...
runtime/bin/vmstats.h:1: // Copyright (c) 2012, the Dart project authors. 
Please see the AUTHORS file
On 2013/02/09 01:00:57, siva wrote:
> 2013

Done.

https://codereview.chromium.org/12221022/diff/13003/runtime/include/dart_debu...
File runtime/include/dart_debugger_api.h (right):

https://codereview.chromium.org/12221022/diff/13003/runtime/include/dart_debu...
runtime/include/dart_debugger_api.h:628: const char* status_type,
Dart_VmStatusCallback callback);
On 2013/02/09 01:00:57, siva wrote:
> I guess I am not clear about why external entities need to register their
status
> plugins with the VM. Can't this be filtered out in the webserver loop that you
> have.
> 
> The VM needs to deal only with the stuff that it can provide.

It's so other libraries like io can add status. Is there a better place for
these?

https://codereview.chromium.org/12221022/diff/13003/runtime/vm/dart.cc
File runtime/vm/dart.cc (right):

https://codereview.chromium.org/12221022/diff/13003/runtime/vm/dart.cc#newcod...
runtime/vm/dart.cc:102: VmStatusService::InitOnce();
On 2013/02/09 01:00:57, siva wrote:
> I guess this InitOnce is needed to for the plug-in registration and the map
that
> you maintain for supporting that. We need to discuss about the need for
external
> entities registering their plugins with the VM.

That's right -- I can lazily do initialization on first request, if that's
preferable.

https://codereview.chromium.org/12221022/diff/13003/runtime/vm/debugger.h
File runtime/vm/debugger.h (right):

https://codereview.chromium.org/12221022/diff/13003/runtime/vm/debugger.h#new...
runtime/vm/debugger.h:9: #include <map>
On 2013/02/09 01:00:57, siva wrote:
> We have generally tried to avoid usage of map inside the VM sources.

STL references removed.

https://codereview.chromium.org/12221022/diff/13003/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/12221022/diff/13003/runtime/vm/isolate.cc#new...
runtime/vm/isolate.cc:664: stream << "  }" << std::endl << "}";
On 2013/02/09 01:00:57, siva wrote:
> Can this whole sequence be constructed using OS::SNPrint instead of this C++
> sstream stuff. We have tried to avoid this sstream stuff in the VM also
(trying
> to minimize porting surprises).

Done.

[siva, 2013-02-15 05:59:10]

The overall structure looks good.

I had a few comments with regards to implementation details.

https://codereview.chromium.org/12221022/diff/24016/runtime/bin/main.cc
File runtime/bin/main.cc (right):

https://codereview.chromium.org/12221022/diff/24016/runtime/bin/main.cc#newco...
runtime/bin/main.cc:588: "\n"
Should we also list help for the '--stats-root' option here.

https://codereview.chromium.org/12221022/diff/24016/runtime/bin/main.cc#newco...
runtime/bin/main.cc:753: if (vmstats_port > 0) {
What about the case when vmstats_port is -1 ?

https://codereview.chromium.org/12221022/diff/24016/runtime/bin/vmstats.h
File runtime/bin/vmstats.h (right):

https://codereview.chromium.org/12221022/diff/24016/runtime/bin/vmstats.h#new...
runtime/bin/vmstats.h:12: * function using Dart_RegisterStatusPlugin. When
Dart_GetVMStatus is
The comment seems a bit out dated (Dart_GetVMStatus is not involved anymore for
this right).

https://codereview.chromium.org/12221022/diff/24016/runtime/bin/vmstats_impl.cc
File runtime/bin/vmstats_impl.cc (right):

https://codereview.chromium.org/12221022/diff/24016/runtime/bin/vmstats_impl....
runtime/bin/vmstats_impl.cc:24: if (instance_ != NULL) {
Where is instance_ set? I don't see it being set to a Non NULL value anywhere in
the code.

https://codereview.chromium.org/12221022/diff/24016/runtime/bin/vmstats_impl....
runtime/bin/vmstats_impl.cc:28: VmStats* vmstats = new VmStats;
probably you meant instance_ = new VmStats(); here.

https://codereview.chromium.org/12221022/diff/24016/runtime/bin/vmstats_impl....
runtime/bin/vmstats_impl.cc:42: }
If port is -1 we probably need code here to get the socket address of the socket
that was created and print out the port number so that people know which port to
connect to.

https://codereview.chromium.org/12221022/diff/24016/runtime/bin/vmstats_impl....
runtime/bin/vmstats_impl.cc:55: delete instance_;
The WebServer thread is still running and it accesses instance_, we could end up
with access after delete if we have an inflight message just as we are shutting
down.

You should probably close the socket here and rendezvous with the WebServer
thread to ensure it has exited before deleting instance_.

https://codereview.chromium.org/12221022/diff/24016/runtime/bin/vmstats_impl....
runtime/bin/vmstats_impl.cc:90: std::string path(url);
might be more efficient to just do a
const char* suffix = strrchr(url, '.');
if (suffix != NULL) {
  if (!strcmp(suffix, ".html")) {
    ...
  }
  ...
  ...
}
return "text/plain";

https://codereview.chromium.org/12221022/diff/24016/runtime/bin/vmstats_impl....
runtime/bin/vmstats_impl.cc:134: int len = read(socket, buffer, BUFSIZE);
We may need to handle partial reads here but let us figure that one out later.

https://codereview.chromium.org/12221022/diff/24016/runtime/bin/vmstats_impl....
runtime/bin/vmstats_impl.cc:151: Socket::Write(socket, response,
strlen(response));
Does Socket::Write assert that the entire buffer passed in was written out?

https://codereview.chromium.org/12221022/diff/24016/runtime/bin/vmstats_impl....
runtime/bin/vmstats_impl.cc:152: close(socket);
Socket::Close(socket); ?

https://codereview.chromium.org/12221022/diff/24016/runtime/bin/vmstats_impl....
runtime/bin/vmstats_impl.cc:254: void VmStatusService::InitOnce() {
ASSERT(VmStatusService::Instance_ == NULL);

https://codereview.chromium.org/12221022/diff/24016/runtime/bin/vmstats_impl....
runtime/bin/vmstats_impl.cc:266: int
VmStatusService::RegisterPlugin(Dart_VmStatusCallback callback) {
We should probably protect this call also under a lock as we cannot assume that
this will be called always from a single thread.

https://codereview.chromium.org/12221022/diff/24016/runtime/vm/debugger_api_i...
File runtime/vm/debugger_api_impl.cc (right):

https://codereview.chromium.org/12221022/diff/24016/runtime/vm/debugger_api_i...
runtime/vm/debugger_api_impl.cc:756: if (strncmp(request, "/isolate/", 9) == 0)
{
You should probably add a TODO here that we need to figure out which isolate
this command needs to be sent to after parsing the string and then send an OOB
message to the isolate.

[Tom Ball, 2013-02-16 00:57:19]

https://codereview.chromium.org/12221022/diff/24016/runtime/bin/main.cc
File runtime/bin/main.cc (right):

https://codereview.chromium.org/12221022/diff/24016/runtime/bin/main.cc#newco...
runtime/bin/main.cc:588: "\n"
On 2013/02/15 05:59:10, siva wrote:
> Should we also list help for the '--stats-root' option here.

Done.

https://codereview.chromium.org/12221022/diff/24016/runtime/bin/main.cc#newco...
runtime/bin/main.cc:753: if (vmstats_port > 0) {
On 2013/02/15 05:59:10, siva wrote:
> What about the case when vmstats_port is -1 ?

Good catch. It turns out 0 is the official dynamic port, not -1 (Linux will take
either, but it's probably not cross-platform). I changed it so -1 indicates "no
port specified", and 0 means dynamic port, and updated this test.

https://codereview.chromium.org/12221022/diff/24016/runtime/bin/vmstats.h
File runtime/bin/vmstats.h (right):

https://codereview.chromium.org/12221022/diff/24016/runtime/bin/vmstats.h#new...
runtime/bin/vmstats.h:12: * function using Dart_RegisterStatusPlugin. When
Dart_GetVMStatus is
On 2013/02/15 05:59:10, siva wrote:
> The comment seems a bit out dated (Dart_GetVMStatus is not involved anymore
for
> this right).

Updated.

https://codereview.chromium.org/12221022/diff/24016/runtime/bin/vmstats_impl.cc
File runtime/bin/vmstats_impl.cc (right):

https://codereview.chromium.org/12221022/diff/24016/runtime/bin/vmstats_impl....
runtime/bin/vmstats_impl.cc:24: if (instance_ != NULL) {
On 2013/02/15 05:59:10, siva wrote:
> Where is instance_ set? I don't see it being set to a Non NULL value anywhere
in
> the code.

It was the last statement in Start(), but I changed it to just use instance_.

https://codereview.chromium.org/12221022/diff/24016/runtime/bin/vmstats_impl....
runtime/bin/vmstats_impl.cc:28: VmStats* vmstats = new VmStats;
On 2013/02/15 05:59:10, siva wrote:
> probably you meant instance_ = new VmStats(); here.

Done.

https://codereview.chromium.org/12221022/diff/24016/runtime/bin/vmstats_impl....
runtime/bin/vmstats_impl.cc:42: }
On 2013/02/15 05:59:10, siva wrote:
> If port is -1 we probably need code here to get the socket address of the
socket
> that was created and print out the port number so that people know which port
to
> connect to.

Done.

https://codereview.chromium.org/12221022/diff/24016/runtime/bin/vmstats_impl....
runtime/bin/vmstats_impl.cc:55: delete instance_;
On 2013/02/15 05:59:10, siva wrote:
> The WebServer thread is still running and it accesses instance_, we could end
up
> with access after delete if we have an inflight message just as we are
shutting
> down.
> 
> You should probably close the socket here and rendezvous with the WebServer
> thread to ensure it has exited before deleting instance_.

Reworked stop so it only sets a running_ instance flag to false, which causes
the web server to drop out of its request processing loop. I added
VmStats::Shutdown(), which has this code, and is called by the web server after
its loop finishes. This allows the server to finish its current request,
simplifying shutdown.

https://codereview.chromium.org/12221022/diff/24016/runtime/bin/vmstats_impl....
runtime/bin/vmstats_impl.cc:90: std::string path(url);
On 2013/02/15 05:59:10, siva wrote:
> might be more efficient to just do a
> const char* suffix = strrchr(url, '.');
> if (suffix != NULL) {
>   if (!strcmp(suffix, ".html")) {
>     ...
>   }
>   ...
>   ...
> }
> return "text/plain";

Done.

https://codereview.chromium.org/12221022/diff/24016/runtime/bin/vmstats_impl....
runtime/bin/vmstats_impl.cc:134: int len = read(socket, buffer, BUFSIZE);
On 2013/02/15 05:59:10, siva wrote:
> We may need to handle partial reads here but let us figure that one out later.

This buffer works as long as the server only supports GET, since the HTTP
request header shouldn't be bigger than 8K (it would take a really long URL!).
You're right, though, since POST is useful to hide request details. I added a
TODO.

https://codereview.chromium.org/12221022/diff/24016/runtime/bin/vmstats_impl....
runtime/bin/vmstats_impl.cc:151: Socket::Write(socket, response,
strlen(response));
On 2013/02/15 05:59:10, siva wrote:
> Does Socket::Write assert that the entire buffer passed in was written out?

It does because I previously set the socket to blocking mode. Prior to my doing
that, the buffers were only partially written.

https://codereview.chromium.org/12221022/diff/24016/runtime/bin/vmstats_impl....
runtime/bin/vmstats_impl.cc:152: close(socket);
On 2013/02/15 05:59:10, siva wrote:
> Socket::Close(socket); ?

Done.

https://codereview.chromium.org/12221022/diff/24016/runtime/bin/vmstats_impl....
runtime/bin/vmstats_impl.cc:254: void VmStatusService::InitOnce() {
On 2013/02/15 05:59:10, siva wrote:
> ASSERT(VmStatusService::Instance_ == NULL);

Done.

https://codereview.chromium.org/12221022/diff/24016/runtime/bin/vmstats_impl....
runtime/bin/vmstats_impl.cc:266: int
VmStatusService::RegisterPlugin(Dart_VmStatusCallback callback) {
On 2013/02/15 05:59:10, siva wrote:
> We should probably protect this call also under a lock as we cannot assume
that
> this will be called always from a single thread.

Done.

https://codereview.chromium.org/12221022/diff/24016/runtime/vm/debugger_api_i...
File runtime/vm/debugger_api_impl.cc (right):

https://codereview.chromium.org/12221022/diff/24016/runtime/vm/debugger_api_i...
runtime/vm/debugger_api_impl.cc:756: if (strncmp(request, "/isolate/", 9) == 0)
{
On 2013/02/15 05:59:10, siva wrote:
> You should probably add a TODO here that we need to figure out which isolate
> this command needs to be sent to after parsing the string and then send an OOB
> message to the isolate.

Done.

[siva, 2013-02-25 06:47:41]

The shutdown code still appears to have race conditions otherwise it LGTM.

https://codereview.chromium.org/12221022/diff/34001/runtime/bin/vmstats_impl.cc
File runtime/bin/vmstats_impl.cc (right):

https://codereview.chromium.org/12221022/diff/34001/runtime/bin/vmstats_impl....
runtime/bin/vmstats_impl.cc:66: int errno = Thread::Start(WebServer, address);
Where is the server socket represented by address closed?

https://codereview.chromium.org/12221022/diff/34001/runtime/bin/vmstats_impl....
runtime/bin/vmstats_impl.cc:74: void VmStats::Stop() {
Need MonitorLocker ml&instance_monitor_); here as you
are modifying instance_->running_;

https://codereview.chromium.org/12221022/diff/34001/runtime/bin/vmstats_impl....
runtime/bin/vmstats_impl.cc:136: MonitorLocker ml(&instance_monitor_);
This should be outside the while? otherwise you would be
accessing instance_running_ without a lock?

Actually holding the lock like this could be a bad idea as you have several
blocking calls in this code.

Not sure if this scheme of checking for instance_->running will bring this
thread down as you require that a connection needs to be made so that the accept
will wake up and then go back in the loop and check for instance_->running.

[Tom Ball, 2013-02-26 21:50:48]

https://codereview.chromium.org/12221022/diff/34001/runtime/bin/vmstats_impl.cc
File runtime/bin/vmstats_impl.cc (right):

https://codereview.chromium.org/12221022/diff/34001/runtime/bin/vmstats_impl....
runtime/bin/vmstats_impl.cc:66: int errno = Thread::Start(WebServer, address);
On 2013/02/25 06:47:41, siva wrote:
> Where is the server socket represented by address closed?

Saved bind_address to instance, closed in VmStats::Shutdown().

https://codereview.chromium.org/12221022/diff/34001/runtime/bin/vmstats_impl....
runtime/bin/vmstats_impl.cc:74: void VmStats::Stop() {
On 2013/02/25 06:47:41, siva wrote:
> Need MonitorLocker ml&instance_monitor_); here as you
> are modifying instance_->running_;

Done.

https://codereview.chromium.org/12221022/diff/34001/runtime/bin/vmstats_impl....
runtime/bin/vmstats_impl.cc:136: MonitorLocker ml(&instance_monitor_);
On 2013/02/25 06:47:41, siva wrote:
As discussed, lock is just held when accessing instance_. instance_->running_
test above is now "while (true)", with test moved to right before VM calls.

[Tom Ball, 2013-02-26 22:34:17]

Committed patchset #13 manually as r19091 (presubmit successful).