Store OSR'd optimized code on the native context.

Store OSR'd optimized code on the native context.

Since we OSR code rarely, it makes sense to store it and look for it on the native context rather than the SharedFunctionInfo. This makes the OptimizedCodeMap data structure more space efficient, as it doesn't have to store an ast ID for the OSR entry point.

BUG=

Committed: https://crrev.com/378b6b22fb7925ac5b672335a54599f5739e7758
Cr-Commit-Position: refs/heads/master@{#41554}

[mvstanton, 2016-12-03 12:13:41]

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

[commit-bot: I haz the power, 2016-12-03 12:13:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-03 12:29:20]

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

[commit-bot: I haz the power, 2016-12-03 12:29:20]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[mvstanton, 2016-12-05 13:38:16]

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

[commit-bot: I haz the power, 2016-12-05 13:38:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mvstanton, 2016-12-05 13:41:59]

Description was changed from

==========
Store OSR'd optimized code on the native context.

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

to

==========
Store OSR'd optimized code on the native context.

Since we OSR code rarely, it makes sense to store it and look for it on the
native context rather than the SharedFunctionInfo. This makes the
OptimizedCodeMap data structure more space efficient, as it doesn't have to
store an ast ID for the OSR entry point.

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

[mvstanton, 2016-12-05 13:41:59]

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

[mvstanton, 2016-12-05 13:41:59]

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

[mvstanton, 2016-12-05 13:42:42]

Hi Michi, hi Ulan,
Can you guys have a look at this fellow?
Thanks!
--Michael

[mvstanton, 2016-12-05 22:01:36]

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

[mvstanton, 2016-12-05 22:01:36]

Hi guys,
I addressed an issue where I could leave optimized code laying around in a debug
scenario. 

I apologize for some terminology confusion. The long term goal (post-next week,
after a move of LiteralsArray storage location) is to remove the
OptimizedCodeMap. I'm calling this structure the osr function table. But for
ease of review, I kept function names the same when I was creating analogue
functions in the Context. I'd propose finishing the renaming job in a follow-on
CL, but I can do it now too if you like...

Ulan, I made you a non-required reviewer, thanks to the land of a flag removal
that means I'm not touching heap/object-visiting-inl.h anymore.

Okay thanks, sorry for the churn after you may have started commenting. Much
appreciated...
--Michael

[mvstanton, 2016-12-05 22:01:52]

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

[commit-bot: I haz the power, 2016-12-05 22:01:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-05 22:58:51]

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

[commit-bot: I haz the power, 2016-12-05 22:58:52]

Dry run: This issue passed the CQ dry run.

[Michael Starzinger, 2016-12-07 09:26:32]

LGTM.

Not entierly happy with the duplicated code from objects.cc in contexts.cc. Also
I didn't fully think through all of it (assuming it is mostly a copy from
objects.cc).

I am fine with renaming functions in a follow-up CL. But lets get the names of
the new fields and new enum values right, there is no need to postpone that to a
subsequent CL.

https://codereview.chromium.org/2549753002/diff/120001/src/contexts.cc
File src/contexts.cc (right):

https://codereview.chromium.org/2549753002/diff/120001/src/contexts.cc#newcod...
src/contexts.cc:504: // TODO(mstarzinger): Temporary workaround. The allocation
above might
This should no longer happen now that flushing has been removed from the GC. Can
we get by with not copying this workaround over?

https://codereview.chromium.org/2549753002/diff/120001/src/contexts.h
File src/contexts.h (right):

https://codereview.chromium.org/2549753002/diff/120001/src/contexts.h#newcode285
src/contexts.h:285: V(OSRD_OPTIMIZED_TABLE_INDEX, FixedArray,
osrd_function_table)               \
nit: The table stores code, not functions. How would you feel about naming this
"osr_code_table" instead? Also, please keep the enum and the accessor name in
sync.

[Michael Starzinger, 2016-12-07 11:08:33]

One other high-level thought: Currently we clear the entire optimized code map
in {CodeFlusher::ProcessSharedFunctionInfoCandidates} when we remove the
unoptimized code for a {SharedFunctionInfo} that ensures we don't accidentally
pick up optimized code for which the deoptimization target has disappeared.
Could it happen that optimized OSR code is kept in the per-native-context table
even though the deoptimization target has disappeared? It might very well be
that the code referenced from the {deoptimization_data} prevents that. I just
wanted to point it out so that another brain thinks through this case as well.

[mvstanton, 2016-12-07 14:41:02]

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

[mvstanton, 2016-12-07 14:41:07]

The CQ bit was checked by mvstanton@chromium.org

[mvstanton, 2016-12-07 14:41:08]

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/2549753002/#ps140001
(title: "Comment response.")

[commit-bot: I haz the power, 2016-12-07 14:41:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mvstanton, 2016-12-07 14:42:32]

