In background compilation make a copy of Field in order to freeze its state

In background compilation make a copy of Field in order to freeze its state. Add flag --force_clone_compiler_objects for debugging purpose.

BUG=
R=asiva@google.com

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

[srdjan, 2016-02-25 20:20:17]

Description was changed from

==========
In background compilation make a copy of Field in order to freeze its state

BUG=
==========

to

==========
In background compilation make a copy of Field in order to freeze its state. Add
flag --force_clone_compiler_objects for debugging purpose.

BUG=
==========

[srdjan, 2016-02-25 21:25:57]

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

[siva, 2016-02-25 23:38:33]

LGTM with comments mostly on the style of returning pointer to handles in file
object.h (we have not done that so far).

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

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/ast.cc#newc...
runtime/vm/ast.cc:43: return value.CloneFromOriginal();
Why not create the zone handle here instead of having CloneFromOriginal return a
pointer to a handle. In Object.h we have kept a pattern of returning raw
pointers and taking in arguments as handles.

  return &Field::ZoneHandle(value.CloneFromOriginal());

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/ast.cc#newc...
runtime/vm/ast.cc:50: 
Add 
const Field* AstNode::OriginalAsHandle(const Field& value) const {
  return OriginalAsHandle(Thread::Current()->zone(), value);
}
const Field* AstNode::OriginalAsHandle(Zone* zone, const Field& value) const {
  return &Field::ZoneHandle(zone, value.Original());
}

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/ast.cc#newc...
runtime/vm/ast.cc:597: token_pos(), *field().OriginalAsHandle(), rhs);
token_pos(), *OriginalAsHandle(field(), rhs);

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

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/ast_printer...
runtime/vm/ast_printer.cc:140: VisitGenericFieldNode(node,
*node->field().OriginalAsHandle());
*node->OriginalAsHandle(node->field()));

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/ast_transfo...
File runtime/vm/ast_transformer.cc (right):

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/ast_transfo...
runtime/vm/ast_transformer.cc:525: *node->field().OriginalAsHandle(Z),
ditto comment.

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_builder.cc:3737: *node->field().OriginalAsHandle(Z),
token_pos));
ditto comment.

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/intermediat...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/intermediat...
runtime/vm/intermediate_language_arm.cc:2117: __ LoadObject(temp,
*field().OriginalAsHandle(compiler->zone()));
__ LoadObject(temp, Field::ZoneHandle(compiler->zone(), field().Original()));

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/intermediat...
runtime/vm/intermediate_language_arm.cc:2257: __ LoadObject(temp,
*field().OriginalAsHandle(compiler->zone()));
Ditto.

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/intermediat...
File runtime/vm/intermediate_language_arm64.cc (right):

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/intermediat...
runtime/vm/intermediate_language_arm64.cc:1840: __ LoadObject(temp,
*field().OriginalAsHandle(compiler->zone()));
Ditto.

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/intermediat...
runtime/vm/intermediate_language_arm64.cc:1978: __ LoadObject(temp,
*field().OriginalAsHandle(compiler->zone()));
Ditto.

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/intermediat...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/intermediat...
runtime/vm/intermediate_language_ia32.cc:1832: __ LoadObject(temp,
*field().OriginalAsHandle(compiler->zone()));
Ditto.

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/intermediat...
runtime/vm/intermediate_language_ia32.cc:1978: __ LoadObject(temp,
*field().OriginalAsHandle(compiler->zone()));
Ditto.

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/intermediat...
File runtime/vm/intermediate_language_mips.cc (right):

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/intermediat...
runtime/vm/intermediate_language_mips.cc:2004: __ LoadObject(temp,
*field().OriginalAsHandle(compiler->zone()));
Ditto.

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/intermediat...
runtime/vm/intermediate_language_mips.cc:2110: __ LoadObject(temp,
*field().OriginalAsHandle(compiler->zone()));
Ditto.

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/intermediat...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/intermediat...
runtime/vm/intermediate_language_x64.cc:1858: __ LoadObject(temp,
*field().OriginalAsHandle(compiler->zone()));
Ditto.

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/intermediat...
runtime/vm/intermediate_language_x64.cc:2000: __ LoadObject(temp,
*field().OriginalAsHandle(compiler->zone()));
Ditto.

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

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/object.cc#n...
runtime/vm/object.cc:7077: return field.raw_ptr()->owner_;
Maybe
  if (IsOriginal()) {
    return raw_ptr()->owner_;
  } else {
    const Field& field.....
    ....
    ....
  }

