Bug 125920

Summary: [MIPS] Use shorter j <address> jump in MacroAssembler::replaceWithJump.
Product: WebKit Reporter: Balazs Kilvady <kilvadyb>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: NEW    
Severity: Normal CC: benjamin, bfulgham, cmarcelo, commit-queue, fpizlo, fu, gergely, ggaren, jbriance, mhahnenberg, msaboff, palfia, ssseintr1
Priority: P2 Keywords: DoNotImportToRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 108664    
Attachments:
Description Flags
proposed patch. fpizlo: review-, fpizlo: commit-queue-

Balazs Kilvady
Reported 2013-12-18 04:15:33 PST
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
Attachments
proposed patch. (9.54 KB, patch)
2013-12-18 04:46 PST, Balazs Kilvady 		fpizlo: review-
fpizlo: commit-queue-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Balazs Kilvady
Comment 1 2013-12-18 04:46:34 PST
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
Comment 2 2015-07-24 05:14:46 PDT
+ } 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
Comment 3 2015-07-24 05:25:16 PDT
(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
Comment 4 2015-07-27 08:38:36 PDT
(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
Comment 5 2015-07-27 08:58:37 PDT
> 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 6 2015-07-27 10:03:59 PDT
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 7 2016-01-26 05:33:49 PST
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.
Note You need to log in before you can comment on or make changes to this bug.