Move of the type feedback vector to the closure.

Move of the type feedback vector to the closure.

We get less "pollution" of type feedback if we have one vector per native
context, rather than one for the whole system. This CL moves the vector
appropriately.

BUG=

Committed: https://crrev.com/91c88644dcf396285f88977080ea85f4614bbc02
Cr-Commit-Position: refs/heads/master@{#36539}

[mvstanton, 2016-04-22 09:41:48]

mvstanton@chromium.org changed reviewers:
+ mstarzinger@chromium.org, ulan@chromium.org

[mvstanton, 2016-04-22 09:41:49]

mvstanton@chromium.org changed required reviewers:
+ mstarzinger@chromium.org, ulan@chromium.org

[mvstanton, 2016-04-22 09:41:50]

Hi Michael,
Here is the vector move. Much of this was reviewed/in-the-tree in February, but
this copes with the compiler.cc improvements. There is a TODO in there I'll
address asap (shared->set_num_literals()).

Ulan, can you have a look at the changes in src/heap/?

Thanks!
--Mike

[Michael Starzinger, 2016-05-10 13:47:21]

First round of comments.

https://codereview.chromium.org/1906823002/diff/40001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/1906823002/diff/40001/src/compiler.cc#newcode524
src/compiler.cc:524:
shared->set_num_literals(lit->materialized_literal_count());
Depending on who computes the literals count for a given FunctionLiteral, this
should either be moved to "InitFromFunctionLiteral" (if computed by parser) or
into "Renumber" (if computed by the numbering phase).

https://codereview.chromium.org/1906823002/diff/40001/src/compiler/access-bui...
File src/compiler/access-builder.cc (right):

https://codereview.chromium.org/1906823002/diff/40001/src/compiler/access-bui...
src/compiler/access-builder.cc:415: FieldAccess
AccessBuilder::ForJSFunctionLiterals() {
nit: Please keep these in the same order as in the header (i.e. move the
function to where it was before).

https://codereview.chromium.org/1906823002/diff/40001/src/compiler/access-bui...
File src/compiler/access-builder.h (right):

https://codereview.chromium.org/1906823002/diff/40001/src/compiler/access-bui...
src/compiler/access-builder.h:132: // Provides access to
JSFunction::literals()[0] field.
Accessing an element as a field breaks invariants in TurboFan, this should be
modeled using an ElementAccess. Also this function seems to be unused.

https://codereview.chromium.org/1906823002/diff/40001/src/compiler/js-generic...
File src/compiler/js-generic-lowering.cc (right):

https://codereview.chromium.org/1906823002/diff/40001/src/compiler/js-generic...
src/compiler/js-generic-lowering.cc:483: // functions that don't need literals
cloning.
nit: Last part of the comment is outdated. Better just "Use the
FastNewClusreStin only for functions allocated in new space" here.

https://codereview.chromium.org/1906823002/diff/40001/src/full-codegen/full-c...
File src/full-codegen/full-codegen.cc (right):

https://codereview.chromium.org/1906823002/diff/40001/src/full-codegen/full-c...
src/full-codegen/full-codegen.cc:999: // space for nested functions that don't
need literals cloning. If
nit: Comment is outdated.

https://codereview.chromium.org/1906823002/diff/40001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1906823002/diff/40001/src/heap/heap.cc#newcod...
src/heap/heap.cc:2797: SKIP_WRITE_BARRIER);
nit: I have a hard time reasoning about whether skipping this write barrier is
actually safe. Could we instead just keep the write barrier here? This
initialization is only used in the no-snapshot case anyways.

https://codereview.chromium.org/1906823002/diff/40001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1906823002/diff/40001/src/objects.h#newcode4724
src/objects.h:4724: static const int kFeedbackVectorOffset =
FixedArray::kHeaderSize;
nit: It is possible to use OffsetOfElementAt(kVectorIndex) to initialize this?

https://codereview.chromium.org/1906823002/diff/40001/src/objects.h#newcode4725
src/objects.h:4725: static const int kOffsetToFirstLiteral =
kFeedbackVectorOffset + kPointerSize;
nit: Likewise.

https://codereview.chromium.org/1906823002/diff/40001/src/objects.h#newcode7409
src/objects.h:7409: static void EnsureLiterals(Handle<JSFunction> function);
nit: Please move this down to the "[literals]" section, it doesn't affect the
"[code]" field.

https://codereview.chromium.org/1906823002/diff/40001/src/objects.h#newcode7436
src/objects.h:7436: void ClearTypeFeedbackInfo();
nit: Likewise, please move this down to after the "feedback_vector" accessor.

https://codereview.chromium.org/1906823002/diff/40001/src/optimizing-compile-...
File src/optimizing-compile-dispatcher.cc (right):

https://codereview.chromium.org/1906823002/diff/40001/src/optimizing-compile-...
src/optimizing-compile-dispatcher.cc:25: //
JSFunction::EnsureLiterals(function);
The call is commented out. If it is not needed, then let's drop the entire
paragraph.

https://codereview.chromium.org/1906823002/diff/40001/test/cctest/compiler/fu...
File test/cctest/compiler/function-tester.h (right):

https://codereview.chromium.org/1906823002/diff/40001/test/cctest/compiler/fu...
test/cctest/compiler/function-tester.h:236:
JSFunction::EnsureLiterals(function);
nit: This is only used to compile hand-crafted graphs, they shouldn't access the
literals array. Can we drop all three calls here?

[mvstanton, 2016-05-24 16:02:55]

Patchset #5 (id:70054) has been deleted

[mvstanton, 2016-05-24 16:31:50]

Hi Michael,
Thanks for the great comments, have a second glance.
All the best,
--Mike

https://codereview.chromium.org/1906823002/diff/40001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/1906823002/diff/40001/src/compiler.cc#newcode524
src/compiler.cc:524:
shared->set_num_literals(lit->materialized_literal_count());
On 2016/05/10 13:47:20, Michael Starzinger wrote:
> Depending on who computes the literals count for a given FunctionLiteral, this
> should either be moved to "InitFromFunctionLiteral" (if computed by parser) or
> into "Renumber" (if computed by the numbering phase).

Happily, I could eliminate this piece completely (your refactoring a while ago
moved the work into the factory).

https://codereview.chromium.org/1906823002/diff/40001/src/compiler/access-bui...
File src/compiler/access-builder.cc (right):

https://codereview.chromium.org/1906823002/diff/40001/src/compiler/access-bui...
src/compiler/access-builder.cc:415: FieldAccess
AccessBuilder::ForJSFunctionLiterals() {
On 2016/05/10 13:47:20, Michael Starzinger wrote:
> nit: Please keep these in the same order as in the header (i.e. move the
> function to where it was before).

Done.

https://codereview.chromium.org/1906823002/diff/40001/src/compiler/access-bui...
File src/compiler/access-builder.h (right):

https://codereview.chromium.org/1906823002/diff/40001/src/compiler/access-bui...
src/compiler/access-builder.h:132: // Provides access to
JSFunction::literals()[0] field.
On 2016/05/10 13:47:20, Michael Starzinger wrote:
> Accessing an element as a field breaks invariants in TurboFan, this should be
> modeled using an ElementAccess. Also this function seems to be unused.

Done.

https://codereview.chromium.org/1906823002/diff/40001/src/compiler/js-generic...
File src/compiler/js-generic-lowering.cc (right):

https://codereview.chromium.org/1906823002/diff/40001/src/compiler/js-generic...
src/compiler/js-generic-lowering.cc:483: // functions that don't need literals
cloning.
On 2016/05/10 13:47:20, Michael Starzinger wrote:
> nit: Last part of the comment is outdated. Better just "Use the
> FastNewClusreStin only for functions allocated in new space" here.

Done.

https://codereview.chromium.org/1906823002/diff/40001/src/full-codegen/full-c...
File src/full-codegen/full-codegen.cc (right):

https://codereview.chromium.org/1906823002/diff/40001/src/full-codegen/full-c...
src/full-codegen/full-codegen.cc:999: // space for nested functions that don't
need literals cloning. If
On 2016/05/10 13:47:20, Michael Starzinger wrote:
> nit: Comment is outdated.

Done.

https://codereview.chromium.org/1906823002/diff/40001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1906823002/diff/40001/src/heap/heap.cc#newcod...
src/heap/heap.cc:2797: SKIP_WRITE_BARRIER);
On 2016/05/10 13:47:20, Michael Starzinger wrote:
> nit: I have a hard time reasoning about whether skipping this write barrier is
> actually safe. Could we instead just keep the write barrier here? This
> initialization is only used in the no-snapshot case anyways.

Done.

https://codereview.chromium.org/1906823002/diff/40001/src/heap/heap.cc#newcod...
src/heap/heap.cc:2797: SKIP_WRITE_BARRIER);
On 2016/05/10 13:47:20, Michael Starzinger wrote:
> nit: I have a hard time reasoning about whether skipping this write barrier is
> actually safe. Could we instead just keep the write barrier here? This
> initialization is only used in the no-snapshot case anyways.

