Copy ICData descriptors when starting background compilation, so that they do not change while comp…

Copy ICData descriptors when starting background compilation, so that they do not change while compiling

BUG=
R=asiva@google.com

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

[srdjan, 2015-10-27 22:49:58]

srdjan@google.com changed reviewers:
+ asiva@google.com

[siva, 2015-10-27 23:36:46]

LGTM with some comments.

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

https://codereview.chromium.org/1414933006/diff/20001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:432: ASSERT(!Thread::Current()->IsMutatorThread());
ASSERT(!thread->IsMutatorThread());

https://codereview.chromium.org/1414933006/diff/20001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:436: ASSERT(Thread::Current()->IsMutatorThread());
Ditto.

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

https://codereview.chromium.org/1414933006/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:12765: result.set_ic_data_array(Array::Handle(zone,
from.ic_data()));
The ic_data array doesn't have to be cloned?

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

https://codereview.chromium.org/1414933006/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:7145: }
This function seems identical to RestoreICDataMap except for the CloneDescriptor
part.

Can we just call RestoreICDataMap in both cases (IsMutatorThread and
!IsMutatorThread) and then in the case of !IsMutatorThread we could do a loop to
clone the descriptors ?

Or we could add a 'clone_descriptor' parameter to RestoreICDataMap and call it
accordingly.

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

https://codereview.chromium.org/1414933006/diff/40001/runtime/vm/object.h#new...
runtime/vm/object.h:1923: static RawICData* CloneDescriptor(const ICData& from);
Should this be a private method?

[srdjan, 2015-10-28 16:54:43]

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

https://codereview.chromium.org/1414933006/diff/20001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:432: ASSERT(!Thread::Current()->IsMutatorThread());
On 2015/10/27 23:36:45, siva wrote:
> ASSERT(!thread->IsMutatorThread());

Done.

https://codereview.chromium.org/1414933006/diff/20001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:436: ASSERT(Thread::Current()->IsMutatorThread());
On 2015/10/27 23:36:45, siva wrote:
> Ditto.

Done.

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

https://codereview.chromium.org/1414933006/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:12765: result.set_ic_data_array(Array::Handle(zone,
from.ic_data()));
On 2015/10/27 23:36:45, siva wrote:
> The ic_data array doesn't have to be cloned?

No. Added the following comment in .h:

  // Generates a new ICData with descriptor data copied (shallow clone).
  // Entry array of the result is the same as in 'from'. Once entry array is
  // created, it can only change the 'count', all other properties are invariant
  // (target, cids, number of checks).

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

https://codereview.chromium.org/1414933006/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:7145: }
On 2015/10/27 23:36:45, siva wrote:
> This function seems identical to RestoreICDataMap except for the
CloneDescriptor
> part.
> 
> Can we just call RestoreICDataMap in both cases (IsMutatorThread and
> !IsMutatorThread) and then in the case of !IsMutatorThread we could do a loop
to
> clone the descriptors ?
> 
> Or we could add a 'clone_descriptor' parameter to RestoreICDataMap and call it
> accordingly.

Yes, passing argument clone_descriptor.
Also added (as discussed offline) Compiler::IsBackgroundCompilation().

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

https://codereview.chromium.org/1414933006/diff/40001/runtime/vm/object.h#new...
runtime/vm/object.h:1923: static RawICData* CloneDescriptor(const ICData& from);
On 2015/10/27 23:36:46, siva wrote:
> Should this be a private method?

No, it is used by Function::RestoreICDataMap.

[srdjan, 2015-10-28 17:20:15]

Committed patchset #5 (id:80001) manually as
44ccbb5e295c44b29754705ebca526ff56a0635f (presubmit successful).