Track which entries in the ObjectPool are native entries, and reset them when loading a precompiled…

Track which entries in the ObjectPool are native entries, and reset them when loading a precompiled snapshot.

R=fschneider@google.com

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

[rmacnak, 2015-09-03 23:21:35]

rmacnak@google.com changed reviewers:
+ fschneider@google.com

[Florian Schneider, 2015-09-04 08:05:19]

Why is this needed?

In precompiled code all natives initially invoke LinkNativeCall and patch the
constant pool with the real target.

[rmacnak, 2015-09-04 18:37:07]

On 2015/09/04 08:05:19, Florian Schneider wrote:
> Why is this needed?
> 
> In precompiled code all natives initially invoke LinkNativeCall and patch the
> constant pool with the real target.

const String.fromEnvironment, for example, may become linked when we precompile
- it needs to be unlinked to it will relink in the new process.

Arbitrary natives may become linked when we have a warmup phase. Though
depending on how we do that--maybe we throw all the code out again away--these
might not matter.

[Florian Schneider, 2015-09-05 08:31:53]

On 2015/09/04 18:37:07, rmacnak wrote:
> On 2015/09/04 08:05:19, Florian Schneider wrote:
> > Why is this needed?
> > 
> > In precompiled code all natives initially invoke LinkNativeCall and patch
the
> > constant pool with the real target.
> 
> const String.fromEnvironment, for example, may become linked when we
precompile
> - it needs to be unlinked to it will relink in the new process.
> 
> Arbitrary natives may become linked when we have a warmup phase. Though
> depending on how we do that--maybe we throw all the code out again away--these
> might not matter.

Ok, I see. I guess with precompilation, there is no code executed before
everything is compiled, so there would be no need to reset.

But we you are right to keep that in mind when we may be putting optimized code
in snapshots. Then we need to reset a bunch of things.

[Florian Schneider, 2015-09-05 08:32:58]

On 2015/09/05 08:31:53, Florian Schneider wrote:
> On 2015/09/04 18:37:07, rmacnak wrote:
> > On 2015/09/04 08:05:19, Florian Schneider wrote:
> > > Why is this needed?
> > > 
> > > In precompiled code all natives initially invoke LinkNativeCall and patch
> the
> > > constant pool with the real target.
> > 
> > const String.fromEnvironment, for example, may become linked when we
> precompile
> > - it needs to be unlinked to it will relink in the new process.
> > 
> > Arbitrary natives may become linked when we have a warmup phase. Though
> > depending on how we do that--maybe we throw all the code out again
away--these
> > might not matter.
> 
> Ok, I see. I guess with precompilation, there is no code executed before
> everything is compiled, so there would be no need to reset.
> 

In the same way I'd expect no code executed before the snapshot is loaded,
right?


> But we you are right to keep that in mind when we may be putting optimized
code
> in snapshots. Then we need to reset a bunch of things.

[rmacnak, 2015-09-09 00:44:21]

On 2015/09/05 08:32:58, Florian Schneider wrote:
> On 2015/09/05 08:31:53, Florian Schneider wrote:
> > On 2015/09/04 18:37:07, rmacnak wrote:
> > > On 2015/09/04 08:05:19, Florian Schneider wrote:
> > > > Why is this needed?
> > > > 
> > > > In precompiled code all natives initially invoke LinkNativeCall and
patch
> > the
> > > > constant pool with the real target.
> > > 
> > > const String.fromEnvironment, for example, may become linked when we
> > precompile
> > > - it needs to be unlinked to it will relink in the new process.
> > > 
> > > Arbitrary natives may become linked when we have a warmup phase. Though
> > > depending on how we do that--maybe we throw all the code out again
> away--these
> > > might not matter.
> > 
> > Ok, I see. I guess with precompilation, there is no code executed before
> > everything is compiled, so there would be no need to reset.
> > 
> 
> In the same way I'd expect no code executed before the snapshot is loaded,
> right?
> 
> 
> > But we you are right to keep that in mind when we may be putting optimized
> code
> > in snapshots. Then we need to reset a bunch of things.

Some loading code is in Dart and runs before taking the snapshot.

[rmacnak, 2015-09-09 00:47:36]

