Encode interpreter::SourcePositionTable as variable-length ints.

Encode interpreter::SourcePositionTable as variable-length ints.

This reduces the memory consumption of SourcePositionTable by ca. 2/3.
Over Octane, this reduces the source position table memory consumption
from ~370kB to ~115kB, which makes it ca. 10% of the total bytecode size
(~1.1MB)

BUG=

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

[vogelheim, 2016-02-17 14:26:03]

Description was changed from

==========
Encode interpreter::SourcePositionTable as variable-length ints.

This reduces the memory consumption of SourcePositionTable by ca. 2/3.
Over Octane, this reduces the source position table memory consumption
from ~270kB to ~115kB, which makes it ca. 10% of the total bytecode size
(~1.1MB)

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

to

==========
Encode interpreter::SourcePositionTable as variable-length ints.

This reduces the memory consumption of SourcePositionTable by ca. 2/3.
Over Octane, this reduces the source position table memory consumption
from ~370kB to ~115kB, which makes it ca. 10% of the total bytecode size
(~1.1MB)

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

[vogelheim, 2016-02-17 14:30:58]

vogelheim@chromium.org changed reviewers:
+ jochen@chromium.org, rmcilroy@chromium.org, yangguo@chromium.org

[vogelheim, 2016-02-17 14:30:58]

rmcilroy: src/interpreter OWNERs review.
jochen: src/heap OWNERs review. heap.cc change is only moderately exciting,
though.
yangguo: everything. :)


Supporting data:
https://docs.google.com/a/chromium.org/spreadsheets/d/1fQLVbTF4eW7RoP1u2AUHG8...

[jochen (gone - plz use gerrit), 2016-02-17 14:38:29]

lgtm

[Yang, 2016-02-17 19:54:51]

lgtm with a bunch of comments.

https://codereview.chromium.org/1704943002/diff/40001/src/interpreter/source-...
File src/interpreter/source-position-table.cc (right):

https://codereview.chromium.org/1704943002/diff/40001/src/interpreter/source-...
src/interpreter/source-position-table.cc:37: class TypeBit : public
BitField8<bool, 6, 1> {};
Since we always stuff the type bit into the bytecode offset, wouldn't it be more
straight-forward to have MoreBit, SourcePositionBits, TypeBit, and
CodeOffsetBits?

https://codereview.chromium.org/1704943002/diff/40001/src/interpreter/source-...
src/interpreter/source-position-table.cc:44: while (value >= (1 << size *
ValueBits::kSize)) size++;
Let's use brackets around the multiplication.

https://codereview.chromium.org/1704943002/diff/40001/src/interpreter/source-...
src/interpreter/source-position-table.cc:63: // Helper: Encode a byte w/ a flag
value.
Can we call this 'type' instead of 'flag' to be consistent?

https://codereview.chromium.org/1704943002/diff/40001/src/interpreter/source-...
src/interpreter/source-position-table.cc:82: value = -value;
Let's assert that value is not kIntMin. Or else this could get ugly.

https://codereview.chromium.org/1704943002/diff/40001/src/interpreter/source-...
src/interpreter/source-position-table.cc:152: // SourcePositionTableCodec
I don't really see the value of having a standalone class for encoding or
decoding. The only thing encoding and decoding shares are the previous_* fields,
but otherwise do not share any code. I'd simply move them into the builder and
iterator classes.

https://codereview.chromium.org/1704943002/diff/40001/src/interpreter/source-...
File src/interpreter/source-position-table.h (right):

https://codereview.chromium.org/1704943002/diff/40001/src/interpreter/source-...
src/interpreter/source-position-table.h:31: class SourcePositionTableCodec {
I expect this to be only used by the builder and the iterator. Can we put this
class in an anonymous namespace?

https://codereview.chromium.org/1704943002/diff/40001/src/interpreter/source-...
src/interpreter/source-position-table.h:98: explicit
SourcePositionTableIterator(ByteArray* byte_array);
I'd actually be perfectly fine if we changed the public constructor to take a
ByteArray* and change all the call sites. Your decision :)

https://codereview.chromium.org/1704943002/diff/40001/test/unittests/interpre...
File test/unittests/interpreter/source-position-table-unittest.cc (right):

