Initial UserTag and dart:profiler library

Initial UserTag and dart:profiler library

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

Committed: https://code.google.com/p/dart/source/detail?r=34949

[srdjan, 2014-04-09 17:08:18]

lgtm

https://codereview.chromium.org/230863005/diff/20001/runtime/lib/profiler.cc
File runtime/lib/profiler.cc (right):

https://codereview.chromium.org/230863005/diff/20001/runtime/lib/profiler.cc#...
runtime/lib/profiler.cc:34: GET_NON_NULL_NATIVE_ARGUMENT(UserTag, self,
arguments->NativeArgAt(0));
Use UserTag::CheckedHandle where type is known (e.g., it is the receiver)

https://codereview.chromium.org/230863005/diff/20001/runtime/lib/profiler.cc#...
runtime/lib/profiler.cc:40: GET_NON_NULL_NATIVE_ARGUMENT(UserTag, self,
arguments->NativeArgAt(0));
ditto

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

https://codereview.chromium.org/230863005/diff/20001/runtime/vm/object.cc#new...
runtime/vm/object.cc:1170: 
Remove 2 empty lines (one is enough).

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

https://codereview.chromium.org/230863005/diff/20001/runtime/vm/object.h#newc...
runtime/vm/object.h:6660: void make_active();
MakeActive

https://codereview.chromium.org/230863005/diff/20001/runtime/vm/object.h#newc...
runtime/vm/object.h:6669: static bool TagTableFull(Isolate* isolate);
TagTableIsFull ?

https://codereview.chromium.org/230863005/diff/40001/runtime/lib/profiler.cc
File runtime/lib/profiler.cc (right):

https://codereview.chromium.org/230863005/diff/40001/runtime/lib/profiler.cc#...
runtime/lib/profiler.cc:17: 
Since natives are slow comparably, it is important to have this natives inlined
in one of next CLs.

https://codereview.chromium.org/230863005/diff/40001/runtime/lib/profiler.dart
File runtime/lib/profiler.dart (right):

https://codereview.chromium.org/230863005/diff/40001/runtime/lib/profiler.dar...
runtime/lib/profiler.dart:17: void makeCurrent() native "UserTag_makeCurrent";
As discussed, user will need more support: e.g, disable active tag, query active
tag.

[Cutch, 2014-04-09 20:28:53]

https://codereview.chromium.org/230863005/diff/20001/runtime/lib/profiler.cc
File runtime/lib/profiler.cc (right):

https://codereview.chromium.org/230863005/diff/20001/runtime/lib/profiler.cc#...
runtime/lib/profiler.cc:34: GET_NON_NULL_NATIVE_ARGUMENT(UserTag, self,
arguments->NativeArgAt(0));
On 2014/04/09 17:08:18, srdjan wrote:
> Use UserTag::CheckedHandle where type is known (e.g., it is the receiver)

Done.

https://codereview.chromium.org/230863005/diff/20001/runtime/lib/profiler.cc#...
runtime/lib/profiler.cc:40: GET_NON_NULL_NATIVE_ARGUMENT(UserTag, self,
arguments->NativeArgAt(0));
On 2014/04/09 17:08:18, srdjan wrote:
> ditto

Done.

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

https://codereview.chromium.org/230863005/diff/20001/runtime/vm/object.cc#new...
runtime/vm/object.cc:1170: 
On 2014/04/09 17:08:18, srdjan wrote:
> Remove 2 empty lines (one is enough).

Done.

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

https://codereview.chromium.org/230863005/diff/20001/runtime/vm/object.h#newc...
runtime/vm/object.h:6660: void make_active();
On 2014/04/09 17:08:18, srdjan wrote:
> MakeActive

Done.

https://codereview.chromium.org/230863005/diff/20001/runtime/vm/object.h#newc...
runtime/vm/object.h:6669: static bool TagTableFull(Isolate* isolate);
On 2014/04/09 17:08:18, srdjan wrote:
> TagTableIsFull ?

Done.

https://codereview.chromium.org/230863005/diff/40001/runtime/lib/profiler.cc
File runtime/lib/profiler.cc (right):

https://codereview.chromium.org/230863005/diff/40001/runtime/lib/profiler.cc#...
runtime/lib/profiler.cc:17: 
On 2014/04/09 17:08:19, srdjan wrote:
> Since natives are slow comparably, it is important to have this natives
inlined
> in one of next CLs.

This will be in the next CL.

https://codereview.chromium.org/230863005/diff/40001/runtime/lib/profiler.dart
File runtime/lib/profiler.dart (right):

https://codereview.chromium.org/230863005/diff/40001/runtime/lib/profiler.dar...
runtime/lib/profiler.dart:17: void makeCurrent() native "UserTag_makeCurrent";
On 2014/04/09 17:08:19, srdjan wrote:
> As discussed, user will need more support: e.g, disable active tag, query
active
> tag.

Added clearCurrentTag and getCurrentTag

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

