[Interpreter] Add BytecodeArray class and add to SharedFunctionInfo.

[Interpreter] Add BytecodeArray class and add to SharedFunctionInfo.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/cb6e7056862ee6991d3d884a92d36bebe45b8152
Cr-Commit-Position: refs/heads/master@{#29843}

[rmcilroy, 2015-07-15 13:33:36]

rmcilroy@chromium.org changed reviewers:
+ rmcilroy@chromium.org

[rmcilroy, 2015-07-15 13:33:38]

Looks good overall. Lots of mostly nitty comments.

https://codereview.chromium.org/1230753004/diff/40001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/1230753004/diff/40001/src/factory.cc#newcode880
src/factory.cc:880: length, start, pretenure),
strange indentation here - did you run "git cl format"?

https://codereview.chromium.org/1230753004/diff/40001/src/factory.h
File src/factory.h (right):

https://codereview.chromium.org/1230753004/diff/40001/src/factory.h#newcode286
src/factory.h:286: Handle<BytecodeArray> NewBytecodeArray(int length, const
byte* start,
nit - /s/start/raw_bytecodes.

https://codereview.chromium.org/1230753004/diff/40001/src/factory.h#newcode287
src/factory.h:287: PretenureFlag pretenure = NOT_TENURED);
Let's remove the pretenure flag and just always tenure the bytecode - it is
referenced by the SharedFunctionInfo which is also pretenured (allocated in the
old space) and should always live long enough that it would always get promoted
anyway.

https://codereview.chromium.org/1230753004/diff/40001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/1230753004/diff/40001/src/flag-definitions.h#...
src/flag-definitions.h:280: DEFINE_BOOL(ignition, false, "use ignition
interpreter")
I'll change this in my CL so it should disappear once you've rebased.

https://codereview.chromium.org/1230753004/diff/40001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1230753004/diff/40001/src/heap/heap.h#newcode...
src/heap/heap.h:1675: // Allocates a bytecode array with given contents
nit - fullstop at the end of the comment.

https://codereview.chromium.org/1230753004/diff/40001/src/interpreter/bytecod...
File src/interpreter/bytecode-emitter.cc (right):

https://codereview.chromium.org/1230753004/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-emitter.cc:28: void BytecodeEmitter::VisitBlock(Block*
node) {}
Could you please add UNIMPLEMENTED(); to each of these blocks so that we know
when we hit one of the unimplemented AST blocks.

https://codereview.chromium.org/1230753004/diff/40001/src/interpreter/bytecod...
File src/interpreter/bytecode-emitter.h (right):