Would save creation of a handle in the common case.

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/object.cc#n...
runtime/vm/object.cc:7089: return PatchClass::Cast(obj).patched_class();
See comment below in Field::Origin

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/object.cc#n...
runtime/vm/object.cc:7101: return PatchClass::Cast(obj).origin_class();
Wouldn't it be more efficient to write this as :
Object& obj = Object::Handle(field.raw_ptr()->owner_);
if (obj.IsField()) {
  obj ^= Field::Cast(obj).raw_ptr()->owner_;
}
if (obj.IsClass()) {
 ...
}
...
...

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/object.cc#n...
runtime/vm/object.cc:7113: return PatchClass::Cast(obj).script();
Ditto comment.

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

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/object.h#ne...
runtime/vm/object.h:2890: }
Can this function be moved to the callers (e.g ast.h) instead of here. We have
been consistent of having all methods in this file return pointer to raw
objects.

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/object.h#ne...
runtime/vm/object.h:2893: return Original() == this->raw();
Wouldn't it be easier to just do:
{
  if (IsNull()) {
    return true;
  }
  NoSafepointScope no_safepoint;
  return this->raw() != this->raw_ptr()->owner_;
}

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/object.h#ne...
runtime/vm/object.h:2898: Field* CloneFromOriginal() const;
RawField* CloneFromOriginal() const;

(See comment in MayCloneField() - ast.cc)

[srdjan, 2016-02-26 00:40:43]

Thanks!

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

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/ast.cc#newc...
runtime/vm/ast.cc:43: return value.CloneFromOriginal();
On 2016/02/25 23:38:32, siva wrote:
> Why not create the zone handle here instead of having CloneFromOriginal return
a
> pointer to a handle. In Object.h we have kept a pattern of returning raw
> pointers and taking in arguments as handles.
> 
>   return &Field::ZoneHandle(value.CloneFromOriginal());

OK, always returning RawField* instead of Field* in object.h.

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/ast.cc#newc...
runtime/vm/ast.cc:50: 
On 2016/02/25 23:38:32, siva wrote:
> Add 
> const Field* AstNode::OriginalAsHandle(const Field& value) const {
>   return OriginalAsHandle(Thread::Current()->zone(), value);
> }
> const Field* AstNode::OriginalAsHandle(Zone* zone, const Field& value) const {
>   return &Field::ZoneHandle(zone, value.Original());
> }

It feels wrong to put that shared functionality in ASTNode, especially since it
cannot be used in the frequent intermediate_language_xxx.cc case. Removing
OriginalAsHandle

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/ast.cc#newc...
runtime/vm/ast.cc:597: token_pos(), *field().OriginalAsHandle(), rhs);
On 2016/02/25 23:38:32, siva wrote:
> token_pos(), *OriginalAsHandle(field(), rhs);

Resolved using Field::ZoneHandle

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

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/ast_printer...
runtime/vm/ast_printer.cc:140: VisitGenericFieldNode(node,
*node->field().OriginalAsHandle());
On 2016/02/25 23:38:32, siva wrote:
> *node->OriginalAsHandle(node->field()));

Resolved using Field::ZoneHandle

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/ast_transfo...
File runtime/vm/ast_transformer.cc (right):

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/ast_transfo...
runtime/vm/ast_transformer.cc:525: *node->field().OriginalAsHandle(Z),
On 2016/02/25 23:38:32, siva wrote:
> ditto comment.


Resolved using Field::ZoneHandle

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_builder.cc:3737: *node->field().OriginalAsHandle(Z),
token_pos));
On 2016/02/25 23:38:32, siva wrote:
> ditto comment.


