Share literals arrays per <NativeContext, SharedFunctionInfo> pair.

Share literals arrays per <NativeContext, SharedFunctionInfo> pair.

This CL also renames wrongly named test for v8:4173.

BUG=v8:4121
LOG=Y

Committed: https://crrev.com/4dd45e115be0122ee8b6ba1d9b3be5e86ea3b97c
Cr-Commit-Position: refs/heads/master@{#30879}

[Igor Sheludko, 2015-09-21 10:25:19]

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

[Igor Sheludko, 2015-09-21 10:25:20]

PTAL

[Michael Starzinger, 2015-09-22 10:44:40]

I like this. First round of comments. I still need to think through the GC
support more thoroughly.

https://codereview.chromium.org/1353363002/diff/60001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/1353363002/diff/60001/src/code-stubs-hydrogen...
src/code-stubs-hydrogen.cc:1679: // and the literals
nit: This comment doesn't provide any value anymore, let's drop it.

https://codereview.chromium.org/1353363002/diff/60001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/1353363002/diff/60001/src/factory.cc#newcode1325
src/factory.cc:1325: } else if (cached.literals == nullptr && !info->bound()) {
nit: The way the condition is written, there is no need for the "else-if", I
think we should just make this a normal "if" to increase readability.

https://codereview.chromium.org/1353363002/diff/60001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1353363002/diff/60001/src/objects.h#newcode6293
src/objects.h:6293: int SearchOptimizedCodeMapEntry(Context* native_context,
I think we should avoid exposing this function providing a raw index into the
map. We had such an interface before and it turned out to be a nightmare (which
is why I added the CodeAndLiterals struct). Can we make this function local to
objects.cc by putting it into an anonymous namespace there?

[Michael Starzinger, 2015-09-22 10:57:34]

https://codereview.chromium.org/1353363002/diff/60001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1353363002/diff/60001/src/objects.cc#newcode1...
src/objects.cc:11309: if (FLAG_trace_opt) {
This tracing will also be printed when adding new entries to the optimized code
map. This will turn out to be misleading.

[Igor Sheludko, 2015-09-22 12:53:50]

Patchset #5 (id:70001) has been deleted

[Igor Sheludko, 2015-09-22 12:58:11]

Addressed online and offline comments. 
Now undefined value is used to indicate non-existing code in the optimized code
map entry instead of smi-zero.

https://codereview.chromium.org/1353363002/diff/60001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/1353363002/diff/60001/src/code-stubs-hydrogen...
src/code-stubs-hydrogen.cc:1679: // and the literals
On 2015/09/22 at 10:44:39, Michael Starzinger wrote:
> nit: This comment doesn't provide any value anymore, let's drop it.

Done.

https://codereview.chromium.org/1353363002/diff/60001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/1353363002/diff/60001/src/factory.cc#newcode1325
src/factory.cc:1325: } else if (cached.literals == nullptr && !info->bound()) {
On 2015/09/22 at 10:44:39, Michael Starzinger wrote:
> nit: The way the condition is written, there is no need for the "else-if", I
think we should just make this a normal "if" to increase readability.

Indeed, the "cached.literals == nullptr &&" part is no longer necessary.

https://codereview.chromium.org/1353363002/diff/60001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1353363002/diff/60001/src/objects.cc#newcode1...
src/objects.cc:11309: if (FLAG_trace_opt) {
On 2015/09/22 at 10:57:34, Michael Starzinger wrote:
> This tracing will also be printed when adding new entries to the optimized
code map. This will turn out to be misleading.

Fixed.

https://codereview.chromium.org/1353363002/diff/60001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1353363002/diff/60001/src/objects.h#newcode6293
src/objects.h:6293: int SearchOptimizedCodeMapEntry(Context* native_context,
On 2015/09/22 at 10:44:39, Michael Starzinger wrote:
> I think we should avoid exposing this function providing a raw index into the
map. We had such an interface before and it turned out to be a nightmare (which
is why I added the CodeAndLiterals struct). Can we make this function local to
objects.cc by putting it into an anonymous namespace there?

The implementation uses a lot of internal SFI stuff. What about just moving the
method to private part?

[Michael Starzinger, 2015-09-23 08:14:22]

LGTM.

https://codereview.chromium.org/1353363002/diff/60001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1353363002/diff/60001/src/objects.h#newcode6293
src/objects.h:6293: int SearchOptimizedCodeMapEntry(Context* native_context,
On 2015/09/22 12:58:11, Igor Sheludko wrote:
> On 2015/09/22 at 10:44:39, Michael Starzinger wrote:
> > I think we should avoid exposing this function providing a raw index into
the
> map. We had such an interface before and it turned out to be a nightmare
(which
> is why I added the CodeAndLiterals struct). Can we make this function local to
> objects.cc by putting it into an anonymous namespace there?
> 
> The implementation uses a lot of internal SFI stuff. What about just moving
the
> method to private part?

Acknowledged. Yep, works for me.

[Igor Sheludko, 2015-09-23 08:21:41]

The CQ bit was checked by ishell@chromium.org

[commit-bot: I haz the power, 2015-09-23 08:21:53]

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

[commit-bot: I haz the power, 2015-09-23 08:46:16]

Committed patchset #5 (id:90001)

[commit-bot: I haz the power, 2015-09-23 08:46:40]

Patchset 5 (id:??) landed as
https://crrev.com/4dd45e115be0122ee8b6ba1d9b3be5e86ea3b97c
Cr-Commit-Position: refs/heads/master@{#30879}