https://codereview.chromium.org/1230753004/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-emitter.h:15: class BytecodeEmitter : public AstVisitor
{
Let's move this off to a separate CL and keep this one just for adding the
BytecodeArray object.

https://codereview.chromium.org/1230753004/diff/40001/src/interpreter/interpr...
File src/interpreter/interpreter.h (right):

https://codereview.chromium.org/1230753004/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.h:66: #endif  // V8_INTERPRETER_INTERPRETER_H_
good catch, I'll fix this, thanks :).

https://codereview.chromium.org/1230753004/diff/40001/src/objects-printer.cc
File src/objects-printer.cc (right):

https://codereview.chromium.org/1230753004/diff/40001/src/objects-printer.cc#...
src/objects-printer.cc:213: << static_cast<interpreter::Bytecode>(bc);
How about we move this to a BytecodeArray::Dissassemble() function so it can be
used from elsewhere if necessary.

https://codereview.chromium.org/1230753004/diff/40001/src/objects-printer.cc#...
src/objects-printer.cc:783: // TODO(oth): implement real bytecode printer
Actually I think emitting the pointer here is fine for the ShareFunctionInfo
object. I would remove the TODO. (someone can always call BytecodeArrayPrint on
the pointer to get the bytecode pretty-printed.

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

https://codereview.chromium.org/1230753004/diff/40001/src/objects.h#newcode4267
src/objects.h:4267: // BytecodeArray represents a sequence interpreter
bytecodes.
/s/sequence interpreter/sequence of interpreter

https://codereview.chromium.org/1230753004/diff/40001/src/objects.h#newcode4277
src/objects.h:4277: // have.
Copy and pasted ByteArray comment here?

https://codereview.chromium.org/1230753004/diff/40001/src/objects.h#newcode4278
src/objects.h:4278: static int LengthFor(int size_in_bytes) {
Do we need this function? I think it's only necessary for ByteArray not
BytecodeArray

https://codereview.chromium.org/1230753004/diff/40001/src/objects.h#newcode4285
src/objects.h:4285: inline Address GetDataStartAddress() {
nit - GetFirstBytecodeAddress() ?

https://codereview.chromium.org/1230753004/diff/40001/src/objects.h#newcode4290
src/objects.h:4290: static inline BytecodeArray* FromDataStartAddress(Address
address);
Looks like this isn't used, let's drop it until it's necessary.

https://codereview.chromium.org/1230753004/diff/40001/src/objects.h#newcode4295
src/objects.h:4295: inline int BytecodeArraySize() { return
SizeFor(this->length()); }
Same for this (also remove comment).

https://codereview.chromium.org/1230753004/diff/40001/src/objects.h#newcode4303
src/objects.h:4303: static const int kMaxSize = 512 * MB;
nit - /s/512 * MB/ByteArray::kMaxSize/

https://codereview.chromium.org/1230753004/diff/40001/src/objects.h#newcode6718
src/objects.h:6718: // [bytecode_array]: Interpreter byte codes
nit - /s/byte codes/bytecodes ?

[oth, 2015-07-16 09:15:50]

Thanks! All done/ack'ed.

https://codereview.chromium.org/1230753004/diff/40001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/1230753004/diff/40001/src/factory.cc#newcode880
src/factory.cc:880: length, start, pretenure),
On 2015/07/15 13:33:37, rmcilroy wrote:
> strange indentation here - did you run "git cl format"?

Done. git cl bogon.

https://codereview.chromium.org/1230753004/diff/40001/src/factory.h
File src/factory.h (right):

https://codereview.chromium.org/1230753004/diff/40001/src/factory.h#newcode286
src/factory.h:286: Handle<BytecodeArray> NewBytecodeArray(int length, const
byte* start,
On 2015/07/15 13:33:37, rmcilroy wrote:
> nit - /s/start/raw_bytecodes.

Done.

https://codereview.chromium.org/1230753004/diff/40001/src/factory.h#newcode287
src/factory.h:287: PretenureFlag pretenure = NOT_TENURED);
On 2015/07/15 13:33:37, rmcilroy wrote:
> Let's remove the pretenure flag and just always tenure the bytecode - it is
> referenced by the SharedFunctionInfo which is also pretenured (allocated in
the
> old space) and should always live long enough that it would always get
promoted
> anyway.

Done.

https://codereview.chromium.org/1230753004/diff/40001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/1230753004/diff/40001/src/flag-definitions.h#...
src/flag-definitions.h:280: DEFINE_BOOL(ignition, false, "use ignition
interpreter")
On 2015/07/15 13:33:37, rmcilroy wrote:
> I'll change this in my CL so it should disappear once you've rebased.

Acknowledged.

https://codereview.chromium.org/1230753004/diff/40001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1230753004/diff/40001/src/heap/heap.h#newcode...
src/heap/heap.h:1675: // Allocates a bytecode array with given contents
On 2015/07/15 13:33:38, rmcilroy wrote:
> nit - fullstop at the end of the comment.

Done and renamed 'start' to 'raw_bytecodes'.

https://codereview.chromium.org/1230753004/diff/40001/src/interpreter/bytecod...
File src/interpreter/bytecode-emitter.cc (right):

https://codereview.chromium.org/1230753004/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-emitter.cc:28: void BytecodeEmitter::VisitBlock(Block*
node) {}
On 2015/07/15 13:33:38, rmcilroy wrote:
> Could you please add UNIMPLEMENTED(); to each of these blocks so that we know
> when we hit one of the unimplemented AST blocks.

Done.

https://codereview.chromium.org/1230753004/diff/40001/src/interpreter/bytecod...
File src/interpreter/bytecode-emitter.h (right):

https://codereview.chromium.org/1230753004/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-emitter.h:15: class BytecodeEmitter : public AstVisitor
{
On 2015/07/15 13:33:38, rmcilroy wrote:
> Let's move this off to a separate CL and keep this one just for adding the
> BytecodeArray object.

Acknowledged.

https://codereview.chromium.org/1230753004/diff/40001/src/interpreter/interpr...
File src/interpreter/interpreter.h (right):

https://codereview.chromium.org/1230753004/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.h:66: #endif  // V8_INTERPRETER_INTERPRETER_H_
On 2015/07/15 13:33:38, rmcilroy wrote:
> good catch, I'll fix this, thanks :).

Acknowledged.

https://codereview.chromium.org/1230753004/diff/40001/src/objects-printer.cc
File src/objects-printer.cc (right):

https://codereview.chromium.org/1230753004/diff/40001/src/objects-printer.cc#...
src/objects-printer.cc:213: << static_cast<interpreter::Bytecode>(bc);
On 2015/07/15 13:33:38, rmcilroy wrote:
> How about we move this to a BytecodeArray::Dissassemble() function so it can
be
> used from elsewhere if necessary.

Done.

https://codereview.chromium.org/1230753004/diff/40001/src/objects-printer.cc#...
src/objects-printer.cc:783: // TODO(oth): implement real bytecode printer
On 2015/07/15 13:33:38, rmcilroy wrote:
> Actually I think emitting the pointer here is fine for the ShareFunctionInfo
> object. I would remove the TODO. (someone can always call BytecodeArrayPrint
on
> the pointer to get the bytecode pretty-printed.

Done.

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

https://codereview.chromium.org/1230753004/diff/40001/src/objects.h#newcode4267
src/objects.h:4267: // BytecodeArray represents a sequence interpreter
bytecodes.
On 2015/07/15 13:33:38, rmcilroy wrote:
> /s/sequence interpreter/sequence of interpreter

Done.

https://codereview.chromium.org/1230753004/diff/40001/src/objects.h#newcode4277
src/objects.h:4277: // have.
On 2015/07/15 13:33:38, rmcilroy wrote:
> Copy and pasted ByteArray comment here?

Done.

https://codereview.chromium.org/1230753004/diff/40001/src/objects.h#newcode4278
src/objects.h:4278: static int LengthFor(int size_in_bytes) {
On 2015/07/15 13:33:38, rmcilroy wrote:
> Do we need this function? I think it's only necessary for ByteArray not
> BytecodeArray

Done.

https://codereview.chromium.org/1230753004/diff/40001/src/objects.h#newcode4285
src/objects.h:4285: inline Address GetDataStartAddress() {
On 2015/07/15 13:33:38, rmcilroy wrote:
> nit - GetFirstBytecodeAddress() ?

Done.

https://codereview.chromium.org/1230753004/diff/40001/src/objects.h#newcode4290
src/objects.h:4290: static inline BytecodeArray* FromDataStartAddress(Address
address);
On 2015/07/15 13:33:38, rmcilroy wrote:
> Looks like this isn't used, let's drop it until it's necessary.

Done.

https://codereview.chromium.org/1230753004/diff/40001/src/objects.h#newcode4295
src/objects.h:4295: inline int BytecodeArraySize() { return
SizeFor(this->length()); }
On 2015/07/15 13:33:38, rmcilroy wrote:
> Same for this (also remove comment).

Done.

https://codereview.chromium.org/1230753004/diff/40001/src/objects.h#newcode4303
src/objects.h:4303: static const int kMaxSize = 512 * MB;
On 2015/07/15 13:33:38, rmcilroy wrote:
> nit - /s/512 * MB/ByteArray::kMaxSize/

Done.

https://codereview.chromium.org/1230753004/diff/40001/src/objects.h#newcode6718
src/objects.h:6718: // [bytecode_array]: Interpreter byte codes
On 2015/07/15 13:33:38, rmcilroy wrote:
> nit - /s/byte codes/bytecodes ?

Done.

[rmcilroy, 2015-07-16 10:29:45]

Looks pretty good to me. A couple of comments.

https://codereview.chromium.org/1230753004/diff/120001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1230753004/diff/120001/src/heap/heap.h#newcod...
src/heap/heap.h:1684: PretenureFlag pretenure = NOT_TENURED);
Since this should always be TENURED let's remove the PretenureFlag from here too
and just always allocate in the OLD space in the implementation.

https://codereview.chromium.org/1230753004/diff/120001/src/interpreter/interp...
File src/interpreter/interpreter.cc (left):

https://codereview.chromium.org/1230753004/diff/120001/src/interpreter/interp...
src/interpreter/interpreter.cc:47: // argument.
Did you mean to drop this comment?

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

https://codereview.chromium.org/1230753004/diff/120001/src/objects.cc#newcode...
src/objects.cc:11803: << static_cast<interpreter::Bytecode>(bc) << "\n";
We will need to do something more complex than this (probably in Bytecodes.cc)
to deal with bytecode arguments and such, but this is probably fine for now.

[oth, 2015-07-16 13:14:39]

Thanks, incorporated changes.

https://codereview.chromium.org/1230753004/diff/120001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1230753004/diff/120001/src/heap/heap.h#newcod...
src/heap/heap.h:1684: PretenureFlag pretenure = NOT_TENURED);
On 2015/07/16 10:29:44, rmcilroy wrote:
> Since this should always be TENURED let's remove the PretenureFlag from here
too
> and just always allocate in the OLD space in the implementation.

Done. Now allocated directly in the OLD_SPACE in heap.cc.

https://codereview.chromium.org/1230753004/diff/120001/src/interpreter/interp...
File src/interpreter/interpreter.cc (left):

https://codereview.chromium.org/1230753004/diff/120001/src/interpreter/interp...
src/interpreter/interpreter.cc:47: // argument.
On 2015/07/16 10:29:44, rmcilroy wrote:
> Did you mean to drop this comment?

Nope. Fixed.

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

https://codereview.chromium.org/1230753004/diff/120001/src/objects.cc#newcode...
src/objects.cc:11803: << static_cast<interpreter::Bytecode>(bc) << "\n";
On 2015/07/16 10:29:44, rmcilroy wrote:
> We will need to do something more complex than this (probably in Bytecodes.cc)
> to deal with bytecode arguments and such, but this is probably fine for now.

Ack. Sync'ed with current Bytecode representation though haven't moved code.

[rmcilroy, 2015-07-20 11:39:45]

rmcilroy@chromium.org changed reviewers:
+ ulan@chromium.org

[rmcilroy, 2015-07-20 11:39:46]

Generally looks good to me with a few more comments.

Ulan: could you take a look and let us know if we've missed anything that needs
to be added for a new object type?

https://codereview.chromium.org/1230753004/diff/200001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1230753004/diff/200001/src/heap/heap.h#newcod...
src/heap/heap.h:1685: 
nit - drop extra newline

https://codereview.chromium.org/1230753004/diff/200001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/1230753004/diff/200001/src/objects-inl.h#newc...
src/objects-inl.h:3657: return READ_INT_FIELD(this, kFrameSizeOffset);
These should be stored as SMI values. Could you just use the SMI_ACCESSORS
macro?

https://codereview.chromium.org/1230753004/diff/200001/src/objects-printer.cc
File src/objects-printer.cc (right):

https://codereview.chromium.org/1230753004/diff/200001/src/objects-printer.cc...
src/objects-printer.cc:206: os << buf.start();
nit - do you need to change this function?

https://codereview.chromium.org/1230753004/diff/200001/src/objects-printer.cc...
src/objects-printer.cc:214: os << buf.start();
I don't think these lines is necessary - it should be clear from Dissasemble
which prints the addresses of the bytecodes as it prints them.

https://codereview.chromium.org/1230753004/diff/200001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1230753004/diff/200001/src/objects.cc#newcode...
src/objects.cc:11801: int bytecode_size = 0;
Could you print out frame size / locals count here too please. Also start with
some header showing this is a Bytecode array being printed.

https://codereview.chromium.org/1230753004/diff/200001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1230753004/diff/200001/src/objects.h#newcode4277
src/objects.h:4277: // Accessors for frame size and offsets
/s/offsets/locals count./

[oth, 2015-07-20 13:47:31]

Thanks!

https://codereview.chromium.org/1230753004/diff/200001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1230753004/diff/200001/src/heap/heap.h#newcod...
src/heap/heap.h:1685: 
On 2015/07/20 11:39:46, rmcilroy wrote:
> nit - drop extra newline

Done.

https://codereview.chromium.org/1230753004/diff/200001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/1230753004/diff/200001/src/objects-inl.h#newc...
src/objects-inl.h:3657: return READ_INT_FIELD(this, kFrameSizeOffset);
On 2015/07/20 11:39:46, rmcilroy wrote:
> These should be stored as SMI values. Could you just use the SMI_ACCESSORS
> macro?

Done.

https://codereview.chromium.org/1230753004/diff/200001/src/objects-printer.cc
File src/objects-printer.cc (right):

https://codereview.chromium.org/1230753004/diff/200001/src/objects-printer.cc...
src/objects-printer.cc:206: os << buf.start();
On 2015/07/20 11:39:46, rmcilroy wrote:
> nit - do you need to change this function?

Done.

https://codereview.chromium.org/1230753004/diff/200001/src/objects-printer.cc...
src/objects-printer.cc:214: os << buf.start();
On 2015/07/20 11:39:46, rmcilroy wrote:
> I don't think these lines is necessary - it should be clear from Dissasemble
> which prints the addresses of the bytecodes as it prints them.

Done.

https://codereview.chromium.org/1230753004/diff/200001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1230753004/diff/200001/src/objects.cc#newcode...
src/objects.cc:11801: int bytecode_size = 0;
On 2015/07/20 11:39:46, rmcilroy wrote:
> Could you print out frame size / locals count here too please. Also start with
> some header showing this is a Bytecode array being printed.

Done.

https://codereview.chromium.org/1230753004/diff/200001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1230753004/diff/200001/src/objects.h#newcode4277
src/objects.h:4277: // Accessors for frame size and offsets
On 2015/07/20 11:39:46, rmcilroy wrote:
> /s/offsets/locals count./

Done.

[rmcilroy, 2015-07-21 11:25:16]

lgtm other than the dissasembly (which I think should be in a different CL).

Ulan: Could you take a look please?

https://codereview.chromium.org/1230753004/diff/220001/src/interpreter/byteco...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1230753004/diff/220001/src/interpreter/byteco...
src/interpreter/bytecodes.h:50: static const int kMaximumSize = 1 +
kMaximumNumberOfArguments;
Is this just used for the disassembly? I don't think we should have this here
just for that, isn't there another way of formatting the disassembly to avoid
having to hard-code the maximum number of bytecode arguments?

https://codereview.chromium.org/1230753004/diff/220001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1230753004/diff/220001/src/objects.cc#newcode...
src/objects.cc:11817: os << bytecode;
What does this print out, it's hard to see from the code. I was hoping for
something like:

  0x4e327c60     0  0001          LoadLiteral0 [1]
  0x4e327c62     2  01            Return

(like the code disassembly)

Would it be possible to do something like this? If you like we can just put a
TODO in here and do the actual disassembly as a separate CL.

[ulan, 2015-07-21 11:55:24]

Everything looks good, except:

1) Memory overhead (see comment below).
2) Please add a test that creates bytecode array, checks, and does GC.

https://codereview.chromium.org/1230753004/diff/220001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1230753004/diff/220001/src/objects.h#newcode6710
src/objects.h:6710: DECL_ACCESSORS(bytecode_array, BytecodeArray)
This will increase memory footprint. Couple of month ago I saw that google inbox
had ~50K shared function infos. So this would mean additional 200K-400K live
memory depending on architecture.

Maybe we can find a way to stash the pointer somewhere, so that we pay the cost
only when there is an actual byte array? Or do this behind build flag.

[rmcilroy, 2015-07-21 12:12:55]

On 2015/07/21 11:55:24, ulan wrote:
> Everything looks good, except:
> 
> 1) Memory overhead (see comment below).
> 2) Please add a test that creates bytecode array, checks, and does GC.
> 
> https://codereview.chromium.org/1230753004/diff/220001/src/objects.h
> File src/objects.h (right):
> 
>
https://codereview.chromium.org/1230753004/diff/220001/src/objects.h#newcode6710
> src/objects.h:6710: DECL_ACCESSORS(bytecode_array, BytecodeArray)
> This will increase memory footprint. Couple of month ago I saw that google
inbox
> had ~50K shared function infos. So this would mean additional 200K-400K live
> memory depending on architecture.
> 
> Maybe we can find a way to stash the pointer somewhere, so that we pay the
cost
> only when there is an actual byte array? Or do this behind build flag.

Yeah this was a valid concern. I would prefer not to put this behind a build
time flag because that would mean we couldn't run the interpreter tests on the
waterfall (unless we set up a separate Bot, but then the tests could probably
only run on a single arch). 

Stashing in a spare field sounds ideal if possible. I'll have a look, but do you
have any candidates?

[ulan, 2015-07-21 12:26:14]

> Stashing in a spare field sounds ideal if possible. I'll have a look, but do
you
> have any candidates?

Logical way would be to share the "code" field and make it (code | bytecode)
field, which points
1) to code if there is no bytecode
2) to bytecode if there is no code
3) to pair (code, bytecode) if both are present. (do you expect this to be
common case?)
That probably requires some work. Not sure if it can regress perf.

