Simplify various name flavors in VM.

Simplify various name flavors in VM.

R=asiva@google.com, johnmccutchan@google.com

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

[regis, 2016-02-22 23:43:15]

regis@google.com changed reviewers:
+ johnmccutchan@google.com

[regis, 2016-02-23 16:45:47]

I am getting a couple of errors with dart_noopt. I guess some names are not in
sync. So you can wait until I resolve them and update this cl before reviewing
it.

Thanks,
Regis

[regis, 2016-02-23 17:02:27]

On 2016/02/23 16:45:47, regis wrote:
> I am getting a couple of errors with dart_noopt. I guess some names are not in
> sync. So you can wait until I resolve them and update this cl before reviewing
> it.
> 
> Thanks,
> Regis

Never mind. These failures have nothing to do with this cl. They even currently
occur on the buildbots, although the buildbots remain green.

[Cutch, 2016-02-23 17:42:53]

https://codereview.chromium.org/1723733002/diff/1/runtime/observatory/tests/s...
File runtime/observatory/tests/service/get_object_rpc_test.dart (right):

https://codereview.chromium.org/1723733002/diff/1/runtime/observatory/tests/s...
runtime/observatory/tests/service/get_object_rpc_test.dart:84:
expect(result['class']['name'], equals('int'));
I think we still want to use the ScrubbedName (the variant that keeps the
internal class names) here and elsewhere in the service protocol.

[regis, 2016-02-23 18:51:00]

On 2016/02/23 17:42:53, Cutch wrote:
>
https://codereview.chromium.org/1723733002/diff/1/runtime/observatory/tests/s...
> File runtime/observatory/tests/service/get_object_rpc_test.dart (right):
> 
>
https://codereview.chromium.org/1723733002/diff/1/runtime/observatory/tests/s...
> runtime/observatory/tests/service/get_object_rpc_test.dart:84:
> expect(result['class']['name'], equals('int'));
> I think we still want to use the ScrubbedName (the variant that keeps the
> internal class names) here and elsewhere in the service protocol.

It is not what we discussed and agreed upon yesterday. Using the UserVisibleName
and the unchanged VM name was the premise of this cl. If you still want to
display the scrubbed name for types, I have to go back to supporting 3 different
flavors when building type names. Let's discuss again in person tomorrow.

[Cutch, 2016-02-23 18:55:54]

On 2016/02/23 18:51:00, regis wrote:
> On 2016/02/23 17:42:53, Cutch wrote:
> >
>
https://codereview.chromium.org/1723733002/diff/1/runtime/observatory/tests/s...
> > File runtime/observatory/tests/service/get_object_rpc_test.dart (right):
> > 
> >
>
https://codereview.chromium.org/1723733002/diff/1/runtime/observatory/tests/s...
> > runtime/observatory/tests/service/get_object_rpc_test.dart:84:
> > expect(result['class']['name'], equals('int'));
> > I think we still want to use the ScrubbedName (the variant that keeps the
> > internal class names) here and elsewhere in the service protocol.
> 
> It is not what we discussed and agreed upon yesterday. Using the
UserVisibleName
> and the unchanged VM name was the premise of this cl. If you still want to
> display the scrubbed name for types, I have to go back to supporting 3
different
> flavors when building type names. Let's discuss again in person tomorrow.

Yes, I realize I was mistaken yesterday. Let's discuss tomorrow in person and
see what we can do

[regis, 2016-02-24 23:41:39]

regis@google.com changed reviewers:
+ asiva@google.com, iposva@google.com, rmacnak@google.com

[regis, 2016-02-24 23:41:40]

PTAL

I like this version much better. I do not change expectations for class names
in the vm service or profiler anymore.

However, types do not have a PrettyName (or ScrubbedName) anymore, only an
internal name and a user visible name.

In the case of functions and fields, there is no difference between a scrubbed
name and a user visible name (no remapping of function names). The difference
only exists if the name is qualified. In that case, the owner class name is
either scrubbed or user visible (scrubbed with remapping). 