https://codereview.chromium.org/230863005/diff/40001/runtime/vm/tags.cc#newco...
runtime/vm/tags.cc:86: const Instance& tag = Instance::Handle(
This is dead code.

[siva, 2014-04-09 21:32:00]

https://codereview.chromium.org/230863005/diff/60001/runtime/lib/profiler.cc
File runtime/lib/profiler.cc (right):

https://codereview.chromium.org/230863005/diff/60001/runtime/lib/profiler.cc#...
runtime/lib/profiler.cc:49: return object_store->current_tag();
Not sure why the current tag is in the object store and not in the Isolate
object?

Normally the object store contains objects that don't change.

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

https://codereview.chromium.org/230863005/diff/60001/runtime/vm/object.cc#new...
runtime/vm/object.cc:1161: RegisterPrivateClass(cls, Symbols::_UserTag(), lib);
pending_classes.Add(cls) ?

https://codereview.chromium.org/230863005/diff/60001/runtime/vm/object.cc#new...
runtime/vm/object.cc:1168: pending_classes.Add(cls);
Why do you have to initialize the UserTag class like this, wouldn't it get setup
when the profiler library is parsed?

https://codereview.chromium.org/230863005/diff/60001/runtime/vm/object.cc#new...
runtime/vm/object.cc:18309: isolate->object_store()->set_current_tag(*this);
should be isolate->set_current_tag();

https://codereview.chromium.org/230863005/diff/60001/runtime/vm/object.cc#new...
runtime/vm/object.cc:18351: isolate, isolate->object_store()->tag_table());
Why is the tag table in the object store and not in the isolate.

https://codereview.chromium.org/230863005/diff/60001/runtime/vm/object.cc#new...
runtime/vm/object.cc:18379: #endif
We should asert here that TagTableIsFull(isolate) is false.

https://codereview.chromium.org/230863005/diff/60001/runtime/vm/object.cc#new...
runtime/vm/object.cc:18404: }
If tag ids go sequentially this would just be an index into the growable array
right there would not be a need to iterate and match the tag id.

https://codereview.chromium.org/230863005/diff/60001/runtime/vm/object.cc#new...
runtime/vm/object.cc:18431: }
As discussed offline, since you don't provide a means to remove a tag we could
just go with a scheme to increment the tag ids and grad the next one instead of
this bit vector scheme of finding the next available tag.

I think the ability to delete tags is probably not going to be needed.

https://codereview.chromium.org/230863005/diff/60001/runtime/vm/object_store.h
File runtime/vm/object_store.h (right):

https://codereview.chromium.org/230863005/diff/60001/runtime/vm/object_store....
runtime/vm/object_store.h:505: RawUserTag* current_tag_;
current_tag_ and tag_table_ move to the isolate structure...

https://codereview.chromium.org/230863005/diff/60001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/230863005/diff/60001/runtime/vm/raw_object.h#...
runtime/vm/raw_object.h:72: V(UserTag)                                          
                      \
IsObjectStoreClassId in snapshot.cc needs to be updated for this new class id.

https://codereview.chromium.org/230863005/diff/60001/runtime/vm/tags.cc
File runtime/vm/tags.cc (right):

https://codereview.chromium.org/230863005/diff/60001/runtime/vm/tags.cc#newco...
runtime/vm/tags.cc:91: const String& label = String::Handle(tag.label());
isolate, tag.label()) here as you already use that pattern above?

[Cutch, 2014-04-09 22:27:20]

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

https://codereview.chromium.org/230863005/diff/60001/runtime/vm/object.cc#new...
runtime/vm/object.cc:1161: RegisterPrivateClass(cls, Symbols::_UserTag(), lib);
On 2014/04/09 21:32:01, siva wrote:
> pending_classes.Add(cls) ?

Done.

https://codereview.chromium.org/230863005/diff/60001/runtime/vm/object.cc#new...
runtime/vm/object.cc:1168: pending_classes.Add(cls);
On 2014/04/09 21:32:01, siva wrote:
> Why do you have to initialize the UserTag class like this, wouldn't it get
setup
> when the profiler library is parsed?

Done.

https://codereview.chromium.org/230863005/diff/60001/runtime/vm/object.cc#new...
runtime/vm/object.cc:18309: isolate->object_store()->set_current_tag(*this);
On 2014/04/09 21:32:01, siva wrote:
> should be isolate->set_current_tag();

Done.

https://codereview.chromium.org/230863005/diff/60001/runtime/vm/object.cc#new...
runtime/vm/object.cc:18351: isolate, isolate->object_store()->tag_table());
On 2014/04/09 21:32:01, siva wrote:
> Why is the tag table in the object store and not in the isolate.

Moved to isolate.

https://codereview.chromium.org/230863005/diff/60001/runtime/vm/object.cc#new...
runtime/vm/object.cc:18379: #endif
On 2014/04/09 21:32:01, siva wrote:
> We should asert here that TagTableIsFull(isolate) is false.

Done.