Alternative is to find another field that is rarely used and share it with
bytecode, e.g. inferred_name, debug_info, counters. This sharing would be
somewhat arbitrary and hacky.

[Michael Starzinger, 2015-07-21 12:51:03]

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

[Michael Starzinger, 2015-07-21 12:51:04]

Drive-by ...

How about we use the SharedFunctionInfo::function_data for that overloading?
This field is currently used for two purposes ...

1) For API function which are backed by C++ implementation. I don't think we
will ever need bytecode for them. :)

2) For number special builtin functions. For now we would just exclude these
functions from being interpreted. See the FUNCTIONS_WITH_ID_LIST list in the
objects.h file for a list of what we are talking about here.

The above two cases are clearly distinguishable from bytecode, as either IsSmi()
or IsFunctionTemplateInfo() will hold for them respectively.

WDYT?

[oth, 2015-07-21 13:41:58]

On 2015/07/21 12:51:04, Michael Starzinger wrote:
> Drive-by ...
> 
> How about we use the SharedFunctionInfo::function_data for that overloading?
> This field is currently used for two purposes ...
> 
> 1) For API function which are backed by C++ implementation. I don't think we
> will ever need bytecode for them. :)
> 
> 2) For number special builtin functions. For now we would just exclude these
> functions from being interpreted. See the FUNCTIONS_WITH_ID_LIST list in the
> objects.h file for a list of what we are talking about here.
> 
> The above two cases are clearly distinguishable from bytecode, as either
IsSmi()
> or IsFunctionTemplateInfo() will hold for them respectively.
> 
> WDYT?