When PRODUCT is defined, no caching of user visible name occurs in Class. Also,
scrubbed names and user visible names are identical and only lose the private
portion of their names (no remapping, no getter/setter/constructor scrubbing).
We still need to test if this is acceptable. Revisiting this does not change the
name cleanup done in this cl.

One more detail: the distinction between Code::Name() and Code::PrettyName()
did not make much sense when looking at the implementation. The difference is
actually whether the name is qualified or not. Therefore, there is now
Code::Name() and Code::QualifiedName().

[Cutch, 2016-02-25 00:10:07]

Nice!

LGTM with some suggestions.

https://codereview.chromium.org/1723733002/diff/20001/runtime/vm/deopt_instru...
File runtime/vm/deopt_instructions.cc (right):

https://codereview.chromium.org/1723733002/diff/20001/runtime/vm/deopt_instru...
runtime/vm/deopt_instructions.cc:146: // Allocate what is needed before
StarEvent, which is blocking allocation
suggestion: Allocate all Dart objects needed before calling StartEvent, which
blocks safe points until Complete is called.

https://codereview.chromium.org/1723733002/diff/20001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/1723733002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:222: #if !defined(PRODUCT)
NOT_IN_PRODUCT(
....
);

https://codereview.chromium.org/1723733002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:272: #if !defined(PRODUCT)
NOT_IN_PRODUCT(
....
);

https://codereview.chromium.org/1723733002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:3199: #if !defined(PRODUCT)
NOT_IN_PRODUCT(
....
);

[Ivan Posva, 2016-02-25 00:22:14]

https://codereview.chromium.org/1723733002/diff/20001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/1723733002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:3331: #endif  // !defined(PRODUCT)
I don't think this will compile in PRODUCT, because half of the switch statement
survives.

How about:
NOT_IN_PRODUCT(
  switch(id()) {
    ... list of class ids and corresponding remappings ...
    // default: left out.
  })
  const String& name = ...
  return String::ScrubName(name);

[Ivan Posva, 2016-02-25 00:23:41]

https://codereview.chromium.org/1723733002/diff/20001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/1723733002/diff/20001/runtime/vm/object.h#new...
runtime/vm/object.h:2104: RawString* ScrubbedName() const;
Why do we need two different entry points doing exactly the same thing?

https://codereview.chromium.org/1723733002/diff/20001/runtime/vm/object.h#new...
runtime/vm/object.h:2887: RawString* ScrubbedName() const;
ditto

[regis, 2016-02-25 00:34:12]

https://codereview.chromium.org/1723733002/diff/20001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/1723733002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:3331: #endif  // !defined(PRODUCT)
On 2016/02/25 00:22:14, Ivan Posva wrote:
> I don't think this will compile in PRODUCT, because half of the switch
statement
> survives.
> 
> How about:
> NOT_IN_PRODUCT(
>   switch(id()) {
>     ... list of class ids and corresponding remappings ...
>     // default: left out.
>   })
>   const String& name = ...
>   return String::ScrubName(name);

Indeed, it does not compile. A debug build does not build product, as I just
noticed while trying a release build on the shuttle :-)

I will fix it and upload a new version tomorrow. Is there an easy way I can test
a product build?

[Ivan Posva, 2016-02-25 00:37:30]

On 2016/02/25 00:34:12, regis wrote:
> https://codereview.chromium.org/1723733002/diff/20001/runtime/vm/object.cc
> File runtime/vm/object.cc (right):
> 
>
https://codereview.chromium.org/1723733002/diff/20001/runtime/vm/object.cc#ne...
> runtime/vm/object.cc:3331: #endif  // !defined(PRODUCT)
> On 2016/02/25 00:22:14, Ivan Posva wrote:
> > I don't think this will compile in PRODUCT, because half of the switch
> statement
> > survives.
> > 
> > How about:
> > NOT_IN_PRODUCT(
> >   switch(id()) {
> >     ... list of class ids and corresponding remappings ...
> >     // default: left out.
> >   })
> >   const String& name = ...
> >   return String::ScrubName(name);
> 
> Indeed, it does not compile. A debug build does not build product, as I just
> noticed while trying a release build on the shuttle :-)
> 
> I will fix it and upload a new version tomorrow. Is there an easy way I can
test
> a product build?

