Make sure the constant pool size is as promised.

Make sure the constant pool size is as promised.

LOG=N
R=bmeurer@chromium.org
BUG=chromium:506443
Committed: https://crrev.com/619570b3ddff5f538158f6dd542018f26b47c289
Cr-Commit-Position: refs/heads/master@{#29463}

[Yang, 2015-07-02 11:23:07]

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

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

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1217673003/1

[commit-bot: I haz the power, 2015-07-02 11:48:30]

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

[commit-bot: I haz the power, 2015-07-02 11:48:30]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2015-07-02 13:59:48]

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

[rmcilroy, 2015-07-02 13:59:48]

https://codereview.chromium.org/1217673003/diff/1/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (left):

https://codereview.chromium.org/1217673003/diff/1/src/arm/assembler-arm.cc#ol...
src/arm/assembler-arm.cc:3859: for (int j = 0; j < i; j++) {
By removing this sharing you are going to increase the size of the generated Arm
code which is not ideal (especially considering we are trying all we can to
reduce heap usage on Android). Is there another way we could deal with the
duplicates (e.g., having some way for the debugger to estimate the size of the
constant pool ahead of time?).

Happy to VC about this if it helps.

[Yang, 2015-07-02 14:25:18]

On 2015/07/02 13:59:48, rmcilroy wrote:
> https://codereview.chromium.org/1217673003/diff/1/src/arm/assembler-arm.cc
> File src/arm/assembler-arm.cc (left):
> 
>
https://codereview.chromium.org/1217673003/diff/1/src/arm/assembler-arm.cc#ol...
> src/arm/assembler-arm.cc:3859: for (int j = 0; j < i; j++) {
> By removing this sharing you are going to increase the size of the generated
Arm
> code which is not ideal (especially considering we are trying all we can to
> reduce heap usage on Android). Is there another way we could deal with the
> duplicates (e.g., having some way for the debugger to estimate the size of the
> constant pool ahead of time?).
> 
> Happy to VC about this if it helps.

I'd like to have correctness first. This bug is not new, it just wasnt exposed
before. Currently neither the reloc info nor the constant pool header reports
the correct number of constants and there is no way to figure out after the fact
how many constants we have.

We could deduplicate while inserting, but that seems more involved, and I'd like
to not have this block this bug fix.

[Yang, 2015-07-02 14:29:02]

On 2015/07/02 14:25:18, Yang wrote:
> On 2015/07/02 13:59:48, rmcilroy wrote:
> > https://codereview.chromium.org/1217673003/diff/1/src/arm/assembler-arm.cc
> > File src/arm/assembler-arm.cc (left):
> > 
> >
>
https://codereview.chromium.org/1217673003/diff/1/src/arm/assembler-arm.cc#ol...
> > src/arm/assembler-arm.cc:3859: for (int j = 0; j < i; j++) {
> > By removing this sharing you are going to increase the size of the generated
> Arm
> > code which is not ideal (especially considering we are trying all we can to
> > reduce heap usage on Android). Is there another way we could deal with the
> > duplicates (e.g., having some way for the debugger to estimate the size of
the
> > constant pool ahead of time?).
> > 
> > Happy to VC about this if it helps.
> 
> I'd like to have correctness first. This bug is not new, it just wasnt exposed
> before. Currently neither the reloc info nor the constant pool header reports
> the correct number of constants and there is no way to figure out after the
fact
> how many constants we have.
> 
> We could deduplicate while inserting, but that seems more involved, and I'd
like
> to not have this block this bug fix.

or we could defer emitting the constants until after deduplicating.

[rmcilroy, 2015-07-02 15:15:08]

On 2015/07/02 14:29:02, Yang wrote:
> On 2015/07/02 14:25:18, Yang wrote:
> > On 2015/07/02 13:59:48, rmcilroy wrote:
> > > https://codereview.chromium.org/1217673003/diff/1/src/arm/assembler-arm.cc
> > > File src/arm/assembler-arm.cc (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/1217673003/diff/1/src/arm/assembler-arm.cc#ol...
> > > src/arm/assembler-arm.cc:3859: for (int j = 0; j < i; j++) {
> > > By removing this sharing you are going to increase the size of the
generated
> > Arm
> > > code which is not ideal (especially considering we are trying all we can
to
> > > reduce heap usage on Android). Is there another way we could deal with the
> > > duplicates (e.g., having some way for the debugger to estimate the size of
> the
> > > constant pool ahead of time?).
> > > 
> > > Happy to VC about this if it helps.
> > 
> > I'd like to have correctness first. This bug is not new, it just wasnt
exposed
> > before. Currently neither the reloc info nor the constant pool header
reports
> > the correct number of constants and there is no way to figure out after the
> fact
> > how many constants we have.

It might be new - https://codereview.chromium.org/1162993006/ changed this quite
a lot and might have changed when we do the deduping in relation to emitting the
constants.
 
> > We could deduplicate while inserting, but that seems more involved, and I'd
> like
> > to not have this block this bug fix.
> 
> or we could defer emitting the constants until after deduplicating.

This would work for me. You should be able to use the  merged_index field in
ConstantPoolEntry to do deduping in-place in the pending_xx_bit_constants_ array
before you go about emitting the constants. WDYT?

[Yang, 2015-07-02 17:04:59]

On 2015/07/02 15:15:08, rmcilroy wrote:
> On 2015/07/02 14:29:02, Yang wrote:
> > On 2015/07/02 14:25:18, Yang wrote:
> > > On 2015/07/02 13:59:48, rmcilroy wrote:
> > > >
https://codereview.chromium.org/1217673003/diff/1/src/arm/assembler-arm.cc
> > > > File src/arm/assembler-arm.cc (left):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1217673003/diff/1/src/arm/assembler-arm.cc#ol...
> > > > src/arm/assembler-arm.cc:3859: for (int j = 0; j < i; j++) {
> > > > By removing this sharing you are going to increase the size of the
> generated
> > > Arm
> > > > code which is not ideal (especially considering we are trying all we can
> to
> > > > reduce heap usage on Android). Is there another way we could deal with
the
> > > > duplicates (e.g., having some way for the debugger to estimate the size
of
> > the
> > > > constant pool ahead of time?).
> > > > 
> > > > Happy to VC about this if it helps.
> > > 
> > > I'd like to have correctness first. This bug is not new, it just wasnt
> exposed
> > > before. Currently neither the reloc info nor the constant pool header
> reports
> > > the correct number of constants and there is no way to figure out after
the
> > fact
> > > how many constants we have.
> 
> It might be new - https://codereview.chromium.org/1162993006/ changed this
quite
> a lot and might have changed when we do the deduping in relation to emitting
the
> constants.
>  
> > > We could deduplicate while inserting, but that seems more involved, and
I'd
> > like
> > > to not have this block this bug fix.
> > 
> > or we could defer emitting the constants until after deduplicating.
> 
> This would work for me. You should be able to use the  merged_index field in
> ConstantPoolEntry to do deduping in-place in the pending_xx_bit_constants_
array
> before you go about emitting the constants. WDYT?

I was thinking that maybe we should sort the list before deduping to save time.

[Yang, 2015-07-03 07:12:27]

On 2015/07/02 17:04:59, Yang wrote:
> On 2015/07/02 15:15:08, rmcilroy wrote:
> > On 2015/07/02 14:29:02, Yang wrote:
> > > On 2015/07/02 14:25:18, Yang wrote:
> > > > On 2015/07/02 13:59:48, rmcilroy wrote:
> > > > >
> https://codereview.chromium.org/1217673003/diff/1/src/arm/assembler-arm.cc
> > > > > File src/arm/assembler-arm.cc (left):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1217673003/diff/1/src/arm/assembler-arm.cc#ol...
> > > > > src/arm/assembler-arm.cc:3859: for (int j = 0; j < i; j++) {
> > > > > By removing this sharing you are going to increase the size of the
> > generated
> > > > Arm
> > > > > code which is not ideal (especially considering we are trying all we
can
> > to
> > > > > reduce heap usage on Android). Is there another way we could deal with
> the
> > > > > duplicates (e.g., having some way for the debugger to estimate the
size
> of
> > > the
> > > > > constant pool ahead of time?).
> > > > > 
> > > > > Happy to VC about this if it helps.
> > > > 
> > > > I'd like to have correctness first. This bug is not new, it just wasnt
> > exposed
> > > > before. Currently neither the reloc info nor the constant pool header
> > reports
> > > > the correct number of constants and there is no way to figure out after
> the
> > > fact
> > > > how many constants we have.
> > 
> > It might be new - https://codereview.chromium.org/1162993006/ changed this
> quite
> > a lot and might have changed when we do the deduping in relation to emitting
> the
> > constants.
> >  
> > > > We could deduplicate while inserting, but that seems more involved, and
> I'd
> > > like
> > > > to not have this block this bug fix.
> > > 
> > > or we could defer emitting the constants until after deduplicating.
> > 
> > This would work for me. You should be able to use the  merged_index field in
> > ConstantPoolEntry to do deduping in-place in the pending_xx_bit_constants_
> array
> > before you go about emitting the constants. WDYT?
> 
> I was thinking that maybe we should sort the list before deduping to save
time.

Uploaded a new patch set. We now deduplicate before emitting anything.

I decided against sorting, since I have no idea what implications that would
have wrt const pool entries that are marked as not sharing_ok.

[Yang, 2015-07-03 07:13:14]

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

[commit-bot: I haz the power, 2015-07-03 07:13:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1217673003/20001

[commit-bot: I haz the power, 2015-07-03 07:38:37]

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

[commit-bot: I haz the power, 2015-07-03 07:38:38]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2015-07-03 08:58:04]

> I decided against sorting, since I have no idea what implications that would
> have wrt const pool entries that are marked as not sharing_ok.

Yes I would avoid this for now - I'm not sure how it would interact with ldr
range for entries which are reordered.

https://codereview.chromium.org/1217673003/diff/20001/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

https://codereview.chromium.org/1217673003/diff/20001/src/arm/assembler-arm.c...
src/arm/assembler-arm.cc:3801: entry.set_merged_index(j);
I think this the wrong way round, you should be setting
...constants[j].set_merged_index[i]. Otherwise the delta for entry will get
larger (since the j is larger than i) which means it might be out of ldr range.
I'm not sure if this is actually a problem though (I can't rememeber how the
assembler checks when it should emit the pool). Also if there was another
duplicate later then wouldnt entry j will get merged with that other dup while i
is still duped with j?

[rmcilroy, 2015-07-03 08:58:04]

> I decided against sorting, since I have no idea what implications that would
> have wrt const pool entries that are marked as not sharing_ok.

Yes I would avoid this for now - I'm not sure how it would interact with ldr
range for entries which are reordered.

https://codereview.chromium.org/1217673003/diff/20001/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

https://codereview.chromium.org/1217673003/diff/20001/src/arm/assembler-arm.c...
src/arm/assembler-arm.cc:3801: entry.set_merged_index(j);
I think this the wrong way round, you should be setting
...constants[j].set_merged_index[i]. Otherwise the delta for entry will get
larger (since the j is larger than i) which means it might be out of ldr range.
I'm not sure if this is actually a problem though (I can't rememeber how the
assembler checks when it should emit the pool). Also if there was another
duplicate later then wouldnt entry j will get merged with that other dup while i
is still duped with j?

[Yang, 2015-07-03 09:07:33]

https://codereview.chromium.org/1217673003/diff/20001/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

https://codereview.chromium.org/1217673003/diff/20001/src/arm/assembler-arm.c...
src/arm/assembler-arm.cc:3801: entry.set_merged_index(j);
On 2015/07/03 08:58:04, rmcilroy wrote:
> I think this the wrong way round, you should be setting
> ...constants[j].set_merged_index[i]. Otherwise the delta for entry will get
> larger (since the j is larger than i) which means it might be out of ldr
range.
> I'm not sure if this is actually a problem though (I can't rememeber how the
> assembler checks when it should emit the pool). Also if there was another
> duplicate later then wouldnt entry j will get merged with that other dup while
i
> is still duped with j?

I haven't changed the merge loop at all. j is never larger than i as indicated
by the for-loop condition. For every entry i we look at prior entries (0 .. i-1)
for duplicates. If we found an entry j, we will use the found prior entry, by
setting the merge_index of i to j.

[rmcilroy, 2015-07-03 09:45:14]

lgtm, thanks!

https://codereview.chromium.org/1217673003/diff/20001/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

https://codereview.chromium.org/1217673003/diff/20001/src/arm/assembler-arm.c...
src/arm/assembler-arm.cc:3801: entry.set_merged_index(j);
On 2015/07/03 09:07:33, Yang wrote:
> On 2015/07/03 08:58:04, rmcilroy wrote:
> > I think this the wrong way round, you should be setting
> > ...constants[j].set_merged_index[i]. Otherwise the delta for entry will get
> > larger (since the j is larger than i) which means it might be out of ldr
> range.
> > I'm not sure if this is actually a problem though (I can't rememeber how the
> > assembler checks when it should emit the pool). Also if there was another
> > duplicate later then wouldnt entry j will get merged with that other dup
while
> i
> > is still duped with j?
> 
> I haven't changed the merge loop at all. j is never larger than i as indicated
> by the for-loop condition. For every entry i we look at prior entries (0 ..
i-1)
> for duplicates. If we found an entry j, we will use the found prior entry, by
> setting the merge_index of i to j.

You're right, oops. I didn't look at the for loop header properly and assumed
that you were doing j=i, j++ :). This looks fine then, thanks.

[Yang, 2015-07-03 09:50:48]

The CQ bit was checked by yangguo@chromium.org

[commit-bot: I haz the power, 2015-07-03 09:50:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1217673003/20001

[commit-bot: I haz the power, 2015-07-03 10:32:34]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-07-03 10:32:47]

Patchset 2 (id:??) landed as
https://crrev.com/619570b3ddff5f538158f6dd542018f26b47c289
Cr-Commit-Position: refs/heads/master@{#29463}