Great spot, let's try it. I'll rev the patch and add a test for BytecodeArray in
in test-heap.cc.
Thanks!

[oth, 2015-07-21 20:39:26]

Thanks all, the latest change adds a test for BytecodeArray (which exposed a
bug!) and saves using a field in SharedFunctionInfo for the bytecodes.

https://codereview.chromium.org/1230753004/diff/220001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1230753004/diff/220001/src/objects.h#newcode6710
src/objects.h:6710: DECL_ACCESSORS(bytecode_array, BytecodeArray)
On 2015/07/21 11:55:24, ulan wrote:
> This will increase memory footprint. Couple of month ago I saw that google
inbox
> had ~50K shared function infos. So this would mean additional 200K-400K live
> memory depending on architecture.
> 
> Maybe we can find a way to stash the pointer somewhere, so that we pay the
cost
> only when there is an actual byte array? Or do this behind build flag.

Done. Moved to SharedFunctionInfo::function_data.

[Michael Starzinger, 2015-07-22 07:20:55]

LGTM on the changes to SharedFunctionInfo. Didn't look at the rest, please wait
for Ulan's sign-off on this.

https://codereview.chromium.org/1230753004/diff/240001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1230753004/diff/240001/src/objects.h#newcode6714
src/objects.h:6714: DECL_ACCESSORS(bytecode_array, BytecodeArray)
nit: These declarations have become obsolete, let's drop them. The getter is
declared below and if we ever need a setter, we should also explicitly declare
it below.