Build with mode=product similar to building with mode=release or mode=debug.

-Ivan

[regis, 2016-02-25 17:47:27]

https://codereview.chromium.org/1723733002/diff/20001/runtime/vm/deopt_instru...
File runtime/vm/deopt_instructions.cc (right):

https://codereview.chromium.org/1723733002/diff/20001/runtime/vm/deopt_instru...
runtime/vm/deopt_instructions.cc:146: // Allocate what is needed before
StarEvent, which is blocking allocation
On 2016/02/25 00:10:06, Cutch wrote:
> suggestion: Allocate all Dart objects needed before calling StartEvent, which
> blocks safe points until Complete is called.

Done.

https://codereview.chromium.org/1723733002/diff/20001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/1723733002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:222: #if !defined(PRODUCT)
On 2016/02/25 00:10:07, Cutch wrote:
> NOT_IN_PRODUCT(
> ....
> );

Done.

https://codereview.chromium.org/1723733002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:272: #if !defined(PRODUCT)
On 2016/02/25 00:10:07, Cutch wrote:
> NOT_IN_PRODUCT(
> ....
> );

Done.

https://codereview.chromium.org/1723733002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:3199: #if !defined(PRODUCT)
On 2016/02/25 00:10:07, Cutch wrote:
> NOT_IN_PRODUCT(
> ....
> );

Done.

https://codereview.chromium.org/1723733002/diff/20001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/1723733002/diff/20001/runtime/vm/object.h#new...
runtime/vm/object.h:2104: RawString* ScrubbedName() const;
On 2016/02/25 00:23:41, Ivan Posva wrote:
> Why do we need two different entry points doing exactly the same thing?

It makes the call sites easier to read when one or the other is expected in a
given context. But comments can achieve the same. I removed the ScrubbedName()
entry point here and in class Field as well.

https://codereview.chromium.org/1723733002/diff/20001/runtime/vm/object.h#new...
runtime/vm/object.h:2887: RawString* ScrubbedName() const;
On 2016/02/25 00:23:41, Ivan Posva wrote:
> ditto

Done.

[siva, 2016-02-25 18:49:48]

lgtm

https://codereview.chromium.org/1723733002/diff/40001/runtime/vm/profiler_ser...
File runtime/vm/profiler_service.cc (right):

https://codereview.chromium.org/1723733002/diff/40001/runtime/vm/profiler_ser...
runtime/vm/profiler_service.cc:564: SetName(name.ToCString());
SetName can be hoisted down outside the if {...} else {...}, similar to have you
have hoisted name up.

[regis, 2016-02-25 19:20:50]

Thanks

https://codereview.chromium.org/1723733002/diff/40001/runtime/vm/profiler_ser...
File runtime/vm/profiler_service.cc (right):

https://codereview.chromium.org/1723733002/diff/40001/runtime/vm/profiler_ser...
runtime/vm/profiler_service.cc:564: SetName(name.ToCString());
On 2016/02/25 18:49:48, siva wrote:
> SetName can be hoisted down outside the if {...} else {...}, similar to have
you
> have hoisted name up.

Done.

[regis, 2016-02-25 19:21:31]

Description was changed from

==========
Simplify various name flavors in VM.
==========

to

==========
Simplify various name flavors in VM.

R=asiva@google.com, johnmccutchan@google.com

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

[regis, 2016-02-25 19:21:33]

Committed patchset #4 (id:60001) manually as
3ffe9922b740ba6aabe2fef0b6adfae05c42a081 (presubmit successful).