Resolved using Field::ZoneHandle

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/intermediat...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/intermediat...
runtime/vm/intermediate_language_arm.cc:2117: __ LoadObject(temp,
*field().OriginalAsHandle(compiler->zone()));
On 2016/02/25 23:38:32, siva wrote:
> __ LoadObject(temp, Field::ZoneHandle(compiler->zone(), field().Original()));

Done.

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/intermediat...
File runtime/vm/intermediate_language_arm64.cc (right):

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/intermediat...
runtime/vm/intermediate_language_arm64.cc:1978: __ LoadObject(temp,
*field().OriginalAsHandle(compiler->zone()));
On 2016/02/25 23:38:32, siva wrote:
> Ditto.

Done.

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

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/object.cc#n...
runtime/vm/object.cc:7077: return field.raw_ptr()->owner_;
On 2016/02/25 23:38:33, siva wrote:
> Maybe
>   if (IsOriginal()) {
>     return raw_ptr()->owner_;
>   } else {
>     const Field& field.....
>     ....
>     ....
>   }
> 
> Would save creation of a handle in the common case.

Done.

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/object.cc#n...
runtime/vm/object.cc:7089: return PatchClass::Cast(obj).patched_class();
On 2016/02/25 23:38:33, siva wrote:
> See comment below in Field::Origin

Discussed offline, leaving it as it is so that the indirection implementation is
encapsulated in fewer functions.

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/object.cc#n...
runtime/vm/object.cc:7101: return PatchClass::Cast(obj).origin_class();
On 2016/02/25 23:38:33, siva wrote:
> Wouldn't it be more efficient to write this as :
> Object& obj = Object::Handle(field.raw_ptr()->owner_);
> if (obj.IsField()) {
>   obj ^= Field::Cast(obj).raw_ptr()->owner_;
> }
> if (obj.IsClass()) {
>  ...
> }
> ...
> ...

Discussed offline, leaving it as it is so that the indirection implementation is
encapsulated in fewer functions.

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/object.cc#n...
runtime/vm/object.cc:7113: return PatchClass::Cast(obj).script();
On 2016/02/25 23:38:33, siva wrote:
> Ditto comment.

Discussed offline, leaving it as it is so that the indirection implementation is
encapsulated in fewer functions.

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

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/object.h#ne...
runtime/vm/object.h:2890: }
On 2016/02/25 23:38:33, siva wrote:
> Can this function be moved to the callers (e.g ast.h) instead of here. We have
> been consistent of having all methods in this file return pointer to raw
> objects.

Removed function.

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/object.h#ne...
runtime/vm/object.h:2893: return Original() == this->raw();
On 2016/02/25 23:38:33, siva wrote:
> Wouldn't it be easier to just do:
> {
>   if (IsNull()) {
>     return true;
>   }
>   NoSafepointScope no_safepoint;
>   return this->raw() != this->raw_ptr()->owner_;
> }

Adapted:
  bool IsOriginal() const {
    if (IsNull()) {
      return true;
    }
    NoSafepointScope no_safepoint;
    return !raw_ptr()->owner_->IsField();
  }

https://codereview.chromium.org/1722733002/diff/120001/runtime/vm/object.h#ne...
runtime/vm/object.h:2898: Field* CloneFromOriginal() const;
On 2016/02/25 23:38:33, siva wrote:
> RawField* CloneFromOriginal() const;
> 
> (See comment in MayCloneField() - ast.cc)

Returning RawField*

[srdjan, 2016-02-26 17:07:53]

Description was changed from

==========
In background compilation make a copy of Field in order to freeze its state. Add
flag --force_clone_compiler_objects for debugging purpose.

BUG=
==========

to

==========
In background compilation make a copy of Field in order to freeze its state. Add
flag --force_clone_compiler_objects for debugging purpose.

BUG=
R=asiva@google.com

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

[srdjan, 2016-02-26 17:07:55]

Committed patchset #9 (id:160001) manually as
5a69e3661b528daefd9b63e433f904e3de130eeb (presubmit successful).