917800 - Odinmonkey: correct rounding up of the heap length.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Douglas Crosher [:dougc]]

Sorry, there was a typo in the function RoundUpToNextValidAsmJSHeapLength
added in bug 911254 that prevents the use of a 4M quanta for heap lengths
less than 1024M.

Comment 1

[Douglas Crosher [:dougc]]

Attachment: Fix the rounding up of the heap length, add some test cases

The error message reported when the heap buffer size is
not large enough for the heap length constraint has also
been modified to report the actual conflicting lengths.

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 806625 [details] [diff] [review]
Fix the rounding up of the heap length, add some test cases

Review of attachment 806625 [details] [diff] [review]:
-----------------------------------------------------------------

D'oh!  Good catch

::: js/src/jit/AsmJSLink.cpp
@@ +225,5 @@
>          // loads and stores start on an aligned boundary and the heap byteLength has larger alignment.
>          JS_ASSERT((module.minHeapLength() - 1) <= INT32_MAX);
>          if (heap->byteLength() < module.minHeapLength())
> +            return LinkFail(cx, JS_smprintf("ArrayBuffer byteLength of 0x%x is less than the largest source code heap length constraint of 0x%x.",
> +                                            heap->byteLength(), module.minHeapLength()));

This line now needs braces.  Also, for the wording, how about: "ArrayBuffer byteLength of 0x%x is less than 0x%x (which is the largest constant heap access offset rounded up to the next valid heap size)"

Comment 3

[Douglas Crosher [:dougc]]

Attachment: Fix the rounding up of the heap length, add some test cases, address reviewer feedback.

Carrying forward r+

Comment 4

[Douglas Crosher [:dougc]]

(In reply to Luke Wagner [:luke] from comment #2)
> Comment on attachment 806625 [details] [diff] [review]
...
> ::: js/src/jit/AsmJSLink.cpp
> @@ +225,5 @@
> >          // loads and stores start on an aligned boundary and the heap byteLength has larger alignment.
> >          JS_ASSERT((module.minHeapLength() - 1) <= INT32_MAX);
> >          if (heap->byteLength() < module.minHeapLength())
> > +            return LinkFail(cx, JS_smprintf("ArrayBuffer byteLength of 0x%x is less than the largest source code heap length constraint of 0x%x.",
> > +                                            heap->byteLength(), module.minHeapLength()));
> 
> This line now needs braces.  Also, for the wording, how about: "ArrayBuffer
> byteLength of 0x%x is less than 0x%x (which is the largest constant heap
> access offset rounded up to the next valid heap size)"

Done. Thank you for the quick review and feedback.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/58e8f3ac3986

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/58e8f3ac3986

Comment 7

[Douglas Crosher [:dougc]]

Comment on attachment 806652 [details] [diff] [review]
Fix the rounding up of the heap length, add some test cases, address reviewer feedback.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 911254
User impact if declined: Broken feature would cause backwards compatibility problems. The feature has already been uplifted in bug 911254, but sorry there was a typo in the table for rounding the heap lengths and it would seem best to correct this.
Testing completed (on m-c, etc.): new tests were added
Risk to taking this patch (and alternatives if risky): very low 
String or IDL/UUID changes made by this patch:

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/02e6cc010fcf