Bug 125920 – [MIPS] Use shorter j <address> jump in MacroAssembler::replaceWithJump.

• Status: NEW
• Component: JavaScriptCore

[Balazs Kilvady]

Using a simpler direct "jump to address" instead of "jump register" instructions in MacroAssembler::replaceWithJump we could generate smaller code and we don't need additional nops in branchTrue() and branchFalse(). For this we need to ensure that the generated JIT code is within a 256MB-aligned region. This problem is discussed in
https://bugs.webkit.org/show_bug.cgi?id=103146

[Balazs Kilvady]

Created attachment 219523 [details]
proposed patch.

With using direct jump instruction the code is simpler, the generated code is smaller and the speed improvements are:
SunSpider  : 5294.0ms -> 5098.7ms
v8-v6: 18846.3ms -> 17286.7ms

[ssseintr]

+        } else if (isWithin256MB(reinterpret_cast<int8_t*>(result) + bytes, bytes)) {
+            // 2nd half is good, release 1st half.
+            if (munmap(result, bytes))
+                CRASH();
+        } else


I think the code is wrong here.
As we are only returning 2nd half of our result, result should be changed as result + bytes. (i.e, result = reinterpret_cast<int8_t*>(result) + bytes, bytes);)

Please comment me, if I'm wrong.

[Julien Brianceau]

(In reply to comment #2)
> I think the code is wrong here.
> As we are only returning 2nd half of our result, result should be changed as
> result + bytes. (i.e, result = reinterpret_cast<int8_t*>(result) + bytes,
> bytes);)
> 

You're right, in this case result should be updated :)

[ssseintr]

(In reply to comment #3)
> (In reply to comment #2)
> > I think the code is wrong here.
> > As we are only returning 2nd half of our result, result should be changed as
> > result + bytes. (i.e, result = reinterpret_cast<int8_t*>(result) + bytes,
> > bytes);)
> > 
> 
> You're right, in this case result should be updated :)

I would like to know, how this problem was not observed by any1 else?

Because it is very common get an address at the end of 256MB range, isn't it?

[Julien Brianceau]

> I would like to know, how this problem was not observed by any1 else?

Mainly because this change has not been committed in WebKit yet ;)
 
> Because it is very common get an address at the end of 256MB range, isn't it?

This case is quite unlikely: the chances that the first mmap doesn't fit between two 256Mb boundardies are low, then the chances that the 2nd mmap doesn't fit are also low. Finally, 50% chance that you'll need the 2nd mmaped area. But of course it's not impossible, so this would need to be fixed.

[Filip Pizlo]

Comment on attachment 219523 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=219523&action=review

We should remove MIPS from svn. You should maintain it downstream. This port doesn't have the kind of regular maintenance that we would expect from an in-tree port.

> Source/WTF/wtf/OSAllocatorPosix.cpp:75
> +    if (executable && UNLIKELY(!isWithin256MB(result, bytes))) {

I don't see how this can happen if you're using a fixed pool.

[Julien Brianceau]

Comment on attachment 219523 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=219523&action=review

>> Source/WTF/wtf/OSAllocatorPosix.cpp:75
>> +    if (executable && UNLIKELY(!isWithin256MB(result, bytes))) {
> 
> I don't see how this can happen if you're using a fixed pool.

It can happen because of the fixed pool allocation itself: this fixed pool must fit in a 256Mb page.