Add current rss and embedder name to Observatory

Add current rss and embedder name to Observatory

Moved the responsibility to provide MaxRSS and CurrentRSS from the VM to
the embedder.

The embedder can opt-it by setting a Dart_GetEmbedderInformation
callback using the public Dart_SetEmbedderInformationCallback API.

The implementation of the Dart_GetEmbedderInformation should mandatory
fill the version field and the ones it is able to fill.
The name field must reference a constant C style string, it will not be
freed by the VM code.

R=zra@google.com

Committed: https://github.com/dart-lang/sdk/commit/ce736d3ef009bf334a92d5b7d354fc0b04805b4f

[cbernaschina, 2017-08-09 22:06:34]

Description was changed from

==========
Add current rss and embedder name to Observatory

Moved the responsibility to provide MaxRSS and CurrentRSS from the VM to
the embedder.

The embedder can opt-it by setting a Dart_GetEmbedderInformation
callback using the public Dart_SetGetEmbedderInformationCallback API.

The implementation of any Dart_GetEmbedderInformation must follow the
same rules used in many C standard libraries like gmtime from the ctime
library.
==========

to

==========
Add current rss and embedder name to Observatory

Moved the responsibility to provide MaxRSS and CurrentRSS from the VM to
the embedder.

The embedder can opt-it by setting a Dart_GetEmbedderInformation
callback using the public Dart_SetGetEmbedderInformationCallback API.

The implementation of any Dart_GetEmbedderInformation must follow the
same rules used in many C standard libraries like gmtime from the ctime
library.
==========

[cbernaschina, 2017-08-09 22:06:34]

cbernaschina@google.com changed reviewers:
+ asiva@google.com, rmacnak@google.com, zra@google.com

[zra, 2017-08-09 22:58:27]

More comments tomorrow...

https://codereview.chromium.org/2996803002/diff/20001/runtime/include/dart_to...
File runtime/include/dart_tools_api.h (right):