https://codereview.chromium.org/1704943002/diff/40001/test/unittests/interpre...
test/unittests/interpreter/source-position-table-unittest.cc:23: 129, 250, 1000,
9999, 12000, 31415926};
Since we are encoding the delta, these are not really suspicous, are they? Could
we have a few offsets where the difference to the last one is some power of 2?

Also, we don't really test negative delta for source positions.

[vogelheim, 2016-02-18 13:06:10]

https://codereview.chromium.org/1704943002/diff/40001/src/interpreter/source-...
File src/interpreter/source-position-table.cc (right):

https://codereview.chromium.org/1704943002/diff/40001/src/interpreter/source-...
src/interpreter/source-position-table.cc:37: class TypeBit : public
BitField8<bool, 6, 1> {};
On 2016/02/17 19:54:51, Yang wrote:
> Since we always stuff the type bit into the bytecode offset, wouldn't it be
more
> straight-forward to have MoreBit, SourcePositionBits, TypeBit, and
> CodeOffsetBits?

I disagree. Only the first byte is different; all subsequent bytes are always
MoreBit|ValueBits. I.e., the data format it:

[MoreBit|TypeBit|TypeValueBits] [MoreBit|ValueBits]*
or: [MoreBit|ValueBits]*

So the value bits really are the bits for all of the values.

https://codereview.chromium.org/1704943002/diff/40001/src/interpreter/source-...
src/interpreter/source-position-table.cc:44: while (value >= (1 << size *
ValueBits::kSize)) size++;
On 2016/02/17 19:54:51, Yang wrote:
> Let's use brackets around the multiplication.

Done.

https://codereview.chromium.org/1704943002/diff/40001/src/interpreter/source-...
src/interpreter/source-position-table.cc:63: // Helper: Encode a byte w/ a flag
value.
On 2016/02/17 19:54:51, Yang wrote:
> Can we call this 'type' instead of 'flag' to be consistent?

Done.

https://codereview.chromium.org/1704943002/diff/40001/src/interpreter/source-...
src/interpreter/source-position-table.cc:82: value = -value;
On 2016/02/17 19:54:51, Yang wrote:
> Let's assert that value is not kIntMin. Or else this could get ugly.

Done.

https://codereview.chromium.org/1704943002/diff/40001/src/interpreter/source-...
src/interpreter/source-position-table.cc:152: // SourcePositionTableCodec
On 2016/02/17 19:54:51, Yang wrote:
> I don't really see the value of having a standalone class for encoding or
> decoding. The only thing encoding and decoding shares are the previous_*
fields,
> but otherwise do not share any code. I'd simply move them into the builder and
> iterator classes.

Done.

https://codereview.chromium.org/1704943002/diff/40001/src/interpreter/source-...
File src/interpreter/source-position-table.h (right):

https://codereview.chromium.org/1704943002/diff/40001/src/interpreter/source-...
src/interpreter/source-position-table.h:31: class SourcePositionTableCodec {
On 2016/02/17 19:54:51, Yang wrote:
> I expect this to be only used by the builder and the iterator. Can we put this
> class in an anonymous namespace?

I removed it entirely, per your other suggestion.

https://codereview.chromium.org/1704943002/diff/40001/src/interpreter/source-...
src/interpreter/source-position-table.h:98: explicit
SourcePositionTableIterator(ByteArray* byte_array);
On 2016/02/17 19:54:51, Yang wrote:
> I'd actually be perfectly fine if we changed the public constructor to take a
> ByteArray* and change all the call sites. Your decision :)

Done. I like your version better.

https://codereview.chromium.org/1704943002/diff/40001/test/unittests/interpre...
File test/unittests/interpreter/source-position-table-unittest.cc (right):

https://codereview.chromium.org/1704943002/diff/40001/test/unittests/interpre...
test/unittests/interpreter/source-position-table-unittest.cc:23: 129, 250, 1000,
9999, 12000, 31415926};
On 2016/02/17 19:54:51, Yang wrote:
> Since we are encoding the delta, these are not really suspicous, are they?
Could
> we have a few offsets where the difference to the last one is some power of 2?
> 
> Also, we don't really test negative delta for source positions.