On 2015/09/09 00:44:21, rmacnak wrote:
> On 2015/09/05 08:32:58, Florian Schneider wrote:
> > On 2015/09/05 08:31:53, Florian Schneider wrote:
> > > On 2015/09/04 18:37:07, rmacnak wrote:
> > > > On 2015/09/04 08:05:19, Florian Schneider wrote:
> > > > > Why is this needed?
> > > > > 
> > > > > In precompiled code all natives initially invoke LinkNativeCall and
> patch
> > > the
> > > > > constant pool with the real target.
> > > > 
> > > > const String.fromEnvironment, for example, may become linked when we
> > > precompile
> > > > - it needs to be unlinked to it will relink in the new process.
> > > > 
> > > > Arbitrary natives may become linked when we have a warmup phase. Though
> > > > depending on how we do that--maybe we throw all the code out again
> > away--these
> > > > might not matter.
> > > 
> > > Ok, I see. I guess with precompilation, there is no code executed before
> > > everything is compiled, so there would be no need to reset.
> > > 
> > 
> > In the same way I'd expect no code executed before the snapshot is loaded,
> > right?
> > 
> > 
> > > But we you are right to keep that in mind when we may be putting optimized
> > code
> > > in snapshots. Then we need to reset a bunch of things.
> 
> Some loading code is in Dart and runs before taking the snapshot.

As brought up in the compiler meeting, RegExp and Symbol also have const native
constructors that will be run during compilation.

[Florian Schneider, 2015-09-10 20:21:09]

lgtm

https://codereview.chromium.org/1316673005/diff/20001/runtime/vm/assembler_ar...
File runtime/vm/assembler_arm64.cc (right):

https://codereview.chromium.org/1316673005/diff/20001/runtime/vm/assembler_ar...
runtime/vm/assembler_arm64.cc:403: LoadWordFromPoolOffsetFixed(dst, offset);
Use LoadWordFromPool(...) -Fixed not needed, since the instructions are never
patched - only the object pool entry.

https://codereview.chromium.org/1316673005/diff/20001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/1316673005/diff/20001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:950: entry,
Simulator::kBootstrapNativeCall, NativeEntry::kNumArguments);
Argument count of 1 is correct, but redirection->argument_count() does not seem
to be used for this type of Redirection (kBootstrapNativeCall). - Not even for
assertions. Should we just drop it?

[rmacnak, 2015-09-10 22:37:27]

https://codereview.chromium.org/1316673005/diff/20001/runtime/vm/assembler_ar...
File runtime/vm/assembler_arm64.cc (right):

https://codereview.chromium.org/1316673005/diff/20001/runtime/vm/assembler_ar...
runtime/vm/assembler_arm64.cc:403: LoadWordFromPoolOffsetFixed(dst, offset);
On 2015/09/10 20:21:09, Florian Schneider wrote:
> Use LoadWordFromPool(...) -Fixed not needed, since the instructions are never
> patched - only the object pool entry.

Done.

https://codereview.chromium.org/1316673005/diff/20001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/1316673005/diff/20001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:950: entry,
Simulator::kBootstrapNativeCall, NativeEntry::kNumArguments);
On 2015/09/10 20:21:09, Florian Schneider wrote:
> Argument count of 1 is correct, but redirection->argument_count() does not
seem
> to be used for this type of Redirection (kBootstrapNativeCall). - Not even for
> assertions. Should we just drop it?

Ah, I didn't see that redirection case was different. I'll add an assert there
rather than introduce another variant of RedirectExternalReference that doesn't
take an arity.

[rmacnak, 2015-09-10 22:41:41]

Committed patchset #3 (id:40001) manually as
8ebb9f9b39e1a93e670361b899f2a73e041ad113 (presubmit successful).

[Florian Schneider, 2015-09-11 08:06:52]

https://codereview.chromium.org/1316673005/diff/40001/runtime/vm/simulator_ar...
File runtime/vm/simulator_arm.cc (right):

https://codereview.chromium.org/1316673005/diff/40001/runtime/vm/simulator_ar...
runtime/vm/simulator_arm.cc:1581: ASSERT(redirection->argument_count() == 1);
What about simulator_mips.cc and simulator_arm64.cc?

[rmacnak, 2015-09-12 22:29:44]

https://codereview.chromium.org/1316673005/diff/40001/runtime/vm/simulator_ar...
File runtime/vm/simulator_arm.cc (right):

https://codereview.chromium.org/1316673005/diff/40001/runtime/vm/simulator_ar...
runtime/vm/simulator_arm.cc:1581: ASSERT(redirection->argument_count() == 1);
On 2015/09/11 08:06:52, Florian Schneider wrote:
> What about simulator_mips.cc and simulator_arm64.cc?

Oops, uploaded another CL.