VM: Make precompiler process constants/fields only once, avoid using pool.InfoAt()

VM: Make precompiler process constants/fields only once, avoid using pool.InfoAt()

R=fschneider@google.com, vegorov@google.com

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

[kustermann, 2016-12-08 13:52:43]

kustermann@google.com changed reviewers:
+ fschneider@google.com, vegorov@google.com

[kustermann, 2016-12-08 13:52:44]

https://codereview.chromium.org/2556193003/diff/1/runtime/vm/precompiler.cc
File runtime/vm/precompiler.cc (right):

https://codereview.chromium.org/2556193003/diff/1/runtime/vm/precompiler.cc#n...
runtime/vm/precompiler.cc:842: if (pool_info.InfoAt(i) ==
ObjectPool::kTaggedObject) {
This moves the handle allocation out of the loop.

Side note: Note that all these handles here which are not used/necessary after
the call to AddCalleesOf() is done, will be kept alive for a long time: It seems
the next closest handle scope upwards the stack is the global one from here:


 323 void Precompiler::DoCompileAll(
 324     Dart_QualifiedFunctionName embedder_entry_points[]) {
 325   ASSERT(I->compilation_allowed());
 326 
 327   {
 328     StackZone stack_zone(T);
 329     zone_ = stack_zone.GetZone();
 330     
 331     { 
 332       HANDLESCOPE(T);

https://codereview.chromium.org/2556193003/diff/1/runtime/vm/precompiler.cc#n...
runtime/vm/precompiler.cc:1039:
consts_to_retain_.Insert(&Instance::ZoneHandle(Z, instance.raw()));
Please note that

  consts_to_retain_.Insert()

calls

  DirectChainedHashMap<InstanceKeyValueTrait>::Insert()

which seems to unconditionally add new elements (even if they already exist)
See here:
https://github.com/dart-lang/sdk/tree/fa038d003baa411f2f39d70c73eb745b9707fc5...


As a side note: This DirectChainedHashMap is really weired (one reason is the
Insert() behavior), since:

  map.Insert(Pair(mykey, 1));
  map.Insert(Pair(mykey, 2));
  map.Lookup(mykey);

might give you 1 back or 2 -- depends on rehashing AFAIK!

Luckily in this case we treat it as a Set so it makes no difference (except for
increased memory footprint for duplicate entries).

https://codereview.chromium.org/2556193003/diff/1/runtime/vm/precompiler.cc#n...
runtime/vm/precompiler.cc:2122: if (pool_info.InfoAt(i) !=
ObjectPool::kTaggedObject) continue;
Same thing: Move handle allocation out of loop.

[Vyacheslav Egorov (Google), 2016-12-08 18:18:19]

lgtm

https://codereview.chromium.org/2556193003/diff/1/runtime/vm/precompiler.cc
File runtime/vm/precompiler.cc (right):

https://codereview.chromium.org/2556193003/diff/1/runtime/vm/precompiler.cc#n...
runtime/vm/precompiler.cc:1039:
consts_to_retain_.Insert(&Instance::ZoneHandle(Z, instance.raw()));
On 2016/12/08 13:52:44, kustermann wrote:
> Please note that
> 
>   consts_to_retain_.Insert()
> 
> calls
> 
>   DirectChainedHashMap<InstanceKeyValueTrait>::Insert()
> 
> which seems to unconditionally add new elements (even if they already exist)
> See here:
>
https://github.com/dart-lang/sdk/tree/fa038d003baa411f2f39d70c73eb745b9707fc5...
> 
> 
> As a side note: This DirectChainedHashMap is really weired (one reason is the
> Insert() behavior), since:
> 
>   map.Insert(Pair(mykey, 1));
>   map.Insert(Pair(mykey, 2));
>   map.Lookup(mykey);
> 
> might give you 1 back or 2 -- depends on rehashing AFAIK!
> 
> Luckily in this case we treat it as a Set so it makes no difference (except
for
> increased memory footprint for duplicate entries).

What about adding an ASSERT there?

[Florian Schneider, 2016-12-08 18:30:45]

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

https://codereview.chromium.org/2556193003/diff/1/runtime/vm/object.h#newcode...
runtime/vm/object.h:8658: class ObjectPoolInfo {
: public ValueObject

https://codereview.chromium.org/2556193003/diff/1/runtime/vm/object.h#newcode...
runtime/vm/object.h:8663: explicit ObjectPoolInfo(const TypedData& info_array) :
array_(info_array) {}
Not needed?

https://codereview.chromium.org/2556193003/diff/1/runtime/vm/precompiler.cc
File runtime/vm/precompiler.cc (right):

https://codereview.chromium.org/2556193003/diff/1/runtime/vm/precompiler.cc#n...
runtime/vm/precompiler.cc:2120: ObjectPoolInfo pool_info(info_array_);
Why not just:

ObjectPoolInfo pool_info(pool_);

[kustermann, 2016-12-08 19:14:38]

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

https://codereview.chromium.org/2556193003/diff/1/runtime/vm/object.h#newcode...
runtime/vm/object.h:8658: class ObjectPoolInfo {
On 2016/12/08 18:30:45, Florian Schneider wrote:
> : public ValueObject

Done.

https://codereview.chromium.org/2556193003/diff/1/runtime/vm/object.h#newcode...
runtime/vm/object.h:8663: explicit ObjectPoolInfo(const TypedData& info_array) :
array_(info_array) {}
On 2016/12/08 18:30:45, Florian Schneider wrote:
> Not needed?

See comment on call site. Using this constructor means one can re-use a
[TypedData] handle when looping over many different functions.

https://codereview.chromium.org/2556193003/diff/1/runtime/vm/precompiler.cc
File runtime/vm/precompiler.cc (right):

https://codereview.chromium.org/2556193003/diff/1/runtime/vm/precompiler.cc#n...
runtime/vm/precompiler.cc:1039:
consts_to_retain_.Insert(&Instance::ZoneHandle(Z, instance.raw()));
On 2016/12/08 18:18:19, Vyacheslav Egorov (Google) wrote:
> On 2016/12/08 13:52:44, kustermann wrote:
> > Please note that
> > 
> >   consts_to_retain_.Insert()
> > 
> > calls
> > 
> >   DirectChainedHashMap<InstanceKeyValueTrait>::Insert()
> > 
> > which seems to unconditionally add new elements (even if they already exist)
> > See here:
> >
>
https://github.com/dart-lang/sdk/tree/fa038d003baa411f2f39d70c73eb745b9707fc5...
> > 
> > 
> > As a side note: This DirectChainedHashMap is really weired (one reason is
the
> > Insert() behavior), since:
> > 
> >   map.Insert(Pair(mykey, 1));
> >   map.Insert(Pair(mykey, 2));
> >   map.Lookup(mykey);
> > 
> > might give you 1 back or 2 -- depends on rehashing AFAIK!
> > 
> > Luckily in this case we treat it as a Set so it makes no difference (except
> for
> > increased memory footprint for duplicate entries).
> 
> What about adding an ASSERT there?

If it's fine with you, I'll do it in a different CL

There might be other places where we accidentally Insert() the same key twice.
So after adding the ASSERT() I should test this in many configurations which
takes a long time.

https://codereview.chromium.org/2556193003/diff/1/runtime/vm/precompiler.cc#n...
runtime/vm/precompiler.cc:2120: ObjectPoolInfo pool_info(info_array_);
On 2016/12/08 18:30:45, Florian Schneider wrote:
> Why not just:
> 
> ObjectPoolInfo pool_info(pool_);

So we allocate one handle per [SwitchICCallsVisitor] instead of one new handle
for every single [Function] it visits.

(i.e. Same reason why all the other handles are instance variables instead of
method locals.)

[Florian Schneider, 2016-12-08 19:20:32]

lgtm

[Vyacheslav Egorov (Google), 2016-12-08 19:21:58]

one more comment (if we are adding ValueObject)

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

https://codereview.chromium.org/2556193003/diff/20001/runtime/vm/object.h#new...
runtime/vm/object.h:8674: const TypedData& array_;
DISALLOW_COPY_AND_ASSIGN?

[kustermann, 2016-12-08 19:30:20]

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

https://codereview.chromium.org/2556193003/diff/20001/runtime/vm/object.h#new...
runtime/vm/object.h:8674: const TypedData& array_;
On 2016/12/08 19:21:58, Vyacheslav Egorov (Google) wrote:
> DISALLOW_COPY_AND_ASSIGN?

Isn't the one from the [ValueObject] superclass enough?

[kustermann, 2016-12-09 08:52:14]

Description was changed from

==========
VM: Make precompiler process constants/fields only once, avoid using
pool.InfoAt()
==========

to

==========
VM: Make precompiler process constants/fields only once, avoid using
pool.InfoAt()

R=fschneider@google.com, vegorov@google.com

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

[kustermann, 2016-12-09 08:52:16]

Committed patchset #2 (id:20001) manually as
b57eaa5f37dcfad2094af83e9ceb6af4becc9f93 (presubmit successful).

[kustermann, 2016-12-09 08:52:32]

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

https://codereview.chromium.org/2556193003/diff/20001/runtime/vm/object.h#new...
runtime/vm/object.h:8674: const TypedData& array_;
On 2016/12/08 19:30:19, kustermann wrote:
> On 2016/12/08 19:21:58, Vyacheslav Egorov (Google) wrote:
> > DISALLOW_COPY_AND_ASSIGN?
> 
> Isn't the one from the [ValueObject] superclass enough?

I think it is.