Closed Bug 1027441 Opened 5 years ago Closed 5 years ago

OdinMonkey: fix use of size() with pending pool entries.

Categories

(Core :: JavaScript Engine: JIT, defect)

ARM
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
firefox31 --- fixed
firefox32 --- fixed
firefox33 --- fixed

People

(Reporter: dougc, Assigned: dougc)

References

Details

(Whiteboard: [qa-])

Attachments

(3 files, 1 obsolete file)

Added an assertion to check for uses of size() with pending pool entries and hit an assertion failure in the AsmJS code. The size() method may not even work if an elastic asm buffer is ever implemented, so could the use of size() simply be avoided by recording all asm buffer offsets using currentOffset() and converting them to their final offset by calling actualOffset() after the buffer is finished?
This might also enabled some of the calls to flush() to be avoided, but they seem appropriate points to flush the pool anyway.

Also considering calling flush() after each function, which would also fix the immediate problem, but each pool dump moves the buffer on to a new slice and each slice is 1024 words, so if there are a lot of very small asm.js functions then this could bloat the buffer.

https://tbpl.mozilla.org/?tree=Try&rev=1f90591f0151
Attachment #8442536 - Flags: review?(luke)
Makes sense.  With this, can all the #ifdef ARM masm.flush's go away in set*Offset?  I thought the purpose of those was to avoid to avoid needing masm.actualOffset().
(In reply to Luke Wagner [:luke] from comment #2)
> Makes sense.  With this, can all the #ifdef ARM masm.flush's go away in
> set*Offset?  I thought the purpose of those was to avoid to avoid needing
> masm.actualOffset().

Yes, these masm.flush's are no longer necessary. As a check, the jit-tests pass and big asm.js demos run with them removed.

They were necessary for size() to give an accurate value. However they might still be useful because flushing the pools before these code segments might avoid an unfortunate pool placement within them, and they appear to be in fast paths. Your call?
(In reply to Douglas Crosher [:dougc] from comment #3)
> They were necessary for size() to give an accurate value. However they might
> still be useful because flushing the pools before these code segments might
> avoid an unfortunate pool placement within them, and they appear to be in
> fast paths. Your call?

That's an interesting point.  It doesn't seem like a significant-enough risk to me, so I'd rather take 'em out.
Comment on attachment 8442536 [details] [diff] [review]
Avoid using size() to fix uses of size() with pending pool entries.

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

r+ with the flushes removed as discussed above.

::: js/src/jit/AsmJSModule.cpp
@@ +261,5 @@
>      // AsmJSModule.
>      JS_ASSERT(uintptr_t(code_) % AsmJSPageSize == 0);
>      masm.executableCopy(code_);
>  
> +    updateFunctionBytes(&masm);

Now that 'finish' is in AsmJSModule, can you just inline this and put it in the #ifdef ARM below where you put the other code offsets updates?

::: js/src/jit/AsmJSModule.h
@@ +202,5 @@
>          void initIonOffset(unsigned off) {
>              JS_ASSERT(!ionCodeOffset_);
>              ionCodeOffset_ = off;
>          }
> +        void updateOffsets(jit::MacroAssembler *masm) {

Can you pass a MacroAssembler & here and below?
Attachment #8442536 - Flags: review?(luke) → review+
Address reviewer feedback. Thank you for the quick review and feedback, it improved the patch. Carrying forward r+.
Attachment #8442536 - Attachment is obsolete: true
Attachment #8444219 - Flags: review+
Summary: OdinMonkey: fix use of size() with a pending pool entries. → OdinMonkey: fix use of size() with pending pool entries.
Comment on attachment 8444223 [details] [diff] [review]
Patch rebased to Aurora 32.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): long standing issue.

User impact if declined: Only noticed issues for the profiler - it could have recorded a slightly inaccurate extent for the asm.js functions. However, pending patches add an assertion check that catches this programming error and this would cause an assertion failure if not fixed, so uplifting this patch would clear the way for these pending fixes.

Testing completed (on m-c, etc.): Locally, and a try build. I run tbpl jit-tests locally, and build debug versions of the browser and testing them on large asm.js demos both in the ARM simulator and on hardware.

Risk to taking this patch (and alternatives if risky): Low.

String or IDL/UUID changes made by this patch: n/a
Attachment #8444223 - Flags: approval-mozilla-aurora?
Comment on attachment 8444224 [details] [diff] [review]
Patch rebased to Beta 31.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): long standing issue.

User impact if declined: Only noticed issues for the profiler - it could have recorded a slightly inaccurate extent for the asm.js functions. However, pending patches add an assertion check that catches this programming error and this would cause an assertion failure if not fixed, so uplifting this patch would clear the way for these pending fixes.

Testing completed (on m-c, etc.): Locally, and a try build. I run tbpl jit-tests locally, and build debug versions of the browser and testing them on large asm.js demos both in the ARM simulator and on hardware.

Risk to taking this patch (and alternatives if risky): Low.

String or IDL/UUID changes made by this patch: n/a
Attachment #8444224 - Flags: approval-mozilla-beta?
Keywords: checkin-needed
Comment on attachment 8444224 [details] [diff] [review]
Patch rebased to Beta 31.

Approving, we discussed with Ryan on IRC.
Attachment #8444224 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8444223 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [qa-]
https://hg.mozilla.org/mozilla-central/rev/ad7616fcaefe
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Depends on: 1036378
No longer depends on: 1036378
You need to log in before you can comment on or make changes to this bug.