Thanks! The code duplication should diminish as the OptimizedCodeMap gets
smaller and (soon) disappears.

I'll follow up soon, as we discussed, with a CL to stop clearing the map at code
flushing time.
--Michael

https://codereview.chromium.org/2549753002/diff/120001/src/contexts.cc
File src/contexts.cc (right):

https://codereview.chromium.org/2549753002/diff/120001/src/contexts.cc#newcod...
src/contexts.cc:504: // TODO(mstarzinger): Temporary workaround. The allocation
above might
On 2016/12/07 09:26:31, Michael Starzinger wrote:
> This should no longer happen now that flushing has been removed from the GC.
Can
> we get by with not copying this workaround over?

Done.

https://codereview.chromium.org/2549753002/diff/120001/src/contexts.h
File src/contexts.h (right):

https://codereview.chromium.org/2549753002/diff/120001/src/contexts.h#newcode285
src/contexts.h:285: V(OSRD_OPTIMIZED_TABLE_INDEX, FixedArray,
osrd_function_table)               \
On 2016/12/07 09:26:31, Michael Starzinger wrote:
> nit: The table stores code, not functions. How would you feel about naming
this
> "osr_code_table" instead? Also, please keep the enum and the accessor name in
> sync.

Good idea, thanks.

[ulan, 2016-12-07 14:54:57]

https://codereview.chromium.org/2549753002/diff/120001/src/contexts.cc
File src/contexts.cc (right):

https://codereview.chromium.org/2549753002/diff/120001/src/contexts.cc#newcod...
src/contexts.cc:481: Handle<WeakCell> code_cell =
isolate->factory()->NewWeakCell(code);
Since the code is optimized, Code::WeakCellFor(code) will return cached weak
cell, so we can save one allocation here.

https://codereview.chromium.org/2549753002/diff/120001/src/contexts.cc#newcod...
src/contexts.cc:514: Handle<WeakCell> code_cell =
isolate->factory()->NewWeakCell(code);
Code::WeakCellFor(code)

https://codereview.chromium.org/2549753002/diff/120001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2549753002/diff/120001/src/objects.cc#newcode...
src/objects.cc:12480: found = WeakCell::cast(code_map->get(src +
kCachedCodeOffset))->value() ==
I think we need two flags here: found and match.

match = WeakCell::cast...;

found = found || match

if (match) {
}

Otherwise, we can set found to back to false if the next entry is not our code
object.

[commit-bot: I haz the power, 2016-12-07 15:10:47]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1481121667705520,
"parent_rev": "efbff9fb3921b598ad4e0a62e2d8e1eaf80ea1c1", "commit_rev":
"622c673414c77c3eada1815ad86ba22f3f7865c3"}

[commit-bot: I haz the power, 2016-12-07 15:10:52]

Description was changed from

==========
Store OSR'd optimized code on the native context.

Since we OSR code rarely, it makes sense to store it and look for it on the
native context rather than the SharedFunctionInfo. This makes the
OptimizedCodeMap data structure more space efficient, as it doesn't have to
store an ast ID for the OSR entry point.

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

to

==========
Store OSR'd optimized code on the native context.

Since we OSR code rarely, it makes sense to store it and look for it on the
native context rather than the SharedFunctionInfo. This makes the
OptimizedCodeMap data structure more space efficient, as it doesn't have to
store an ast ID for the OSR entry point.

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

[commit-bot: I haz the power, 2016-12-07 15:10:54]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-12-07 15:11:15]

Description was changed from

==========
Store OSR'd optimized code on the native context.

Since we OSR code rarely, it makes sense to store it and look for it on the
native context rather than the SharedFunctionInfo. This makes the
OptimizedCodeMap data structure more space efficient, as it doesn't have to
store an ast ID for the OSR entry point.

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

to

==========
Store OSR'd optimized code on the native context.

Since we OSR code rarely, it makes sense to store it and look for it on the
native context rather than the SharedFunctionInfo. This makes the
OptimizedCodeMap data structure more space efficient, as it doesn't have to
store an ast ID for the OSR entry point.

BUG=

Committed: https://crrev.com/378b6b22fb7925ac5b672335a54599f5739e7758
Cr-Commit-Position: refs/heads/master@{#41554}
==========

[commit-bot: I haz the power, 2016-12-07 15:11:16]

Patchset 8 (id:??) landed as
https://crrev.com/378b6b22fb7925ac5b672335a54599f5739e7758
Cr-Commit-Position: refs/heads/master@{#41554}

[Benedikt Meurer, 2016-12-08 06:12:51]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/2561633003/ by bmeurer@chromium.org.

The reason for reverting is: Speculative revert WebGL breakage reported in
https://bugs.chromium.org/p/chromium/issues/detail?id=672367.