https://codereview.chromium.org/2996803002/diff/20001/runtime/include/dart_to...
runtime/include/dart_tools_api.h:804: DART_EXPORT void
Dart_SetGetEmbedderInformationCallback(
Consider a rename to avoid "SetGet", which doesn't read well to me.

[cbernaschina, 2017-08-10 00:29:17]

https://codereview.chromium.org/2996803002/diff/20001/runtime/include/dart_to...
File runtime/include/dart_tools_api.h (right):

https://codereview.chromium.org/2996803002/diff/20001/runtime/include/dart_to...
runtime/include/dart_tools_api.h:804: DART_EXPORT void
Dart_SetGetEmbedderInformationCallback(
On 2017/08/09 22:58:27, zra wrote:
> Consider a rename to avoid "SetGet", which doesn't read well to me.

I called it SetGetEmbedderInformationCallback... following the example of
SetGetServiceAssetsCallback in service.h

I don't like RegisterGetEmbedderInformationCallback because register gives more
an idea of "more than one".

What about SetEmbedderInformationGetter ?

[zra, 2017-08-10 22:19:51]

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

https://codereview.chromium.org/2996803002/diff/20001/runtime/bin/main.cc#new...
runtime/bin/main.cc:1360: static Dart_EmbedderInformation information = {
The VM could pass in a pointer to a Dart_EmbedderInformation that the embedder
then fills in. That way you don't have to make a global, and worry about
threading issues.

https://codereview.chromium.org/2996803002/diff/20001/runtime/bin/process_and...
File runtime/bin/process_android.cc (right):

https://codereview.chromium.org/2996803002/diff/20001/runtime/bin/process_and...
runtime/bin/process_android.cc:872: &current_rss_pages);
It looks like this will fit on the line above.

https://codereview.chromium.org/2996803002/diff/20001/runtime/bin/process_and...
runtime/bin/process_android.cc:873: fclose(statm);
This will blow away errno from fscanf when there is an error.

https://codereview.chromium.org/2996803002/diff/20001/runtime/bin/process_lin...
File runtime/bin/process_linux.cc (right):

https://codereview.chromium.org/2996803002/diff/20001/runtime/bin/process_lin...
runtime/bin/process_linux.cc:873: &current_rss_pages);
It looks like this will fit on the line above.

https://codereview.chromium.org/2996803002/diff/20001/runtime/bin/process_lin...
runtime/bin/process_linux.cc:874: fclose(statm);
ditto

https://codereview.chromium.org/2996803002/diff/20001/runtime/include/dart_to...
File runtime/include/dart_tools_api.h (right):

https://codereview.chromium.org/2996803002/diff/20001/runtime/include/dart_to...
runtime/include/dart_tools_api.h:775: typedef struct {
This should probably have a version field. See for example Dart_InitializeParams
in dart_api.h.

https://codereview.chromium.org/2996803002/diff/20001/runtime/include/dart_to...
runtime/include/dart_tools_api.h:790: typedef const Dart_EmbedderInformation*
(*Dart_GetEmbedderInformation)();
Maybe Dart_GetEmbedderInformation -> Dart_EmbedderInformationCallback

https://codereview.chromium.org/2996803002/diff/20001/runtime/include/dart_to...
runtime/include/dart_tools_api.h:804: DART_EXPORT void
Dart_SetGetEmbedderInformationCallback(
On 2017/08/10 00:29:17, cbernaschina wrote:
> On 2017/08/09 22:58:27, zra wrote:
> > Consider a rename to avoid "SetGet", which doesn't read well to me.
> 
> I called it SetGetEmbedderInformationCallback... following the example of
> SetGetServiceAssetsCallback in service.h
> 
> I don't like RegisterGetEmbedderInformationCallback because register gives
more
> an idea of "more than one".
> 
> What about SetEmbedderInformationGetter ?

Maybe just SetEmbedderInformationCallback?

[cbernaschina, 2017-08-10 22:40:28]

A couple of comments, will address the rest in the morning.

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

https://codereview.chromium.org/2996803002/diff/20001/runtime/bin/main.cc#new...
runtime/bin/main.cc:1360: static Dart_EmbedderInformation information = {
On 2017/08/10 22:19:50, zra wrote:
> The VM could pass in a pointer to a Dart_EmbedderInformation that the embedder
> then fills in. That way you don't have to make a global, and worry about
> threading issues.

That was the first idea, the main problem is related with future expansion of
the structure. It can be used by flutter to send extra information (like the
current skia memory). If we keep the ownership of the structure in the VM if we
add pointers to dynamic arrays it will be responsibility of the VM to free them.

https://codereview.chromium.org/2996803002/diff/20001/runtime/bin/process_and...
File runtime/bin/process_android.cc (right):

https://codereview.chromium.org/2996803002/diff/20001/runtime/bin/process_and...
runtime/bin/process_android.cc:873: fclose(statm);
On 2017/08/10 22:19:50, zra wrote:
> This will blow away errno from fscanf when there is an error.

If matches is 1 it means that the value was successfully red. Should we check
even  ferror? Or do we need to preserve errno for other reasons?

[zra, 2017-08-10 22:49:47]

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

https://codereview.chromium.org/2996803002/diff/20001/runtime/bin/main.cc#new...
runtime/bin/main.cc:1360: static Dart_EmbedderInformation information = {
On 2017/08/10 22:40:28, cbernaschina wrote:
> On 2017/08/10 22:19:50, zra wrote:
> > The VM could pass in a pointer to a Dart_EmbedderInformation that the
embedder
> > then fills in. That way you don't have to make a global, and worry about
> > threading issues.
> 
> That was the first idea, the main problem is related with future expansion of
> the structure. It can be used by flutter to send extra information (like the
> current skia memory). If we keep the ownership of the structure in the VM if
we
> add pointers to dynamic arrays it will be responsibility of the VM to free
them.

I'm not sure this simple call is the right API for anything more complicated
than sending a flat structure of const char*s and numbers. I'd expect that
something as specific as Flutter skia memory should rather be implemented as a
service protocol extension.

https://codereview.chromium.org/2996803002/diff/20001/runtime/bin/process_and...
File runtime/bin/process_android.cc (right):

https://codereview.chromium.org/2996803002/diff/20001/runtime/bin/process_and...
runtime/bin/process_android.cc:873: fclose(statm);
On 2017/08/10 22:40:28, cbernaschina wrote:
> On 2017/08/10 22:19:50, zra wrote:
> > This will blow away errno from fscanf when there is an error.
> 
> If matches is 1 it means that the value was successfully red. Should we check
> even  ferror? Or do we need to preserve errno for other reasons?

If this function returns -1, the caller will make a new Dart OSError object
whose fields are populated by looking at errno.

[cbernaschina, 2017-08-12 00:29:36]

Description was changed from

==========
Add current rss and embedder name to Observatory

Moved the responsibility to provide MaxRSS and CurrentRSS from the VM to
the embedder.

The embedder can opt-it by setting a Dart_GetEmbedderInformation
callback using the public Dart_SetGetEmbedderInformationCallback API.

The implementation of any Dart_GetEmbedderInformation must follow the
same rules used in many C standard libraries like gmtime from the ctime
library.
==========

to

==========
Add current rss and embedder name to Observatory

Moved the responsibility to provide MaxRSS and CurrentRSS from the VM to
the embedder.

The embedder can opt-it by setting a Dart_GetEmbedderInformation
callback using the public Dart_SetEmbedderInformationCallback API.

The implementation of the Dart_GetEmbedderInformation should mandatory
fill the version field and the ones it is able to fill.
The name field must reference a constant C style string, it will not be
freed by the VM code.
==========

[cbernaschina, 2017-08-12 00:29:42]

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

https://codereview.chromium.org/2996803002/diff/20001/runtime/bin/main.cc#new...
runtime/bin/main.cc:1360: static Dart_EmbedderInformation information = {
On 2017/08/10 22:49:46, zra wrote:
> On 2017/08/10 22:40:28, cbernaschina wrote:
> > On 2017/08/10 22:19:50, zra wrote:
> > > The VM could pass in a pointer to a Dart_EmbedderInformation that the
> embedder
> > > then fills in. That way you don't have to make a global, and worry about
> > > threading issues.
> > 
> > That was the first idea, the main problem is related with future expansion
of
> > the structure. It can be used by flutter to send extra information (like the
> > current skia memory). If we keep the ownership of the structure in the VM if
> we
> > add pointers to dynamic arrays it will be responsibility of the VM to free
> them.
> 
> I'm not sure this simple call is the right API for anything more complicated
> than sending a flat structure of const char*s and numbers. I'd expect that
> something as specific as Flutter skia memory should rather be implemented as a
> service protocol extension.

Done.

https://codereview.chromium.org/2996803002/diff/20001/runtime/bin/process_and...
File runtime/bin/process_android.cc (right):

https://codereview.chromium.org/2996803002/diff/20001/runtime/bin/process_and...
runtime/bin/process_android.cc:872: &current_rss_pages);
On 2017/08/10 22:19:50, zra wrote:
> It looks like this will fit on the line above.

Done.

https://codereview.chromium.org/2996803002/diff/20001/runtime/bin/process_and...
runtime/bin/process_android.cc:873: fclose(statm);
On 2017/08/10 22:49:47, zra wrote:
> On 2017/08/10 22:40:28, cbernaschina wrote:
> > On 2017/08/10 22:19:50, zra wrote:
> > > This will blow away errno from fscanf when there is an error.
> > 
> > If matches is 1 it means that the value was successfully red. Should we
check
> > even  ferror? Or do we need to preserve errno for other reasons?
> 
> If this function returns -1, the caller will make a new Dart OSError object
> whose fields are populated by looking at errno.

Done.

https://codereview.chromium.org/2996803002/diff/20001/runtime/bin/process_lin...
File runtime/bin/process_linux.cc (right):

https://codereview.chromium.org/2996803002/diff/20001/runtime/bin/process_lin...
runtime/bin/process_linux.cc:873: &current_rss_pages);
On 2017/08/10 22:19:50, zra wrote:
> It looks like this will fit on the line above.

Done.

https://codereview.chromium.org/2996803002/diff/20001/runtime/bin/process_lin...
runtime/bin/process_linux.cc:874: fclose(statm);
On 2017/08/10 22:19:50, zra wrote:
> ditto

Done.

https://codereview.chromium.org/2996803002/diff/20001/runtime/include/dart_to...
File runtime/include/dart_tools_api.h (right):

https://codereview.chromium.org/2996803002/diff/20001/runtime/include/dart_to...
runtime/include/dart_tools_api.h:775: typedef struct {
On 2017/08/10 22:19:50, zra wrote:
> This should probably have a version field. See for example
Dart_InitializeParams
> in dart_api.h.

Done.

https://codereview.chromium.org/2996803002/diff/20001/runtime/include/dart_to...
runtime/include/dart_tools_api.h:790: typedef const Dart_EmbedderInformation*
(*Dart_GetEmbedderInformation)();
On 2017/08/10 22:19:50, zra wrote:
> Maybe Dart_GetEmbedderInformation -> Dart_EmbedderInformationCallback

Done.

https://codereview.chromium.org/2996803002/diff/20001/runtime/include/dart_to...
runtime/include/dart_tools_api.h:804: DART_EXPORT void
Dart_SetGetEmbedderInformationCallback(
On 2017/08/10 22:19:50, zra wrote:
> On 2017/08/10 00:29:17, cbernaschina wrote:
> > On 2017/08/09 22:58:27, zra wrote:
> > > Consider a rename to avoid "SetGet", which doesn't read well to me.
> > 
> > I called it SetGetEmbedderInformationCallback... following the example of
> > SetGetServiceAssetsCallback in service.h
> > 
> > I don't like RegisterGetEmbedderInformationCallback because register gives
> more
> > an idea of "more than one".
> > 
> > What about SetEmbedderInformationGetter ?
> 
> Maybe just SetEmbedderInformationCallback?

Done.

[zra, 2017-08-14 18:26:32]

lgtm w/ cleanup suggestion

https://codereview.chromium.org/2996803002/diff/40001/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/2996803002/diff/40001/runtime/vm/service.cc#n...
runtime/vm/service.cc:3875: if (embedder_information_callback_ != NULL) {
Possible cleanup: Pull this out into e.g.
Service::PrintJSONForEmbedderInformation()

[cbernaschina, 2017-08-14 20:07:41]

https://codereview.chromium.org/2996803002/diff/40001/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/2996803002/diff/40001/runtime/vm/service.cc#n...
runtime/vm/service.cc:3875: if (embedder_information_callback_ != NULL) {
On 2017/08/14 18:26:32, zra wrote:
> Possible cleanup: Pull this out into e.g.
> Service::PrintJSONForEmbedderInformation()

Done.

[cbernaschina, 2017-08-14 22:22:53]

Description was changed from

==========
Add current rss and embedder name to Observatory

Moved the responsibility to provide MaxRSS and CurrentRSS from the VM to
the embedder.

The embedder can opt-it by setting a Dart_GetEmbedderInformation
callback using the public Dart_SetEmbedderInformationCallback API.

The implementation of the Dart_GetEmbedderInformation should mandatory
fill the version field and the ones it is able to fill.
The name field must reference a constant C style string, it will not be
freed by the VM code.
==========

to

==========
Add current rss and embedder name to Observatory

Moved the responsibility to provide MaxRSS and CurrentRSS from the VM to
the embedder.

The embedder can opt-it by setting a Dart_GetEmbedderInformation
callback using the public Dart_SetEmbedderInformationCallback API.

The implementation of the Dart_GetEmbedderInformation should mandatory
fill the version field and the ones it is able to fill.
The name field must reference a constant C style string, it will not be
freed by the VM code.

R=zra@google.com

Committed:
https://github.com/dart-lang/sdk/commit/ce736d3ef009bf334a92d5b7d354fc0b04805b4f
==========

[cbernaschina, 2017-08-14 22:22:54]

Committed patchset #5 (id:80001) manually as
ce736d3ef009bf334a92d5b7d354fc0b04805b4f.