https://codereview.chromium.org/230863005/diff/60001/runtime/vm/object.cc#new...
runtime/vm/object.cc:18404: }
On 2014/04/09 21:32:01, siva wrote:
> If tag ids go sequentially this would just be an index into the growable array
> right there would not be a need to iterate and match the tag id.

Done.

https://codereview.chromium.org/230863005/diff/60001/runtime/vm/object.cc#new...
runtime/vm/object.cc:18431: }
On 2014/04/09 21:32:01, siva wrote:
> As discussed offline, since you don't provide a means to remove a tag we could
> just go with a scheme to increment the tag ids and grad the next one instead
of
> this bit vector scheme of finding the next available tag.
> 
> I think the ability to delete tags is probably not going to be needed.

Done.

https://codereview.chromium.org/230863005/diff/60001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/230863005/diff/60001/runtime/vm/raw_object.h#...
runtime/vm/raw_object.h:72: V(UserTag)                                          
                      \
On 2014/04/09 21:32:01, siva wrote:
> IsObjectStoreClassId in snapshot.cc needs to be updated for this new class id.

Now that I don't store this in the object_store this doesn't matter?

https://codereview.chromium.org/230863005/diff/60001/runtime/vm/tags.cc
File runtime/vm/tags.cc (right):

https://codereview.chromium.org/230863005/diff/60001/runtime/vm/tags.cc#newco...
runtime/vm/tags.cc:91: const String& label = String::Handle(tag.label());
On 2014/04/09 21:32:01, siva wrote:
> isolate, tag.label()) here as you already use that pattern above?

Done.

[siva, 2014-04-10 21:35:23]

LGTM with comments.

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

https://codereview.chromium.org/230863005/diff/120001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:1464: object_store->set_object_class(cls);
object_store->set_user_tag_class(cls);

https://codereview.chromium.org/230863005/diff/120001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:18331: ASSERT(!TagTableIsFull(isolate));
why is this ASSERT needed here, you have already checked above that the TagTable
is not full.

https://codereview.chromium.org/230863005/diff/120001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:18348: result.set_tag(tag_id);
Maybe it makes sense to move this code which computes the tag_id inside to
AddTagToIsolate as that code consumes the id and it would be kind of atomic
(consume and set the tag id).

https://codereview.chromium.org/230863005/diff/120001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:18400: RawUserTag* UserTag::FindTagById(uword tag_id) {
I don't think this function is used anymore, can be deleted?

https://codereview.chromium.org/230863005/diff/140001/sdk/lib/profiler/profil...
File sdk/lib/profiler/profiler.dart (right):

https://codereview.chromium.org/230863005/diff/140001/sdk/lib/profiler/profil...
sdk/lib/profiler/profiler.dart:17: /// Make [this] the current tag for the
isolate.c
isolate.c ??

https://codereview.chromium.org/230863005/diff/140001/sdk/lib/profiler/profil...
sdk/lib/profiler/profiler.dart:21: class _FakeUserTag implements UserTag {
Maybe add a comment on top of this class to indicate it is the default
implementation. If dart2js implements this functionality the class could be
removed.

[Cutch, 2014-04-10 22:18:29]

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

https://codereview.chromium.org/230863005/diff/120001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:1464: object_store->set_object_class(cls);
On 2014/04/10 21:35:24, siva wrote:
> object_store->set_user_tag_class(cls);

Done.

https://codereview.chromium.org/230863005/diff/120001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:18331: ASSERT(!TagTableIsFull(isolate));
On 2014/04/10 21:35:24, siva wrote:
> why is this ASSERT needed here, you have already checked above that the
TagTable
> is not full.

Done.

https://codereview.chromium.org/230863005/diff/120001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:18348: result.set_tag(tag_id);
On 2014/04/10 21:35:24, siva wrote:
> Maybe it makes sense to move this code which computes the tag_id inside to
> AddTagToIsolate as that code consumes the id and it would be kind of atomic
> (consume and set the tag id).

Done.

https://codereview.chromium.org/230863005/diff/120001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:18400: RawUserTag* UserTag::FindTagById(uword tag_id) {
On 2014/04/10 21:35:24, siva wrote:
> I don't think this function is used anymore, can be deleted?

It is used by UserTags::TagName in tags.cc

https://codereview.chromium.org/230863005/diff/140001/sdk/lib/profiler/profil...
File sdk/lib/profiler/profiler.dart (right):

https://codereview.chromium.org/230863005/diff/140001/sdk/lib/profiler/profil...
sdk/lib/profiler/profiler.dart:17: /// Make [this] the current tag for the
isolate.c
On 2014/04/10 21:35:24, siva wrote:
> isolate.c ??

Done.

https://codereview.chromium.org/230863005/diff/140001/sdk/lib/profiler/profil...
sdk/lib/profiler/profiler.dart:21: class _FakeUserTag implements UserTag {
On 2014/04/10 21:35:24, siva wrote:
> Maybe add a comment on top of this class to indicate it is the default
> implementation. If dart2js implements this functionality the class could be
> removed.

Done.

[Cutch, 2014-04-10 22:33:07]

Committed patchset #11 manually as r34949 (presubmit successful).