913216 - OdinMonkey: (ARM) ensure there is only one cache flush per bounds check rewrite

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Marty Rosenberg [:mjrosenb]]

Attachment: /home/mjrosenb/patches/clumpFlush-r0.patch

The heuristic we currently use for cache flushes in the AutoFlushCache is:
if the new region to be flushed is within a page of the old region, then expand the range. If it is outside of a page from the current range, then there may be a hole, and we need to break it up into two ranges.  flush the new code, and keep the old range.  It ooks like with asm.js, this fails miserably.  I should update the logic, but simply marking the entire range as 'modified' a priori nicely solves this problem.

Comment 1

[Marty Rosenberg [:mjrosenb]]

Attachment: /home/mjrosenb/patches/clumpFlush-r2.patch

Modifications from IRC.  also double version bump because I wanted to make sure we actually ran (we did not)

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 800445 [details] [diff] [review]
/home/mjrosenb/patches/clumpFlush-r2.patch

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

::: js/src/jit/AsmJSModule.cpp
@@ +36,5 @@
> +
> +    if (heapAccesses_.length() > 0)
> +        // We already know the exact extent of areas that need to be patched, just make sure we
> +        // flush all of them at once.
> +        jit::AutoFlushCache::updateTop(uintptr_t(code_), pod.codeBytes_);

You're the expert on this, but shouldn't the cache flush (which happens synchronously now) happen after the loop?

Second, I think it'd be safe to leave off the "if (heapAccesses_.length() > 0)"; any module of significant size is going to be using the heap and a cache flush isn't the end of the world.

Comment 3

[Marty Rosenberg [:mjrosenb]]

> Second, I think it'd be safe to leave off the "if (heapAccesses_.length() >
> 0)"; any module of significant size is going to be using the heap and a
> cache flush isn't the end of the world.
Right you are.  That is left over from when I was using the first and last elements to calculate the range, and I didn't want to access uninitialized memory.

Comment 4

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2a15f832f616