I disagree. Your feedback is correct for the first two tests, but the
EncodeAscending test handles both of those issues. I actually did that on
purpose, so that the different unit tests would test different things. E.g.
EncodeStatement succeeding but EncodeAscending failing would likely indicate an
issue with negative numbers and/or bit the bit boundaries.

[rmcilroy, 2016-02-18 14:29:53]

https://codereview.chromium.org/1704943002/diff/60001/src/interpreter/source-...
File src/interpreter/source-position-table.cc (right):

https://codereview.chromium.org/1704943002/diff/60001/src/interpreter/source-...
src/interpreter/source-position-table.cc:59:
bytes.push_back(MoreBit::encode(more) |
DCHECK value is less than 7 bits?

https://codereview.chromium.org/1704943002/diff/60001/src/interpreter/source-...
src/interpreter/source-position-table.cc:65:
bytes.push_back(MoreBit::encode(more) | TypeBit::encode(type) |
Ditto, but 6 bits?

https://codereview.chromium.org/1704943002/diff/60001/src/interpreter/source-...
src/interpreter/source-position-table.cc:98: EncodeBytes(bytes, value, size -
1);
This is a bit magic, i.e., you are relying on the fact that size is less than
what is required for EncodeBytes of this value and that it will therefore only
encode the bottom bytes of the value. I think this would all be a lot simpler if
you did the seperation between TypeValues and Values at a higher level, and just
feed the encoding helpers raw ints which it encodes uniformly with the variable
byte compression scheme. To do this, you might want to have a typed value have
value first, and be shifted up by one with the type bit in the LSB of the value
(to keep the values small for this compression scheme). WDYT?

https://codereview.chromium.org/1704943002/diff/60001/src/interpreter/source-...
src/interpreter/source-position-table.cc:187: // Reverting means we need to
discard 2 variable length ints. We also need
Hmm, I now like this RevertPosition function even less than when it was
originally added :/. Could we remove it and instead have the
bytecode-array-builder keep track of the last source / expression position and
only add it to the source position table if a bytecode gets emitted (i.e., throw
it away if exit_seen_in_block_ where we currently call RevertPosition)?

[Yang, 2016-02-18 14:36:02]

On 2016/02/18 14:29:53, rmcilroy wrote:
>
https://codereview.chromium.org/1704943002/diff/60001/src/interpreter/source-...
> File src/interpreter/source-position-table.cc (right):
> 
>
https://codereview.chromium.org/1704943002/diff/60001/src/interpreter/source-...
> src/interpreter/source-position-table.cc:59:
> bytes.push_back(MoreBit::encode(more) |
> DCHECK value is less than 7 bits?
> 
>
https://codereview.chromium.org/1704943002/diff/60001/src/interpreter/source-...
> src/interpreter/source-position-table.cc:65:
> bytes.push_back(MoreBit::encode(more) | TypeBit::encode(type) |
> Ditto, but 6 bits?
> 
>
https://codereview.chromium.org/1704943002/diff/60001/src/interpreter/source-...
> src/interpreter/source-position-table.cc:98: EncodeBytes(bytes, value, size -
> 1);
> This is a bit magic, i.e., you are relying on the fact that size is less than
> what is required for EncodeBytes of this value and that it will therefore only
> encode the bottom bytes of the value. I think this would all be a lot simpler
if
> you did the seperation between TypeValues and Values at a higher level, and
just
> feed the encoding helpers raw ints which it encodes uniformly with the
variable
> byte compression scheme. To do this, you might want to have a typed value have
> value first, and be shifted up by one with the type bit in the LSB of the
value
> (to keep the values small for this compression scheme). WDYT?
> 
>
https://codereview.chromium.org/1704943002/diff/60001/src/interpreter/source-...
> src/interpreter/source-position-table.cc:187: // Reverting means we need to
> discard 2 variable length ints. We also need
> Hmm, I now like this RevertPosition function even less than when it was
> originally added :/. Could we remove it and instead have the
> bytecode-array-builder keep track of the last source / expression position and
> only add it to the source position table if a bytecode gets emitted (i.e.,
throw
> it away if exit_seen_in_block_ where we currently call RevertPosition)?

That would mean that everytime you emit bytecode, you'd need to notify the
source position builder.

How about simply buffering one entry, and write it to the table when we get a
new entry? Or keep it in memory the old, bloated way, and encode everything at
the end in ToSourcePositionTable?

[rmcilroy, 2016-02-18 14:43:57]

On 2016/02/18 14:36:02, Yang wrote:
> On 2016/02/18 14:29:53, rmcilroy wrote:
> >
>
https://codereview.chromium.org/1704943002/diff/60001/src/interpreter/source-...
> > File src/interpreter/source-position-table.cc (right):
> > 
> >
>
https://codereview.chromium.org/1704943002/diff/60001/src/interpreter/source-...
> > src/interpreter/source-position-table.cc:59:
> > bytes.push_back(MoreBit::encode(more) |
> > DCHECK value is less than 7 bits?
> > 
> >
>
https://codereview.chromium.org/1704943002/diff/60001/src/interpreter/source-...
> > src/interpreter/source-position-table.cc:65:
> > bytes.push_back(MoreBit::encode(more) | TypeBit::encode(type) |
> > Ditto, but 6 bits?
> > 
> >
>
https://codereview.chromium.org/1704943002/diff/60001/src/interpreter/source-...
> > src/interpreter/source-position-table.cc:98: EncodeBytes(bytes, value, size
-
> > 1);
> > This is a bit magic, i.e., you are relying on the fact that size is less
than
> > what is required for EncodeBytes of this value and that it will therefore
only
> > encode the bottom bytes of the value. I think this would all be a lot
simpler
> if
> > you did the seperation between TypeValues and Values at a higher level, and
> just
> > feed the encoding helpers raw ints which it encodes uniformly with the
> variable
> > byte compression scheme. To do this, you might want to have a typed value
have
> > value first, and be shifted up by one with the type bit in the LSB of the
> value
> > (to keep the values small for this compression scheme). WDYT?
> > 
> >
>
https://codereview.chromium.org/1704943002/diff/60001/src/interpreter/source-...
> > src/interpreter/source-position-table.cc:187: // Reverting means we need to
> > discard 2 variable length ints. We also need
> > Hmm, I now like this RevertPosition function even less than when it was
> > originally added :/. Could we remove it and instead have the
> > bytecode-array-builder keep track of the last source / expression position
and
> > only add it to the source position table if a bytecode gets emitted (i.e.,
> throw
> > it away if exit_seen_in_block_ where we currently call RevertPosition)?
> 
> That would mean that everytime you emit bytecode, you'd need to notify the
> source position builder.
> 
> How about simply buffering one entry, and write it to the table when we get a
> new entry? 

Sorry I probably wasn't explaining myself properly, but this was pretty much
exactly what I meant, although I meant keeping the in-flight entry in
bytecode-array-builder and only emitting it into the source position table when
we emit a bytecode (i.e., BytecodeArrayBuilder::SetExpressionPosition /
BytecodeArrayBuilder::SetStatementPosition only record the source position /
type locally, and then only call AddEntry on the source position table when they
actually emit the next bytecode, then clear this entry so that they only call
into source position table when a position has been set). Doing the buffering in
the source position table would also work, but then we would need to keep the
RevertPosition function which I would really rather get rid of if possible.

> Or keep it in memory the old, bloated way, and encode everything at
> the end in ToSourcePositionTable?

Also possible, but again I would prefer to avoid the RevertPosition at all if
possible.

[vogelheim, 2016-02-18 17:11:19]

https://codereview.chromium.org/1704943002/diff/60001/src/interpreter/source-...
File src/interpreter/source-position-table.cc (right):

https://codereview.chromium.org/1704943002/diff/60001/src/interpreter/source-...
src/interpreter/source-position-table.cc:98: EncodeBytes(bytes, value, size -
1);
On 2016/02/18 14:29:53, rmcilroy wrote:
> This is a bit magic, i.e., you are relying on the fact that size is less than
> what is required for EncodeBytes of this value and that it will therefore only
> encode the bottom bytes of the value. I think this would all be a lot simpler
if
> you did the seperation between TypeValues and Values at a higher level, and
just
> feed the encoding helpers raw ints which it encodes uniformly with the
variable
> byte compression scheme. To do this, you might want to have a typed value have
> value first, and be shifted up by one with the type bit in the LSB of the
value
> (to keep the values small for this compression scheme). WDYT?

Not sure...

My intent was this:

The methods called Encode*Value (also Decode*Value) have a sane interface and
will safely encode the value you put in. However, if I want to avoid code
duplication, I also need some low level helper functions that aren't safe, and
that will rely on their parameters being just right. Those are the Encode*Byte*
versions. The anonymous namespace provides some degree of protection against
mis-use.

EncodeBytes was originally called EncodeRemainder, which might make its purpose
a bit more clear.


I think I'll just implement an alternative as you suggest, and see if I like it
better...

[Yang, 2016-02-19 08:28:02]

https://codereview.chromium.org/1704943002/diff/40001/src/interpreter/source-...
File src/interpreter/source-position-table.cc (right):

https://codereview.chromium.org/1704943002/diff/40001/src/interpreter/source-...
src/interpreter/source-position-table.cc:37: class TypeBit : public
BitField8<bool, 6, 1> {};
On 2016/02/18 13:06:09, vogelheim wrote:
> On 2016/02/17 19:54:51, Yang wrote:
> > Since we always stuff the type bit into the bytecode offset, wouldn't it be
> more
> > straight-forward to have MoreBit, SourcePositionBits, TypeBit, and
> > CodeOffsetBits?
> 
> I disagree. Only the first byte is different; all subsequent bytes are always
> MoreBit|ValueBits. I.e., the data format it:
> 
> [MoreBit|TypeBit|TypeValueBits] [MoreBit|ValueBits]*
> or: [MoreBit|ValueBits]*
> 
> So the value bits really are the bits for all of the values.

I see. Thanks for the explanation.

https://codereview.chromium.org/1704943002/diff/40001/test/unittests/interpre...
File test/unittests/interpreter/source-position-table-unittest.cc (right):

https://codereview.chromium.org/1704943002/diff/40001/test/unittests/interpre...
test/unittests/interpreter/source-position-table-unittest.cc:23: 129, 250, 1000,
9999, 12000, 31415926};
On 2016/02/18 13:06:09, vogelheim wrote:
> On 2016/02/17 19:54:51, Yang wrote:
> > Since we are encoding the delta, these are not really suspicous, are they?
> Could
> > we have a few offsets where the difference to the last one is some power of
2?
> > 
> > Also, we don't really test negative delta for source positions.
> 
> I disagree. Your feedback is correct for the first two tests, but the
> EncodeAscending test handles both of those issues. I actually did that on
> purpose, so that the different unit tests would test different things. E.g.
> EncodeStatement succeeding but EncodeAscending failing would likely indicate
an
> issue with negative numbers and/or bit the bit boundaries.

I see. Ok.

[rmcilroy, 2016-02-22 12:38:41]

lgtm, but two suggestions which I think would make things even better.

https://codereview.chromium.org/1704943002/diff/80001/src/interpreter/source-...
File src/interpreter/source-position-table.cc (right):

https://codereview.chromium.org/1704943002/diff/80001/src/interpreter/source-...
src/interpreter/source-position-table.cc:61: DCHECK(abs(value) < (1 << 30));
Is there a reason this DCHECK is needed?

https://codereview.chromium.org/1704943002/diff/80001/src/interpreter/source-...
src/interpreter/source-position-table.cc:85: BytecodeOffsetBits::kMax));
Could you add a DCHECK that entry.bytecode_offset < BytecodeOffsetBits::kMax

https://codereview.chromium.org/1704943002/diff/80001/src/interpreter/source-...
src/interpreter/source-position-table.cc:134: entry->bytecode_offset = (tmp >>
BytecodeOffsetBits::kShift);
Personally I would just avoid using TypeBit / BytecodeOffsetBits entirely since
you need to do the shift explicitly here - I think that would be simpler and
possibly safer, WDYT?

https://codereview.chromium.org/1704943002/diff/120001/src/interpreter/source...
File src/interpreter/source-position-table.cc (right):

https://codereview.chromium.org/1704943002/diff/120001/src/interpreter/source...
src/interpreter/source-position-table.cc:162: void
SourcePositionTableBuilder::RevertPosition(size_t bytecode_offset) {
I much prefer what you've done here (in PS 5 and 6) but I still think it would
be much simpler and less confusing to have BytecodeArrayBuilder trigger the
emittion when it emits the bytecode rather than trying to fix it up if it
decides not to emit a bytecode for a source position. 

Actually now I think of it, I think all we need to do is have the
BytecodeArrayBuilder::SetReturn/Statement/ExpressionPosition functions check for
exit_seen_in_block_ and only actually encode source position if this is not true
- this would avoid the whole need for reverting positions entirely. WDYT?

[vogelheim, 2016-02-22 18:20:26]

Patchset #8 (id:140001) has been deleted

[vogelheim, 2016-02-22 18:33:33]

https://codereview.chromium.org/1704943002/diff/80001/src/interpreter/source-...
File src/interpreter/source-position-table.cc (right):

https://codereview.chromium.org/1704943002/diff/80001/src/interpreter/source-...
src/interpreter/source-position-table.cc:61: DCHECK(abs(value) < (1 << 30));
On 2016/02/22 12:38:40, rmcilroy wrote:
> Is there a reason this DCHECK is needed?

Done.

https://codereview.chromium.org/1704943002/diff/80001/src/interpreter/source-...
src/interpreter/source-position-table.cc:85: BytecodeOffsetBits::kMax));
On 2016/02/22 12:38:40, rmcilroy wrote:
> Could you add a DCHECK that entry.bytecode_offset < BytecodeOffsetBits::kMax

Done.

https://codereview.chromium.org/1704943002/diff/80001/src/interpreter/source-...
src/interpreter/source-position-table.cc:134: entry->bytecode_offset = (tmp >>
BytecodeOffsetBits::kShift);
On 2016/02/22 12:38:40, rmcilroy wrote:
> Personally I would just avoid using TypeBit / BytecodeOffsetBits entirely
since
> you need to do the shift explicitly here - I think that would be simpler and
> possibly safer, WDYT?

Done.

https://codereview.chromium.org/1704943002/diff/120001/src/interpreter/source...
File src/interpreter/source-position-table.cc (right):

https://codereview.chromium.org/1704943002/diff/120001/src/interpreter/source...
src/interpreter/source-position-table.cc:162: void
SourcePositionTableBuilder::RevertPosition(size_t bytecode_offset) {
On 2016/02/22 12:38:40, rmcilroy wrote:
> I much prefer what you've done here (in PS 5 and 6) but I still think it would
> be much simpler and less confusing to have BytecodeArrayBuilder trigger the
> emittion when it emits the bytecode rather than trying to fix it up if it
> decides not to emit a bytecode for a source position. 
> 
> Actually now I think of it, I think all we need to do is have the
> BytecodeArrayBuilder::SetReturn/Statement/ExpressionPosition functions check
for
> exit_seen_in_block_ and only actually encode source position if this is not
true
> - this would avoid the whole need for reverting positions entirely. WDYT?

Done.

[rmcilroy, 2016-02-22 20:51:52]

Great, thanks for making these changes! Please add BUG=v8:4690 to the
description. LGTM!

https://codereview.chromium.org/1704943002/diff/180001/src/interpreter/byteco...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1704943002/diff/180001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:1205: if (exit_seen_in_block_) return;
nit - could do this before calculating pos.

[Yang, 2016-02-22 21:56:25]

LGTM!

I do have a few suggestions.

https://codereview.chromium.org/1704943002/diff/180001/src/interpreter/byteco...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1704943002/diff/180001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:1205: if (exit_seen_in_block_) return;
On 2016/02/22 20:51:52, rmcilroy wrote:
> nit - could do this before calculating pos.

+1

Maybe also add a comment on why this is being done (because we won't emit code).

https://codereview.chromium.org/1704943002/diff/180001/src/interpreter/source...
File src/interpreter/source-position-table.cc (right):

https://codereview.chromium.org/1704943002/diff/180001/src/interpreter/source...
src/interpreter/source-position-table.cc:69:
bytes.push_back(MoreBit::encode(false));
Maybe we could use a constant here for the value bit, something like
static const int kNegativeSentinel, and encode this byte as
MoreBit::encode(false) | ValueBit::encode(kNegativeSentinel)

https://codereview.chromium.org/1704943002/diff/180001/src/interpreter/source...
src/interpreter/source-position-table.cc:94: if (ValueBits::decode(current) ==
0) {
We would compare this with kNegativeSentinel, instead of 0, which is a bit of a
magic number here.

[vogelheim, 2016-02-24 12:29:30]

https://codereview.chromium.org/1704943002/diff/180001/src/interpreter/byteco...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1704943002/diff/180001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:1205: if (exit_seen_in_block_) return;
On 2016/02/22 20:51:52, rmcilroy wrote:
> nit - could do this before calculating pos.

Done.

https://codereview.chromium.org/1704943002/diff/180001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:1205: if (exit_seen_in_block_) return;
On 2016/02/22 21:56:25, Yang wrote:
> On 2016/02/22 20:51:52, rmcilroy wrote:
> > nit - could do this before calculating pos.
> 
> +1
> 
> Maybe also add a comment on why this is being done (because we won't emit
code).

Done.

https://codereview.chromium.org/1704943002/diff/180001/src/interpreter/source...
File src/interpreter/source-position-table.cc (right):

https://codereview.chromium.org/1704943002/diff/180001/src/interpreter/source...
src/interpreter/source-position-table.cc:69:
bytes.push_back(MoreBit::encode(false));
On 2016/02/22 21:56:25, Yang wrote:
> Maybe we could use a constant here for the value bit, something like
> static const int kNegativeSentinel, and encode this byte as
> MoreBit::encode(false) | ValueBit::encode(kNegativeSentinel)

Done.

https://codereview.chromium.org/1704943002/diff/180001/src/interpreter/source...
src/interpreter/source-position-table.cc:94: if (ValueBits::decode(current) ==
0) {
On 2016/02/22 21:56:25, Yang wrote:
> We would compare this with kNegativeSentinel, instead of 0, which is a bit of
a
> magic number here.

Larry wants "more magic moments", so there you go. :-)

Also, done.

[vogelheim, 2016-02-24 12:51:19]

The CQ bit was checked by vogelheim@chromium.org

[vogelheim, 2016-02-24 12:51:19]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
rmcilroy@chromium.org, yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/1704943002/#ps200001
(title: "More review feedback.")

[commit-bot: I haz the power, 2016-02-24 12:51:29]

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

[commit-bot: I haz the power, 2016-02-24 12:53:47]

Description was changed from

==========
Encode interpreter::SourcePositionTable as variable-length ints.

This reduces the memory consumption of SourcePositionTable by ca. 2/3.
Over Octane, this reduces the source position table memory consumption
from ~370kB to ~115kB, which makes it ca. 10% of the total bytecode size
(~1.1MB)

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

to

==========
Encode interpreter::SourcePositionTable as variable-length ints.

This reduces the memory consumption of SourcePositionTable by ca. 2/3.
Over Octane, this reduces the source position table memory consumption
from ~370kB to ~115kB, which makes it ca. 10% of the total bytecode size
(~1.1MB)

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

[commit-bot: I haz the power, 2016-02-24 12:53:48]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2016-02-24 12:54:00]

Description was changed from

==========
Encode interpreter::SourcePositionTable as variable-length ints.

This reduces the memory consumption of SourcePositionTable by ca. 2/3.
Over Octane, this reduces the source position table memory consumption
from ~370kB to ~115kB, which makes it ca. 10% of the total bytecode size
(~1.1MB)

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

to

==========
Encode interpreter::SourcePositionTable as variable-length ints.

This reduces the memory consumption of SourcePositionTable by ca. 2/3.
Over Octane, this reduces the source position table memory consumption
from ~370kB to ~115kB, which makes it ca. 10% of the total bytecode size
(~1.1MB)

BUG=

Committed: https://crrev.com/a6f41f7b8226555c5900440f6e3092b3545ee0f6
Cr-Commit-Position: refs/heads/master@{#34250}
==========

[commit-bot: I haz the power, 2016-02-24 12:54:01]

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

[vogelheim, 2016-02-24 13:31:41]

A revert of this CL (patchset #10 id:200001) has been created in
https://codereview.chromium.org/1728193003/ by vogelheim@chromium.org.

The reason for reverting is: Build failure on Linux64 arm64 ASAN:

http://build.chromium.org/p/client.v8/builders/V8%20Linux64%20ASAN%20arm64%20...

(Leaks memory, somehow.).