Done.

https://codereview.chromium.org/1906823002/diff/40001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1906823002/diff/40001/src/objects.h#newcode4724
src/objects.h:4724: static const int kFeedbackVectorOffset =
FixedArray::kHeaderSize;
On 2016/05/10 13:47:20, Michael Starzinger wrote:
> nit: It is possible to use OffsetOfElementAt(kVectorIndex) to initialize this?

Done.

https://codereview.chromium.org/1906823002/diff/40001/src/objects.h#newcode4725
src/objects.h:4725: static const int kOffsetToFirstLiteral =
kFeedbackVectorOffset + kPointerSize;
On 2016/05/10 13:47:20, Michael Starzinger wrote:
> nit: Likewise.

Done.

https://codereview.chromium.org/1906823002/diff/40001/src/objects.h#newcode7409
src/objects.h:7409: static void EnsureLiterals(Handle<JSFunction> function);
On 2016/05/10 13:47:20, Michael Starzinger wrote:
> nit: Please move this down to the "[literals]" section, it doesn't affect the
> "[code]" field.

Done.

https://codereview.chromium.org/1906823002/diff/40001/src/objects.h#newcode7436
src/objects.h:7436: void ClearTypeFeedbackInfo();
On 2016/05/10 13:47:20, Michael Starzinger wrote:
> nit: Likewise, please move this down to after the "feedback_vector" accessor.