[rmcilroy, 2015-07-22 07:44:01]

https://codereview.chromium.org/1230753004/diff/240001/src/objects-debug.cc
File src/objects-debug.cc (right):

https://codereview.chromium.org/1230753004/diff/240001/src/objects-debug.cc#n...
src/objects-debug.cc:552: VerifyObjectField(kFunctionDataOffset);
Could you add a CHECK here that kFunctionDataOffset is either a
FunctionTemplateInfo, a Smi, a BytecodeArray or Undefined.

https://codereview.chromium.org/1230753004/diff/240001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1230753004/diff/240001/src/objects.h#newcode6730
src/objects.h:6730: //  - a bytecode array for the interpreter
[HasBytecodeArray()].
nit /s/bytecode array/BytecodeArray

[ulan, 2015-07-22 08:11:59]

lgtm, thanks.

https://codereview.chromium.org/1230753004/diff/240001/src/types.cc
File src/types.cc (right):

https://codereview.chromium.org/1230753004/diff/240001/src/types.cc#newcode1
src/types.cc:1: 
Delete empty line.

[oth, 2015-07-22 08:50:00]

Thanks all!

https://codereview.chromium.org/1230753004/diff/240001/src/objects-debug.cc
File src/objects-debug.cc (right):

https://codereview.chromium.org/1230753004/diff/240001/src/objects-debug.cc#n...
src/objects-debug.cc:552: VerifyObjectField(kFunctionDataOffset);
On 2015/07/22 07:44:00, rmcilroy wrote:
> Could you add a CHECK here that kFunctionDataOffset is either a
> FunctionTemplateInfo, a Smi, a BytecodeArray or Undefined.

