Odinmonkey (ARM): reporting OOM but it appears to be due to failure in the constant pools

RESOLVED WONTFIX

Status

()

RESOLVED WONTFIX
5 years ago
a year ago

People

(Reporter: dougc, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [leave open])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
While testing BananaBench on ARM B2G, the asm.js compiler can fail with
the following report:
"asm.js type error: internal codegen failure (probably out of memory)"

The oom flag is being set due to a call to fail_bail, rather than a real oom.

fail_bail is being called from finishPool, and the source code includes a
comment stating that this should not be possible on the ARM.

IonSpew output:

From finishPool:
[0] Finishing pool 136
[0] Linking entry 5 in pool 0
[0] Fixing offset to -1052
[0] Linking entry 4 in pool 0
[0] Fixing offset to -1048
[0]***Offset -1048 was still out of range!***
[0] Too complicated; bailingp

patchConstantPoolLoad is failing at:
      case PoolHintData::poolVDTR:
        if ((offset + (8 * data.getIndex()) - 8) < -1023 ||
            (offset + (8 * data.getIndex()) - 8) > 1023)
        {
            return false;
        }
with: offset -1048, and data.getIndex() 4, which gives -1024 which is out
of range for this comparison.

This is on a well patched m-c, running a patched benchmark, but reproducible here
for now.
(Reporter)

Comment 1

5 years ago
It seems to be caused, or tickled, by the patch in bug 865516.  Let me look into it.
(Reporter)

Comment 2

5 years ago
Created attachment 793270 [details] [diff] [review]
Leave some head-room in the double pools to help avoid bailing out.
(Reporter)

Comment 3

5 years ago
Comment on attachment 793270 [details] [diff] [review]
Leave some head-room in the double pools to help avoid bailing out.

Should this be considered for ff26?  I know it's a real hack, but
it does reduce the number of failures running asm.js code.
Attachment #793270 - Flags: review?(mrosenberg)
Attachment #793270 - Flags: review?(mrosenberg) → review+
(Reporter)

Comment 4

5 years ago
Created attachment 829950 [details] [diff] [review]
Leave some head-room in the double pools to help avoid bailing out

Rebase.  Carrying forward r+.

Note this does not solve the underlying problems with the pools, they still fail. It just leaves a little room so that a small error in packing the pools can be accepted and thus reduces the number of failures.  I understand there is a larger rewrite of the code that will eventually address the problem.
Attachment #793270 - Attachment is obsolete: true
Attachment #829950 - Flags: review+
(Reporter)

Updated

5 years ago
Keywords: checkin-needed
Whiteboard: [leave open]
(Reporter)

Comment 7

5 years ago
Created attachment 8342714 [details] [diff] [review]
Revert the reservation of some head-room in the constant pools as caused or tickled bug 944972

The patch being reverted was a hack workaround.  It's not clear if it actually caused the assertion failure in bug 944972, or just changed the pool layout and tickled it.
Attachment #8342714 - Flags: review?(mrosenberg)
Attachment #8342714 - Flags: review?(mrosenberg) → review+
(Reporter)

Updated

5 years ago
Keywords: checkin-needed
(Reporter)

Comment 9

5 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #8)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/cce5a57699a4
> 
> Is this WONTFIX now?

I suggest leaving this open as the problem is not fixed and I understand there is ongoing work to address the real problem.  The patch being backed out was just a quick attempt to reduce the frequency of failures in the buffer pools.
(Reporter)

Updated

5 years ago
Depends on: 760642
(Assignee)

Updated

4 years ago
Assignee: general → nobody
Mass-closing JS bugs for which the platform is Gonk (Firefox OS), since Firefox OS is gone. Feel free to re-open if still valid.
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.