Done.

https://codereview.chromium.org/1906823002/diff/40001/test/cctest/compiler/fu...
File test/cctest/compiler/function-tester.h (right):

https://codereview.chromium.org/1906823002/diff/40001/test/cctest/compiler/fu...
test/cctest/compiler/function-tester.h:236:
JSFunction::EnsureLiterals(function);
On 2016/05/10 13:47:20, Michael Starzinger wrote:
> nit: This is only used to compile hand-crafted graphs, they shouldn't access
the
> literals array. Can we drop all three calls here?

Done.

[Michael Starzinger, 2016-05-25 11:20:17]

mstarzinger@chromium.org changed reviewers:
+ yangguo@chromium.org

[Michael Starzinger, 2016-05-25 11:20:18]

LGTM. Only one comment to address. And you will need serializer owner review, I
added Yang.

https://codereview.chromium.org/1906823002/diff/90001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/1906823002/diff/90001/src/compiler.cc#newcode688
src/compiler.cc:688: if (!Compiler::ParseAndAnalyze(info->parse_info())) return
false;
If the call to EnsureFeedbackMetadata is needed for GetOptimizedCodeNow above,
then we should also add it here. The GetOptimizedCodeLater and
GetOptimizedCodeNow helpers need to be kept in sync.

[mvstanton, 2016-05-25 12:57:06]

mvstanton@chromium.org changed reviewers:
+ bmeurer@chromium.org

[mvstanton, 2016-05-25 12:57:08]

Hi Ulan, Yang, Benedikt,