Done. And added call to verify Smi/BytecodeArray.

https://codereview.chromium.org/1230753004/diff/240001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1230753004/diff/240001/src/objects.h#newcode6714
src/objects.h:6714: DECL_ACCESSORS(bytecode_array, BytecodeArray)
On 2015/07/22 07:20:54, Michael Starzinger wrote:
> nit: These declarations have become obsolete, let's drop them. The getter is
> declared below and if we ever need a setter, we should also explicitly declare
> it below.

Done.

https://codereview.chromium.org/1230753004/diff/240001/src/objects.h#newcode6730
src/objects.h:6730: //  - a bytecode array for the interpreter
[HasBytecodeArray()].
On 2015/07/22 07:44:00, rmcilroy wrote:
> nit /s/bytecode array/BytecodeArray

Done.

https://codereview.chromium.org/1230753004/diff/240001/src/types.cc
File src/types.cc (right):

https://codereview.chromium.org/1230753004/diff/240001/src/types.cc#newcode1
src/types.cc:1: 
On 2015/07/22 08:11:58, ulan wrote:
> Delete empty line.

Done.

[oth, 2015-07-23 17:09:43]

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

[commit-bot: I haz the power, 2015-07-23 17:09:54]

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

[commit-bot: I haz the power, 2015-07-23 17:17:19]

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

[commit-bot: I haz the power, 2015-07-23 17:17:20]

Dry run: Try jobs failed on following builders:
  v8_linux_gcc_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)

[oth, 2015-07-24 09:47:25]

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

[commit-bot: I haz the power, 2015-07-24 09:47:31]

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

[oth, 2015-07-24 11:23:08]

The CQ bit was checked by oth@chromium.org

[oth, 2015-07-24 11:23:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org, ulan@chromium.org, mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1230753004/#ps400001
(title: "Fix pedantic build (take2).")

[commit-bot: I haz the power, 2015-07-24 11:23:18]

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

[commit-bot: I haz the power, 2015-07-24 12:02:49]

Committed patchset #21 (id:400001)

[commit-bot: I haz the power, 2015-07-24 12:03:16]

Patchset 21 (id:??) landed as
https://crrev.com/cb6e7056862ee6991d3d884a92d36bebe45b8152
Cr-Commit-Position: refs/heads/master@{#29843}