Could you guys review changes in /heap, /serializer, and /crankshaft?

Thanks!
--Mike

https://codereview.chromium.org/1906823002/diff/90001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/1906823002/diff/90001/src/compiler.cc#newcode688
src/compiler.cc:688: if (!Compiler::ParseAndAnalyze(info->parse_info())) return
false;
On 2016/05/25 11:20:18, Michael Starzinger wrote:
> If the call to EnsureFeedbackMetadata is needed for GetOptimizedCodeNow above,
> then we should also add it here. The GetOptimizedCodeLater and
> GetOptimizedCodeNow helpers need to be kept in sync.

Done.

[Benedikt Meurer, 2016-05-25 12:57:57]

lgtm

[mvstanton, 2016-05-25 12:59:26]

Description was changed from

==========
Move of the type feedback vector to the closure.

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

to

==========
Move of the type feedback vector to the closure.

We get less "pollution" of type feedback if we have one vector per native
context, rather than one for the whole system. This CL moves the vector
appropriately.

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

[Yang, 2016-05-25 13:07:34]

serializer and debug lgtm

[mvstanton, 2016-05-25 14:15:17]

The CQ bit was checked by mvstanton@chromium.org

[mvstanton, 2016-05-25 14:15:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1906823002/#ps110001
(title: "REBASE and comments.")

[commit-bot: I haz the power, 2016-05-25 14:15:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1906823002/110001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1906823002/110001

[commit-bot: I haz the power, 2016-05-25 14:15:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-25 14:15:26]

All required reviewers (with asterisk prefixes) have not yet approved this CL.

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-v8-committers".
Note that this has nothing to do with OWNERS files.

[mvstanton, 2016-05-27 06:42:25]

mvstanton@chromium.org changed reviewers:
- ulan@chromium.org

[mvstanton, 2016-05-27 06:42:25]

mvstanton@chromium.org changed required reviewers:
- ulan@chromium.org

[mvstanton, 2016-05-27 06:58:00]

The CQ bit was checked by mvstanton@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-27 06:58:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1906823002/130001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1906823002/130001

[commit-bot: I haz the power, 2016-05-27 07:41:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-27 07:41:58]

Dry run: This issue passed the CQ dry run.

[mvstanton, 2016-05-27 08:06:45]

The CQ bit was checked by mvstanton@chromium.org

[mvstanton, 2016-05-27 08:06:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, mstarzinger@chromium.org, yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/1906823002/#ps130001
(title: "REBASE.")

[commit-bot: I haz the power, 2016-05-27 08:06:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1906823002/130001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1906823002/130001

[commit-bot: I haz the power, 2016-05-27 08:09:22]

Description was changed from

==========
Move of the type feedback vector to the closure.

We get less "pollution" of type feedback if we have one vector per native
context, rather than one for the whole system. This CL moves the vector
appropriately.

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

to

==========
Move of the type feedback vector to the closure.

We get less "pollution" of type feedback if we have one vector per native
context, rather than one for the whole system. This CL moves the vector
appropriately.

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

[commit-bot: I haz the power, 2016-05-27 08:09:24]

Committed patchset #7 (id:130001)

[commit-bot: I haz the power, 2016-05-27 08:11:05]

Description was changed from

==========
Move of the type feedback vector to the closure.

We get less "pollution" of type feedback if we have one vector per native
context, rather than one for the whole system. This CL moves the vector
appropriately.

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

to

==========
Move of the type feedback vector to the closure.

We get less "pollution" of type feedback if we have one vector per native
context, rather than one for the whole system. This CL moves the vector
appropriately.

BUG=

Committed: https://crrev.com/91c88644dcf396285f88977080ea85f4614bbc02
Cr-Commit-Position: refs/heads/master@{#36539}
==========

[commit-bot: I haz the power, 2016-05-27 08:11:06]

Patchset 7 (id:??) landed as
https://crrev.com/91c88644dcf396285f88977080ea85f4614bbc02
Cr-Commit-Position: refs/heads/master@{#36539}