Closed
Bug 1275204
Opened 8 years ago
Closed 7 years ago
JSValue overloading and crunching on Linux sparc64 causes segfaults
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: glaubitz, Assigned: glaubitz)
References
Details
Attachments
(8 files, 19 obsolete files)
1.42 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
1.23 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
2.90 KB,
patch
|
glandium
:
review+
martin
:
review+
|
Details | Diff | Splinter Review |
886 bytes,
patch
|
glandium
:
review+
martin
:
review+
|
Details | Diff | Splinter Review |
2.98 KB,
patch
|
martin
:
review+
|
Details | Diff | Splinter Review |
1.43 KB,
patch
|
jesup
:
review+
martin
:
review+
|
Details | Diff | Splinter Review |
2.29 KB,
patch
|
martin
:
review+
fitzgen
:
feedback+
|
Details | Diff | Splinter Review |
2.81 KB,
patch
|
ehoogeveen
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:46.0) Gecko/20100101 Firefox/46.0
Build ID: 20160508001106
Steps to reproduce:
Hi!
We're currently working to shape up sparc64 in Debian again and I have had a look at Firefox in the past days. The work is being done on Debian unstable with an up-to-date toolchain and kernel.
After fixing some issues with the compiler definitions [1], I was actually able to run "make" without problems and build Firefox 46 all the way through, however, "make install" crashes when running xpcshell [2]:
Executing /<<PKGBUILDDIR>>/build-browser/dist/bin/xpcshell -g /<<PKGBUILDDIR>>/build-browser/dist/bin/ -a /<<PKGBUILDDIR>>/build-browser/dist/bin/ -f /<<PKGBUILDDIR>>/toolkit/mozapps/installer/precompile_cache.js -e precompile_startupcache("resource://gre/");
Traceback (most recent call last):
File "/<<PKGBUILDDIR>>/toolkit/mozapps/installer/packager.py", line 410, in <module>
main()
File "/<<PKGBUILDDIR>>/toolkit/mozapps/installer/packager.py", line 404, in main
args.source, gre_path, base)
File "/<<PKGBUILDDIR>>/toolkit/mozapps/installer/packager.py", line 161, in precompile_cache
errors.fatal('Error while running startup cache precompilation')
File "/<<PKGBUILDDIR>>/python/mozbuild/mozpack/errors.py", line 103, in fatal
self._handle(self.FATAL, msg)
File "/<<PKGBUILDDIR>>/python/mozbuild/mozpack/errors.py", line 98, in _handle
raise ErrorMessage(msg)
mozpack.errors.ErrorMessage: Error: Error while running startup cache precompilation
Recompiling with --enable-debug-symbols and running xpcshell manually yields this backtrace:
(gdb) bt
#0 0xfff8000102a117c4 in js::gc::Cell::storeBuffer (this=<optimizedout>) at /build/firefox-F4xXKx/firefox-46.0.1/js/src/gc/Heap.h:1339
#1 js::HeapSlot::post (target=..., slot=0, kind=js::HeapSlot::Slot,owner=0xfff8000112523070, this=0xfff8000112523090)
at /build/firefox-F4xXKx/firefox-46.0.1/js/src/gc/Barrier.h:692#2 js::HeapSlot::set (this=0xfff8000112523090,
owner=owner@entry=0xfff8000112523070, slot=slot@entry=0, v=...,kind=js::HeapSlot::Slot) at /build/firefox-F4xXKx/firefox-46.0.1/js/src/gc/Barrier.h:679
#3 0xfff8000102a13634 in js::NativeObject::setSlot(this=0xfff8000112523070, value=..., slot=0) at /build/firefox-F4xXKx/firefox-46.0.1/js/src/vm/NativeObject.h:824
#4 0xfff8000102a13758 in js::NativeObject::setReservedSlot (v=...,index=0, this=<optimized out>) at /build/firefox-F4xXKx/firefox-46.0.1/js/src/vm/NativeObject.h:890
#5 js::ClonedBlockObject::create (cx=cx@entry=0xfff800010a73a400,block=..., enclosing=...) at /build/firefox-F4xXKx/firefox-46.0.1/js/src/vm/ScopeObject.cpp:945
#6 0xfff8000102a18f1c in js::ClonedBlockObject::createGlobal(cx=cx@entry=0xfff800010a73a400, global=...) at /build/firefox-F4xXKx/firefox-46.0.1/js/src/vm/ScopeObject.cpp:976
#7 0xfff80001029b43fc in js::GlobalObject::createInternal(cx=cx@entry=0xfff800010a73a400, clasp=0xfff8000103e07028<JSRuntime::createSelfHostingGlobal(JSContext*)::shgClass>) at/build/firefox-F4xXKx/firefox-46.0.1/js/src/vm/GlobalObject.cpp:278
#8 0xfff8000102a062bc in JSRuntime::createSelfHostingGlobal(cx=cx@entry=0xfff800010a73a400) at /build/firefox-F4xXKx/firefox-46.0.1/js/src/vm/SelfHosting.cpp:1956
#9 0xfff8000102a08988 in JSRuntime::initSelfHosting
(this=this@entry=0xfff800010c3d2000, cx=cx@entry=0xfff800010a73a400)
at /build/firefox-F4xXKx/firefox-46.0.1/js/src/vm/SelfHosting.cpp:1988
#10 0xfff8000102876f74 in js::NewContext (rt=0xfff800010c3d2000,rt@entry=0x0, stackChunkSize=stackChunkSize@entry=8192)
at /build/firefox-F4xXKx/firefox-46.0.1/js/src/jscntxt.cpp:121
#11 0xfff8000102876fbc in JS_NewContext (rt=0x0,stackChunkSize=stackChunkSize@entry=8192) at/build/firefox-F4xXKx/firefox-46.0.1/js/src/jsapi.cpp:573
#12 0xfff8000101058308 in XPCJSContextStack::InitSafeJSContext(this=0xfff800010b248240) at/build/firefox-F4xXKx/firefox-46.0.1/js/xpconnect/src/XPCJSContextStack.cpp:122
#13 0xfff80001010738ac in nsXPConnect::InitStatics () at/build/firefox-F4xXKx/firefox-46.0.1/js/xpconnect/src/nsXPConnect.cpp:126
#14 0xfff800010105ccd4 in xpcModuleCtor () at/build/firefox-F4xXKx/firefox-46.0.1/js/xpconnect/src/XPCModule.cpp:13
#15 0xfff800010229b818 in Initialize () at/build/firefox-F4xXKx/firefox-46.0.1/layout/build/nsLayoutModule.cpp:436
#16 0xfff8000100bf9070 in nsComponentManagerImpl::KnownModule::Load(this=0xfff800010b22f280) at/build/firefox-F4xXKx/firefox-46.0.1/xpcom/components/nsComponentManager.cpp:898
#17 0xfff8000100bfa018 in nsFactoryEntry::GetFactory(this=0xfff800010b23a180) at/build/firefox-F4xXKx/firefox-46.0.1/xpcom/components/nsComponentManager.cpp:1934
#18 0xfff8000100bfa8d4 in nsComponentManagerImpl::CreateInstanceByContractID (this=0xfff800010a7f60c0, aContractID=0xfff8000102bb8b98 "@mozilla.org/moz/jsloader;1",aDelegate=0x0, aIID=..., Result=0x7feffffe930) at /build/firefox-F4xXKx/firefox-46.0.1/xpcom/components/nsComponentManager.cpp:1232
#19 0xfff8000100bfc298 in nsComponentManagerImpl::GetServiceByContractID (this=0xfff800010a7f60c0, aContractID=0xfff8000102bb8b98 "@mozilla.org/moz/jsloader;1",aIID=..., aResult=0x7feffffeac0) at /build/firefox-F4xXKx/firefox-46.0.1/xpcom/components/nsComponentManager.cpp:1592
#20 0xfff8000100c28514 in CallGetService (aContractID=0xfff8000102bab760 <nsISupports::COMTypeInfo<nsISupports, void>::kIID> "", aIID=..., aResult=0x0, aResult@entry=0x7feffffeac0) at
/build/firefox-F4xXKx/firefox-46.0.1/xpcom/glue/nsComponentManagerUtils.cpp:69
#21 0xfff8000100c28538 in nsGetServiceByContractID::operator() (this=this@entry=0x7feffffeb58, aIID=..., aInstancePtr=aInstancePtr@entry=0x7feffffeac0) at
/build/firefox-F4xXKx/firefox-46.0.1/xpcom/glue/nsComponentManagerUtils.cpp:280
#22 0xfff8000100c202b0 in nsCOMPtr_base::assign_from_gs_contractid (this=this@entry=0x7feffffeba0, aGS=..., aIID=...) at /build/firefox-F4xXKx/firefox-46.0.1/xpcom/glue/nsCOMPtr.cpp:103
#23 0xfff8000100c24c28 in nsCOMPtr<nsISupports>::nsCOMPtr (aGS=..., this=0x7feffffeba0) at /build/firefox-F4xXKx/firefox-46.0.1/build-browser/dist/include/nsCOMPtr.h:855
#24 NS_InitXPCOM2 (aResult=0x7feffffedd8, aBinDirectory=<optimized out>, aAppFileLocationProvider=<optimized out>) at /build/firefox-F4xXKx/firefox-46.0.1/xpcom/build/XPCOMInit.cpp:724
---Type <return> to continue, or q <return> to quit---
#25 0xfff8000100c24de0 in NS_InitXPCOM2 (aResult=0x1, aBinDirectory=0x7fefffff598, aAppFileLocationProvider=0x7fefffff5a8, aAppFileLocationProvider@entry=0x7feffffee90) at /build/firefox-F4xXKx/firefox-46.0.1/xpcom/build/XPCOMInit.cpp:481
#26 0xfff800010107a794 in XRE_XPCShellMain (argc=1, argv=0x7fefffff598, envp=0x7fefffff5a8) at /build/firefox-F4xXKx/firefox-46.0.1/js/xpconnect/src/XPCShellImpl.cpp:1424
#27 0x0000010000004710 in main (argc=1, argv=0x7fefffff598, envp=0x7fefffff5a8) at /build/firefox-ORgwZM/firefox-46.0.1/js/xpconnect/shell/xpcshell.cpp:54
(gdb)
When I print (this), I get the following:
(gdb) print *this
Cannot access memory at address 0x116622060
and
(gdb) print/x *this
$4 = {<js::WriteBarrieredBase<JS::Value>> = {<js::BarrieredBase<JS::Value>> = {<js::BarrieredBaseMixins<JS::Value>> = {<js::ValueOperations<js::WriteBarrieredBase<JS::Value> >> = {<No data fields>}, <No data fields>}, value = {data = {asBits = 0xfffc000116622060, debugView = {tag = 0x1fff8, payload47 = 0x116622060}, s = {padding = 0xfffc0001, payload = { i32 = 0x16622060, u32 = 0x16622060, why = 0x16622060}}, asDouble = 0x8000000000000000, asPtr = 0xfffc000116622060, asWord = 0xfffc000116622060, asUIntPtr = 0xfffc000116622060}}}, <No data fields>}, <No data fields>}
Thus, an issue with JSValue again like it has been seen on ia64 [3] and NetBSD/sparc64 [4].
As a first hack, I extended all the ia64 #ifdefs from [3] to apply on Linux/sparc64 but then xpcshell crashes with an out of memory error. This is caused by the fact that mmap on Linux completely ignores the hint when the memory area has already been allocated and unlike NetBSD, it will just allocate memory from the upper region again.
This can be demonstrated with a small piece of code both run on Linux and NetBSD:
On Linux:
glaubitz@ikarus:~$ cat vmtest.c
#include <stdio.h>
#include <sys/mman.h>
int main(void)
{
void *ptr1, *ptr2, *ptr3, *ptr4, *ptr5, *ptr6;
ptr1 = mmap(NULL, 1024*1024, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
ptr2 = mmap((void*)0x0000070000000000ULL, 1024*1024, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
ptr3 = mmap((void*)0x0000070000000000ULL, 1024*1024, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
ptr4 = mmap((void*)0x0000070000000000ULL, 1024*1024, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
ptr5 = mmap((void*)0x0000070000000000ULL, 1024*1024, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
ptr6 = mmap(NULL, 1024*1024, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
printf("%p %p %p %p %p %p\n", ptr1, ptr2, ptr3, ptr4, ptr5, ptr6);
}
glaubitz@ikarus:~$ ./vmtest
0x7f80c32f6000 0x70000000000 0x7f80c2d5d000 0x7f80c2c5d000 0x7f80c2b5d000 0x7f80c2a5d000
glaubitz@ikarus:~$ uname -vm
#1 SMP Debian 4.5.1-1 (2016-04-14) x86_64
glaubitz@ikarus:~$
On NetBSD:
testbsd# cat vmtest.c
#include <stdio.h>
#include <sys/mman.h>
int main(void)
{
void *ptr1, *ptr2, *ptr3, *ptr4, *ptr5, *ptr6;
ptr1 = mmap(NULL, 1024*1024, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
ptr2 = mmap((void*)0x0000070000000000ULL, 1024*1024, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
ptr3 = mmap((void*)0x0000070000000000ULL, 1024*1024, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
ptr4 = mmap((void*)0x0000070000000000ULL, 1024*1024, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
ptr5 = mmap((void*)0x0000070000000000ULL, 1024*1024, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
ptr6 = mmap(NULL, 1024*1024, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
printf("%p %p %p %p %p %p\n", ptr1, ptr2, ptr3, ptr4, ptr5, ptr6);
}
testbsd# ./vmtest
0x7f7ff7ef8000 0x70000000000 0x6fffff00000 0x6ffffe00000 0x6ffffd00000 0x7f7ff7b00000
testbsd# uname -vm
NetBSD 7.0 (GENERIC.201509250726Z) amd64
testbsd#
As you can see, on NetBSD, mmmap with a hint will always return memory in the requested region, even if the memory has already been allocated before. On Linux, the hint is ignored when the memory pointed to by it is already allocated.
As a crude hack, I subtracted the value of random() from "hint" in the corresponding mmap() calls which are found in the ia64 #ifdefs which I have activated for Linux/sparc64, too.
With this crude hack, I can actually xpcshell without crashing:
(sid)root@deb4g:/build/firefox-ORgwZM/firefox-46.0.1# export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:/build/firefox-ORgwZM/firefox-46.0.1/build-browser/dist/bin/
(sid)root@deb4g:/build/firefox-ORgwZM/firefox-46.0.1# ./build-browser/dist/bin/xpcshell
(process:186356): Gtk-WARNING **: Locale not supported by C library.
Using the fallback 'C' locale.
js> print ("Hello World!");
Hello World!
js> ^C
(sid)root@deb4g:/build/firefox-ORgwZM/firefox-46.0.1#
But that works for simple applications only. Running something more complex will still make xpcshell crash, I assume because there is much more memory being allocated with mmap and at some point, subtracting random() from "hint" will again yields a value in the upper memory area:
(sid)root@deb4g:/build/firefox-ORgwZM/firefox-46.0.1# /build/firefox-ORgwZM/firefox-46.0.1/build-browser/dist/bin/xpcshell -g /build/firefox-ORgwZM/firefox-46.0.1/build-browser/dist/bin/ -a /build/firefox-ORgwZM/firefox-46.0.1/build-browser/dist/bin/ -f /build/firefox-ORgwZM/firefox-46.0.1/toolkit/mozapps/installer/precompile_cache.js -e precompile_startupcache\("resource://gre/"\);
(process:186399): Gtk-WARNING **: Locale not supported by C library.
Using the fallback 'C' locale.
Bus error
(sid)root@deb4g:/build/firefox-ORgwZM/firefox-46.0.1# /build/firefox-ORgwZM/firefox-46.0.1/build-browser/dist/bin/xpcshell -g /build/firefox-ORgwZM/firefox-46.0.1/build-browser/dist/bin/ -a /build/firefox-ORgwZM/firefox-46.0.1/build-browser/dist/bin/ -f /build/firefox-ORgwZM/firefox-46.0.1/toolkit/mozapps/installer/precompile_cache.js -e precompile_startupcache\("resource://gre/"\);
(process:186442): Gtk-WARNING **: Locale not supported by C library.
Using the fallback 'C' locale.
Segmentation fault
(sid)root@deb4g:/build/firefox-ORgwZM/firefox-46.0.1#
In any case, we have now found the actual reason why JavaScript crashes on Linux/sparc64. But I have no idea how to fix this issue, honestly, given the fact that mmap() doesn't behave as nicely as it does on NetBSD. But I think the problem should be tackled, not just for sparc64, but in general since it too much depends on the underlying hardware and the behavior of mmap().
I have also tracked some of the debugging in a corresponding Debian bug report [5].
Adrian
> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1273764
> [2] https://buildd.debian.org/status/fetch.php?pkg=firefox&arch=sparc64&ver=46.0.1-1&stamp=1463370916
> [3] https://bugzilla.mozilla.org/show_bug.cgi?id=589735
> [4] https://bugzilla.mozilla.org/show_bug.cgi?id=994133
> [5] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=824449
Comment 1•8 years ago
|
||
1. OK, so just to get the most basic thing out of the way first, can you build with --enable-debug and --disable-optimize? We would expect to see an assertion failure before you get this far. 2. On ia64, we rely on the fact that addresses are really only 48-bit: 0000 0000 0000 0000 to 0000 7fff ffff ffff, and ffff 8000 0000 0000 to ffff ffff ffff ffff (All bits to the left of bit 48 are the same as bit 48.) Is this the case on SPARC 64? 3. Furthermore, we need addresses in the upper half of the range. The actual allocation occurs in js/src/gc/Memory.cpp, function MapMemory(); and/or mfbt/TaggedAnonymousMemory.cpp, MozTaggedAnonymousMmap(). The comment in MapMemory() makes it sound like you removing the `defined(__NetBSD__)` part of the #ifdef condition might be worth a shot.
Assignee | ||
Comment 2•8 years ago
|
||
Are you interested in getting an account on a sparc64 porterbox? Might be easier and faster. This machine is fast enough to build Firefox in around 2 hours.
Assignee | ||
Comment 3•8 years ago
|
||
As for the pointers: All the bits left of bit 52 are the same as bit 52 on this particular SPARC machine (SPARC-T5). Older SPARC CPUs may use less address bits.
Comment 4•8 years ago
|
||
We don't have a Value representation scheme that can support 52-bit addresses, so this might be a lost cause. However, option #3 is your best bet if you're willing to try something. I don't have time to look into it myself.
Assignee | ||
Comment 5•8 years ago
|
||
(In reply to Jason Orendorff [:jorendorff] from comment #4) > We don't have a Value representation scheme that can support 52-bit > addresses, so this might be a lost cause. Well, chances are that x86_64 will use more address space in the future, too, as 48 bits are already exhausted at 256 TiB RAM, so I'm not sure whether this is a safe bet. > However, option #3 is your best bet if you're willing to try something. As I said, I have tried the NetBSD fix. However, that doesn't work on Linux since mmap() doesn't behave as nicely on Linux as it does on NetBSD. The "hint" is only honored when the memory region pointed to is not already allocated.
Comment 6•8 years ago
|
||
In order to do that, we'd have to move Number back to the heap. That's not necessarily the end of the world, now that most performance critical code is using TypedArrays and TypedObject. It is, however, also not our highest priority at the moment, given that none of our tier 1 platforms have this issue. In the short term, you can probably paper over the issue by pre-mmaping bits 48 to 52 at process startup.
Assignee | ||
Comment 7•8 years ago
|
||
Hi Terrence! (In reply to Terrence Cole [:terrence] from comment #6) > In the short term, you can probably paper over the issue by pre-mmaping bits > 48 to 52 at process startup. Sorry, I just saw your comment now. Can you elaborate more on how that would work? Wouldn't this actually eat up quite some memory? Or is it possible to pre-mmap the memory without actually occupying any memory? Adrian
Comment 8•8 years ago
|
||
Mapping memory doesn't actually allocate the physical pages unless you touch them, so assuming something else isn't using an address with those bits set, a simple mmap call should work. If something *is* using addresses with the high bit set, it'll probably be pretty annoying to map everything around it, but I guess you could do some sort of binary search.
Assignee | ||
Comment 9•8 years ago
|
||
I have come up with the attached patch which first fixes the pre-processor definitions for sparc64 [1]. Then I tried using the allocator for ARM64 [2] on sparc64 as well in the hope that this would fix the problem. Unfortunately, I still see the crashes on sparc64. But maybe I'm just missing something obvious.
> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1273764
Assignee | ||
Comment 10•8 years ago
|
||
The link for [2] should be: https://bugzilla.mozilla.org/show_bug.cgi?id=1143022
Assignee | ||
Updated•8 years ago
|
Version: 46 Branch → Trunk
Assignee | ||
Comment 11•8 years ago
|
||
Hi!
The attached patch fixes the build on Linux/sparc64 for me. With the patch applied, I can successfully build Firefox 50.0 on Debian/sparc64.
I assume the patch cannot be merged "as is" and I particularly think that it could be split into parts. However, I would like to get some feedback first.
This patch also fixes #1273764 [1].
Thanks,
Adrian
> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1273764
Attachment #8817844 -
Attachment is obsolete: true
Attachment #8818000 -
Flags: review?(nfroyd)
Attachment #8818000 -
Flags: review?(mh+mozilla)
Attachment #8818000 -
Flags: review?(martin)
Attachment #8818000 -
Flags: review?(emanuel.hoogeveen)
Updated•8 years ago
|
Attachment #8818000 -
Flags: review?(martin) → review+
Assignee | ||
Comment 12•8 years ago
|
||
Updated patch, cleaned up some of the comments.
Attachment #8818006 -
Flags: review?(nfroyd)
Attachment #8818006 -
Flags: review?(mh+mozilla)
Attachment #8818006 -
Flags: review?(martin)
Attachment #8818006 -
Flags: review?(emanuel.hoogeveen)
Assignee | ||
Updated•8 years ago
|
Attachment #8818000 -
Attachment is obsolete: true
Attachment #8818000 -
Flags: review?(nfroyd)
Attachment #8818000 -
Flags: review?(mh+mozilla)
Attachment #8818000 -
Flags: review?(emanuel.hoogeveen)
Assignee | ||
Comment 13•8 years ago
|
||
Another update. With this update, the ARM64 allocator is now only used on Linux only (defined(__linux__)). *BSD doesn't need to use the allocator as on *BSD, mmap() always honours the hint parameter and returns memory in the vicinity of the address hint is pointing to. Linux doesn't do that, unfortunately which is why the manual allocator is necessary.
Attachment #8818006 -
Attachment is obsolete: true
Attachment #8818006 -
Flags: review?(nfroyd)
Attachment #8818006 -
Flags: review?(mh+mozilla)
Attachment #8818006 -
Flags: review?(martin)
Attachment #8818006 -
Flags: review?(emanuel.hoogeveen)
Attachment #8818007 -
Flags: review?(nfroyd)
Attachment #8818007 -
Flags: review?(mh+mozilla)
Attachment #8818007 -
Flags: review?(martin)
Attachment #8818007 -
Flags: review?(emanuel.hoogeveen)
Comment 14•8 years ago
|
||
Comment on attachment 8818007 [details] [diff] [review] linux-sparc64-fixes.patch Review of attachment 8818007 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for these fixes. r=me on the ipc/ and xpcom/ changes; please split those changes out into separate patches for commit purposes. If you split out the toolkit/ changes and address the comments there, I can review that patch. I will leave other changes for the appropriate people to review. ::: firefox-50.0.2.orig/ipc/chromium/src/build/build_config.h @@ +83,4 @@ > #elif defined(__ppc__) || defined(__powerpc__) > #define ARCH_CPU_PPC 1 > #define ARCH_CPU_32_BITS 1 > +#elif defined(__sparc__) && defined(__arch64__) This change is OK. ::: firefox-50.0.2.orig/media/webrtc/trunk/build/build_config.h @@ +129,4 @@ > #define ARCH_CPU_PPC 1 > #define ARCH_CPU_32_BITS 1 > #define ARCH_CPU_BIG_ENDIAN 1 > +#elif defined(__sparc__) && defined(__arch64__) The changes in media/webrtc/ should be split out into their own patch and reviewed by :jesup. ::: firefox-50.0.2.orig/toolkit/components/protobuf/src/google/protobuf/stubs/platform_macros.h @@ +67,4 @@ > #define GOOGLE_PROTOBUF_ARCH_32_BIT 1 > #elif defined(sparc) > #define GOOGLE_PROTOBUF_ARCH_SPARC 1 > +#if defined(__sparc_v9__) || defined(__sparcv9) || defined(__arch64__) In addition to this change, any changes in toolkit/components/protobuf/ need to be added to the m-c-changes.patch file in that directory. ::: firefox-50.0.2.orig/widget/gonk/libui/sha1.c @@ +87,4 @@ > > /* old sparc64 gcc could not compile this */ > #undef SPARC64_GCC_WORKAROUND > +#if defined(__sparc__) && defined(__arch64__) && defined(__GNUC__) && __GNUC__ < 3 Is this change just due to __sparc64__ cleanup? widget/gonk/ is probably never going to be built for sparc64... ::: firefox-50.0.2.orig/xpcom/reflect/xptcall/md/unix/moz.build @@ +232,4 @@ > 'xptcstubs_ppc_openbsd.cpp', > ] > > +if CONFIG['OS_ARCH'] == 'Linux' and CONFIG['OS_TEST'] == 'sparc': The changes in this file are OK.
Attachment #8818007 -
Flags: review?(nfroyd) → review+
Updated•8 years ago
|
Attachment #8818007 -
Flags: review?(martin) → review+
Assignee | ||
Comment 15•8 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #14) > r=me on the ipc/ and xpcom/ changes; please split those changes out into > separate patches for commit purposes. If you split out the toolkit/ changes > and address the comments there, I can review that patch. I will leave other > changes for the appropriate people to review. Ok, thanks. Will do! > ::: firefox-50.0.2.orig/media/webrtc/trunk/build/build_config.h > @@ +129,4 @@ > > #define ARCH_CPU_PPC 1 > > #define ARCH_CPU_32_BITS 1 > > #define ARCH_CPU_BIG_ENDIAN 1 > > +#elif defined(__sparc__) && defined(__arch64__) > > The changes in media/webrtc/ should be split out into their own patch and > reviewed by :jesup. Noted, thanks. > ::: > firefox-50.0.2.orig/toolkit/components/protobuf/src/google/protobuf/stubs/ > platform_macros.h > @@ +67,4 @@ > > #define GOOGLE_PROTOBUF_ARCH_32_BIT 1 > > #elif defined(sparc) > > #define GOOGLE_PROTOBUF_ARCH_SPARC 1 > > +#if defined(__sparc_v9__) || defined(__sparcv9) || defined(__arch64__) > > In addition to this change, any changes in toolkit/components/protobuf/ need > to be added to the m-c-changes.patch file in that directory. Ah, that makes sense. Although the protobuf was actually copied verbatim from upstream. Otherwise I had used defined(__sparc__) && defined(__arch64__). See: https://github.com/google/protobuf/blob/master/src/google/protobuf/stubs/platform_macros.h#L68 > ::: firefox-50.0.2.orig/widget/gonk/libui/sha1.c > @@ +87,4 @@ > > > > /* old sparc64 gcc could not compile this */ > > #undef SPARC64_GCC_WORKAROUND > > +#if defined(__sparc__) && defined(__arch64__) && defined(__GNUC__) && __GNUC__ < 3 > > Is this change just due to __sparc64__ cleanup? widget/gonk/ is probably > never going to be built for sparc64... Yeah, just for consistency reasons. > ::: firefox-50.0.2.orig/xpcom/reflect/xptcall/md/unix/moz.build > @@ +232,4 @@ > > 'xptcstubs_ppc_openbsd.cpp', > > ] > > > > +if CONFIG['OS_ARCH'] == 'Linux' and CONFIG['OS_TEST'] == 'sparc': > > The changes in this file are OK. Ok. I think I will split out the xpcom and ipc changes first in a separate patch, then we can have a look how to proceed with the remaining changes. Thanks, Adrian
Assignee | ||
Comment 16•8 years ago
|
||
Hi Nathan! Here's the first split-out patch. Adrian
Attachment #8818015 -
Flags: review?(nfroyd)
Assignee | ||
Comment 17•8 years ago
|
||
Attachment #8818015 -
Attachment is obsolete: true
Attachment #8818015 -
Flags: review?(nfroyd)
Attachment #8818016 -
Flags: review?(nfroyd)
Assignee | ||
Comment 18•8 years ago
|
||
Attachment #8818017 -
Flags: review?(nfroyd)
Updated•8 years ago
|
Attachment #8818016 -
Flags: review?(nfroyd) → review+
Updated•8 years ago
|
Attachment #8818017 -
Flags: review?(nfroyd) → review+
Assignee | ||
Updated•8 years ago
|
Attachment #8818007 -
Attachment is obsolete: true
Attachment #8818007 -
Flags: review?(mh+mozilla)
Attachment #8818007 -
Flags: review?(emanuel.hoogeveen)
Assignee | ||
Comment 19•8 years ago
|
||
Attachment #8818026 -
Flags: review?(mh+mozilla)
Attachment #8818026 -
Flags: review?(martin)
Assignee | ||
Comment 20•8 years ago
|
||
Attachment #8818027 -
Flags: review?(mh+mozilla)
Attachment #8818027 -
Flags: review?(martin)
Assignee | ||
Comment 21•8 years ago
|
||
Attachment #8818028 -
Flags: review?(mh+mozilla)
Attachment #8818028 -
Flags: review?(martin)
Assignee | ||
Comment 22•8 years ago
|
||
Attachment #8818029 -
Flags: review?(mh+mozilla)
Attachment #8818029 -
Flags: review?(martin)
Assignee | ||
Comment 23•8 years ago
|
||
Attachment #8818030 -
Flags: review?(mh+mozilla)
Attachment #8818030 -
Flags: review?(martin)
Assignee | ||
Comment 24•8 years ago
|
||
Attachment #8818031 -
Flags: review?(mh+mozilla)
Attachment #8818031 -
Flags: review?(martin)
Assignee | ||
Comment 25•8 years ago
|
||
Attachment #8818032 -
Flags: review?(mh+mozilla)
Attachment #8818032 -
Flags: review?(martin)
Assignee | ||
Comment 26•8 years ago
|
||
Split everything out. Please note 0002-Bug-1275204-js-Use-the-arm64-allocator-on-Linux-spar.patch must be applied after 0001-js-Use-better-pre-processor-defines-for-sparc64.patch, but the rest can be applied out of order.
Assignee | ||
Comment 27•8 years ago
|
||
Added the change for widget/gonk for consistency reasons.
Attachment #8818036 -
Flags: review?(mh+mozilla)
Attachment #8818036 -
Flags: review?(martin)
Assignee | ||
Comment 28•8 years ago
|
||
Oops, I accidentally messed up this patch when splitting it out.
Attachment #8818027 -
Attachment is obsolete: true
Attachment #8818027 -
Flags: review?(mh+mozilla)
Attachment #8818027 -
Flags: review?(martin)
Attachment #8818045 -
Flags: review?(mh+mozilla)
Attachment #8818045 -
Flags: review?(martin)
Assignee | ||
Comment 29•8 years ago
|
||
Attachment #8818016 -
Attachment is obsolete: true
Attachment #8818064 -
Flags: review?(nfroyd)
Assignee | ||
Comment 30•8 years ago
|
||
Attachment #8818017 -
Attachment is obsolete: true
Attachment #8818065 -
Flags: review?(nfroyd)
Assignee | ||
Comment 31•8 years ago
|
||
Attachment #8818026 -
Attachment is obsolete: true
Attachment #8818026 -
Flags: review?(mh+mozilla)
Attachment #8818026 -
Flags: review?(martin)
Attachment #8818068 -
Flags: review?(mh+mozilla)
Attachment #8818068 -
Flags: review?(martin)
Attachment #8818068 -
Flags: review?(emanuel.hoogeveen)
Assignee | ||
Comment 32•8 years ago
|
||
Attachment #8818030 -
Attachment is obsolete: true
Attachment #8818030 -
Flags: review?(mh+mozilla)
Attachment #8818030 -
Flags: review?(martin)
Attachment #8818070 -
Flags: review?(mh+mozilla)
Attachment #8818070 -
Flags: review?(martin)
Attachment #8818070 -
Flags: review?(emanuel.hoogeveen)
Assignee | ||
Updated•8 years ago
|
Attachment #8818028 -
Attachment is obsolete: true
Attachment #8818028 -
Flags: review?(mh+mozilla)
Attachment #8818028 -
Flags: review?(martin)
Assignee | ||
Updated•8 years ago
|
Attachment #8818029 -
Attachment is obsolete: true
Attachment #8818029 -
Flags: review?(mh+mozilla)
Attachment #8818029 -
Flags: review?(martin)
Comment 33•8 years ago
|
||
Comment on attachment 8818064 [details] [diff] [review] 0001-Use-OpenBSD-sparc64-xptcall-stubs-on-Linux-sparc64.patch Review of attachment 8818064 [details] [diff] [review]: ----------------------------------------------------------------- Didn't I already review this? You may want to look into using https://github.com/mozilla/git-bz-moz so you can upload individual patches and so you don't have to export the whole series everytime...
Attachment #8818064 -
Flags: review?(nfroyd) → review+
Assignee | ||
Updated•8 years ago
|
Attachment #8818031 -
Attachment is obsolete: true
Attachment #8818031 -
Flags: review?(mh+mozilla)
Attachment #8818031 -
Flags: review?(martin)
Updated•8 years ago
|
Attachment #8818065 -
Flags: review?(nfroyd) → review+
Assignee | ||
Updated•8 years ago
|
Attachment #8818032 -
Attachment is obsolete: true
Attachment #8818032 -
Flags: review?(mh+mozilla)
Attachment #8818032 -
Flags: review?(martin)
Assignee | ||
Updated•8 years ago
|
Attachment #8818036 -
Attachment is obsolete: true
Attachment #8818036 -
Flags: review?(mh+mozilla)
Attachment #8818036 -
Flags: review?(martin)
Assignee | ||
Updated•8 years ago
|
Attachment #8818045 -
Attachment is obsolete: true
Attachment #8818045 -
Flags: review?(mh+mozilla)
Attachment #8818045 -
Flags: review?(martin)
Assignee | ||
Comment 34•8 years ago
|
||
Attachment #8818073 -
Flags: review?(mh+mozilla)
Attachment #8818073 -
Flags: review?(martin)
Attachment #8818073 -
Flags: review?(emanuel.hoogeveen)
Assignee | ||
Comment 35•8 years ago
|
||
Attachment #8818074 -
Flags: review?(mh+mozilla)
Attachment #8818074 -
Flags: review?(martin)
Attachment #8818074 -
Flags: review?(emanuel.hoogeveen)
Assignee | ||
Comment 36•8 years ago
|
||
Attachment #8818076 -
Flags: review?(mh+mozilla)
Attachment #8818076 -
Flags: review?(martin)
Attachment #8818076 -
Flags: review?(emanuel.hoogeveen)
Assignee | ||
Comment 37•8 years ago
|
||
Attachment #8818077 -
Flags: review?(mh+mozilla)
Attachment #8818077 -
Flags: review?(martin)
Attachment #8818077 -
Flags: review?(emanuel.hoogeveen)
Assignee | ||
Comment 38•8 years ago
|
||
Attachment #8818078 -
Flags: review?(mh+mozilla)
Attachment #8818078 -
Flags: review?(martin)
Attachment #8818078 -
Flags: review?(emanuel.hoogeveen)
Assignee | ||
Comment 39•8 years ago
|
||
Attachment #8818079 -
Flags: review?(mh+mozilla)
Attachment #8818079 -
Flags: review?(martin)
Attachment #8818079 -
Flags: review?(emanuel.hoogeveen)
Assignee | ||
Comment 40•8 years ago
|
||
Sorry for the noise, but I forgot to add the bug number for some of the patches.
Updated•8 years ago
|
Attachment #8818068 -
Flags: review?(martin) → review+
Updated•8 years ago
|
Attachment #8818079 -
Flags: review?(martin) → review+
Updated•8 years ago
|
Attachment #8818078 -
Flags: review?(martin) → review+
Updated•8 years ago
|
Attachment #8818077 -
Flags: review?(martin) → review+
Updated•8 years ago
|
Attachment #8818076 -
Flags: review?(martin) → review+
Updated•8 years ago
|
Attachment #8818074 -
Flags: review?(martin) → review+
Updated•8 years ago
|
Attachment #8818073 -
Flags: review?(martin) → review+
Updated•8 years ago
|
Attachment #8818070 -
Flags: review?(martin) → review+
Updated•7 years ago
|
Attachment #8818068 -
Flags: review?(mh+mozilla)
Attachment #8818068 -
Flags: review?(emanuel.hoogeveen)
Attachment #8818068 -
Flags: review+
Comment 41•7 years ago
|
||
Comment on attachment 8818070 [details] [diff] [review] 0004-Bug-1275204-js-Use-the-arm64-allocator-on-Linux-spar.patch Review of attachment 8818070 [details] [diff] [review]: ----------------------------------------------------------------- This seems reasonable, but Emanuel should probably take another look.
Attachment #8818070 -
Flags: review?(mh+mozilla) → review+
Comment 42•7 years ago
|
||
Comment on attachment 8818073 [details] [diff] [review] 0005-Bug-1273764-jemalloc-Use-better-pre-processor-define.patch Review of attachment 8818073 [details] [diff] [review]: ----------------------------------------------------------------- ::: memory/jemalloc/src/include/jemalloc/internal/mb.h @@ +76,4 @@ > : "memory" /* Clobbers. */ > ); > } > +#elif defined(__sparc__) && defined(__arch64__) Please submit this upstream to https://github.com/jemalloc/jemalloc. We avoid patching directly our copy.
Attachment #8818073 -
Flags: review?(mh+mozilla)
Attachment #8818073 -
Flags: review?(emanuel.hoogeveen)
Attachment #8818073 -
Flags: review-
Updated•7 years ago
|
Attachment #8818074 -
Flags: review?(mh+mozilla)
Attachment #8818074 -
Flags: review?(emanuel.hoogeveen)
Attachment #8818074 -
Flags: review+
Assignee | ||
Comment 43•7 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #42) > Please submit this upstream to https://github.com/jemalloc/jemalloc. > > We avoid patching directly our copy. No problem. Doing that right away.
Assignee | ||
Comment 44•7 years ago
|
||
(In reply to John Paul Adrian Glaubitz from comment #43) > (In reply to Mike Hommey [:glandium] from comment #42) > > Please submit this upstream to https://github.com/jemalloc/jemalloc. > > > > We avoid patching directly our copy. > > No problem. Doing that right away. Opened a PR: https://github.com/jemalloc/jemalloc/pull/529
Comment 45•7 years ago
|
||
Comment on attachment 8818076 [details] [diff] [review] 0007-Bug-1275204-mozjemalloc-Use-the-JS-arm64-allocator-o.patch Review of attachment 8818076 [details] [diff] [review]: ----------------------------------------------------------------- It rubs me in the wrong direction that this essentially duplicates code from the js engine. As in, it actually rubs me in the wrong direction that the js engine has logic to deal with things like this on its own. It also seems like this could quickly create mmap loops depending on how the kernel actually allocates VMAs on those architectures. ::: memory/mozjemalloc/jemalloc.c @@ +2440,5 @@ > } > #endif > > +#if defined(__sparc__) && defined(__arch64__) && defined(__linux__) > + const uintptr_t start = UINT64_C(0x0000070000000000); The style in this file would be to use the ULL suffix instead of using some macro from mfbt/decimal. @@ +2442,5 @@ > > +#if defined(__sparc__) && defined(__arch64__) && defined(__linux__) > + const uintptr_t start = UINT64_C(0x0000070000000000); > + const uintptr_t end = UINT64_C(0x0000800000000000); > + const uintptr_t step = 8 << 20; /* This is supposed to be ChunkSize */ This should be the mozjemalloc chunk size here. AKA chunksize (or a multiple of it). @@ +2444,5 @@ > + const uintptr_t start = UINT64_C(0x0000070000000000); > + const uintptr_t end = UINT64_C(0x0000800000000000); > + const uintptr_t step = 8 << 20; /* This is supposed to be ChunkSize */ > + > + /* Copied from js/src/Memory.cpp and adapted for this source */ It's js/src/gc/Memory.cpp. Note the implementation in that file also says that it should be merged with the ia64/sparc64 netbsd implementation, and instead of adding this here, I'd rather try that.
Attachment #8818076 -
Flags: review?(mh+mozilla)
Attachment #8818076 -
Flags: review?(emanuel.hoogeveen)
Updated•7 years ago
|
Attachment #8818077 -
Flags: review?(rjesup)
Attachment #8818077 -
Flags: review?(mh+mozilla)
Attachment #8818077 -
Flags: review?(emanuel.hoogeveen)
Comment 46•7 years ago
|
||
Comment on attachment 8818078 [details] [diff] [review] 0009-Bug-1273764-protobuf-Sync-sparc64-pre-processor-defi.patch Review of attachment 8818078 [details] [diff] [review]: ----------------------------------------------------------------- This was fixed upstream almost a year ago. Time to upgrade?
Attachment #8818078 -
Flags: review?(mh+mozilla)
Attachment #8818078 -
Flags: review?(emanuel.hoogeveen)
Attachment #8818078 -
Flags: feedback?(nfitzgerald)
Comment 47•7 years ago
|
||
Comment on attachment 8818079 [details] [diff] [review] 0010-Bug-1273764-widget-gonk-Use-better-pre-processor-def.patch Review of attachment 8818079 [details] [diff] [review]: ----------------------------------------------------------------- widget/gonk is only built for b2g, which only really builds for arm/x86/mips.
Attachment #8818079 -
Flags: review?(mh+mozilla)
Attachment #8818079 -
Flags: review?(emanuel.hoogeveen)
Attachment #8818079 -
Flags: review+
Assignee | ||
Comment 48•7 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #45) > Comment on attachment 8818076 [details] [diff] [review] > 0007-Bug-1275204-mozjemalloc-Use-the-JS-arm64-allocator-o.patch > > Review of attachment 8818076 [details] [diff] [review]: > ----------------------------------------------------------------- > > It rubs me in the wrong direction that this essentially duplicates code from > the js engine. As in, it actually rubs me in the wrong direction that the js > engine has logic to deal with things like this on its own. True. But this is still a separate allocator and it essentially has the same problem as the JS engine, doesn't it? > It also seems like this could quickly create mmap loops depending on how the > kernel actually allocates VMAs on those architectures. I haven't had any issue with that though. With all my current patches applied and the two fixes from #1322660 and #1322112, Firefox works absolutely fine on sparc64 even on Javascript-heavy websites. > ::: memory/mozjemalloc/jemalloc.c > @@ +2440,5 @@ > > } > > #endif > > > > +#if defined(__sparc__) && defined(__arch64__) && defined(__linux__) > > + const uintptr_t start = UINT64_C(0x0000070000000000); > > The style in this file would be to use the ULL suffix instead of using some > macro from mfbt/decimal. Ok, noted. > @@ +2442,5 @@ > > > > +#if defined(__sparc__) && defined(__arch64__) && defined(__linux__) > > + const uintptr_t start = UINT64_C(0x0000070000000000); > > + const uintptr_t end = UINT64_C(0x0000800000000000); > > + const uintptr_t step = 8 << 20; /* This is supposed to be ChunkSize */ > > This should be the mozjemalloc chunk size here. AKA chunksize (or a multiple > of it). Yeah, definitely. I just didn't know where to take it from, so I was hard-coding it for the time being and hoping for some additional comment. > @@ +2444,5 @@ > > + const uintptr_t start = UINT64_C(0x0000070000000000); > > + const uintptr_t end = UINT64_C(0x0000800000000000); > > + const uintptr_t step = 8 << 20; /* This is supposed to be ChunkSize */ > > + > > + /* Copied from js/src/Memory.cpp and adapted for this source */ > > It's js/src/gc/Memory.cpp. Note the implementation in that file also says > that it should be merged with the ia64/sparc64 netbsd implementation, and > instead of adding this here, I'd rather try that. No, merging it with NetBSD/sparc64 would not be a good idea because the manual allocator is actually a dirty work-around and necessary on Linux only. The mmap() function in NetBSD always honors the hint parameter so that you don't have the allocating yourself to avoid getting memory from the region that is not compatible with JSValue. The manual allocator will most likely always have worse performance than anything offered by the kernel so I think we should definitely still leave NetBSD/sparc64 untouched.
Assignee | ||
Comment 49•7 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #47) > widget/gonk is only built for b2g, which only really builds for arm/x86/mips. I understand that. However, the defines used are still incorrect and I think it's generally better to have these fixed so if someone, for whatever reason, decided to use the code or pieces of it on sparc64, they would run into this problem again. I don't think it hurts at all to fix this bug.
Comment 50•7 years ago
|
||
Comment on attachment 8818078 [details] [diff] [review] 0009-Bug-1273764-protobuf-Sync-sparc64-pre-processor-defi.patch Review of attachment 8818078 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, thanks!
Attachment #8818078 -
Flags: feedback?(nfitzgerald) → feedback+
Comment 51•7 years ago
|
||
Comment on attachment 8818070 [details] [diff] [review] 0004-Bug-1275204-js-Use-the-arm64-allocator-on-Linux-spar.patch Review of attachment 8818070 [details] [diff] [review]: ----------------------------------------------------------------- This seems fine assuming it does what you want. r+ with the comments below addressed. ::: js/src/gc/Memory.cpp @@ +501,4 @@ > MapMemoryAt(void* desired, size_t length, int prot = PROT_READ | PROT_WRITE, > int flags = MAP_PRIVATE | MAP_ANON, int fd = -1, off_t offset = 0) > { > +#if defined(__ia64__) || (defined(__sparc__) && defined(__arch64__) && (defined(__NetBSD__) || defined(__linux__))) || defined(__aarch64__) Please split this line so it fits inside the 99 column limit. I suggest the following for clarity: #if defined(__ia64__) || defined(__aarch64__) || \ (defined(__sparc__) && defined(__arch64__) && (defined(__NetBSD__) || defined(__linux__))) ::: js/src/jsapi-tests/testGCAllocator.cpp @@ +312,4 @@ > void* > mapMemoryAt(void* desired, size_t length) > { > +#if defined(__ia64__) || (defined(__sparc__) && defined(__arch64__) && (defined(__NetBSD__) || defined(__linux__))) || defined(__aarch64__) Same here.
Attachment #8818070 -
Flags: review?(emanuel.hoogeveen) → review+
Comment 52•7 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #45) > It rubs me in the wrong direction that this essentially duplicates code from > the js engine. As in, it actually rubs me in the wrong direction that the js > engine has logic to deal with things like this on its own. Yeah, I believe njn suggested once to switch to _aligned_malloc for all of this (I can't find the bug now), but jemalloc still doesn't deal with chunk exhaustion as well as the GC allocator does. > It also seems like this could quickly create mmap loops depending on how the > kernel actually allocates VMAs on those architectures. It's not a workaround I'm particularly happy with, especially since it also potentially adds a lot of overhead on every allocation. The only ways I can see to fix it properly are to either support 48-bit addresses in the js engine (which might be doable, but IIUC it's a fair bit of work), or map large (e.g. 4GB) areas up front and allocate from those (only on 64-bit obviously). I've considered doing the latter for the GC since it would also effectively give us separate heaps, but it would be a fairly complex change and I don't see it happening for jemalloc unless they deal with it upstream.
Updated•7 years ago
|
Attachment #8818077 -
Flags: review?(rjesup) → review+
Assignee | ||
Comment 53•7 years ago
|
||
Hi! Sorry for the long delay. I assume that after adding the requested changes to 0004-Bug-1275204-js-Use-the-arm64-allocator-on-Linux-spar.patch, we can have this merged? (In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #52) > It's not a workaround I'm particularly happy with, especially since it also > potentially adds a lot of overhead on every allocation. The only ways I can > see to fix it properly are to either support 48-bit addresses in the js > engine (which might be doable, but IIUC it's a fair bit of work), or map > large (e.g. 4GB) areas up front and allocate from those (only on 64-bit > obviously). Please keep in mind that sparc64 has a 52-bit address space, so 48 free bits wouldn't be enough on this architecture.
Assignee | ||
Comment 54•7 years ago
|
||
Updated 0004-Bug-1275204-js-Use-the-arm64-allocator-on-Linux-spar.patch with the requested changes.
Attachment #8818070 -
Attachment is obsolete: true
Attachment #8821752 -
Flags: review?(emanuel.hoogeveen)
Comment 55•7 years ago
|
||
Comment on attachment 8821752 [details] [diff] [review] 0004-Bug-1275204-js-Use-the-arm64-allocator-on-Linux-spar.patch Thanks. Looking at the raw patch, it looks like you aren't using the usual diff format. Could you check https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_diff_and_patch_files.3F and maybe https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch to make sure you're using the standard format?
Attachment #8821752 -
Flags: review?(emanuel.hoogeveen) → review+
Assignee | ||
Comment 56•7 years ago
|
||
I have been working with a read-only git repository to create the patches. This has been fine in the past so far with previous patches I sent in and I can naturally use Mercurial for future patches. I hope these patches are fine for the time being though.
Comment 57•7 years ago
|
||
Alright, if you've had similar patches checked in before then it's probably fine :)
Comment 58•7 years ago
|
||
(In reply to John Paul Adrian Glaubitz from comment #49) > (In reply to Mike Hommey [:glandium] from comment #47) > > widget/gonk is only built for b2g, which only really builds for arm/x86/mips. > > I understand that. However, the defines used are still incorrect and I think > it's generally better to have these fixed so if someone, for whatever > reason, decided to use the code or pieces of it on sparc64, they would run > into this problem again. I don't think it hurts at all to fix this bug. If someone does something with that code, they should remove that copy of sha1.c and use one of the three other SHA1 implementations we have in mozilla-central (picking the one from MFBT would be the logical choice).
Updated•7 years ago
|
Attachment #8818079 -
Attachment is obsolete: true
Updated•7 years ago
|
Attachment #8818073 -
Attachment is obsolete: true
Updated•7 years ago
|
Assignee: nobody → glaubitz
Comment 60•7 years ago
|
||
Pushed by mh@glandium.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/c2b253c9b16d Use OpenBSD/sparc64 xptcall stubs on Linux/sparc64. r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/079ab3960351 ipc:chromium: Use better pre-processor defines for sparc64. r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/f9307a83e555 js: Use better pre-processor defines for sparc64. r=glandium https://hg.mozilla.org/integration/mozilla-inbound/rev/0cb0fe7e92f6 js: Use the arm64 allocator on Linux/sparc64. r=ehoogeveen https://hg.mozilla.org/integration/mozilla-inbound/rev/71d3800a2ef3 mozjemalloc: Use better pre-processor defines for sparc64. r=glandium https://hg.mozilla.org/integration/mozilla-inbound/rev/ed065d780ed3 mozjemalloc: Use the JS arm64 allocator on Linux/sparc64. r=glandium https://hg.mozilla.org/integration/mozilla-inbound/rev/c320f09ac65a media:webrtc: Use better pre-processor defines for sparc64. r=jesup https://hg.mozilla.org/integration/mozilla-inbound/rev/21d692e82582 protobuf: Sync sparc64 pre-processor defines from upstream. r=fitzgen
Assignee | ||
Comment 61•7 years ago
|
||
(In reply to John Paul Adrian Glaubitz from comment #44) > (In reply to John Paul Adrian Glaubitz from comment #43) > > (In reply to Mike Hommey [:glandium] from comment #42) > > > Please submit this upstream to https://github.com/jemalloc/jemalloc. > > > > > > We avoid patching directly our copy. > > > > No problem. Doing that right away. > > Opened a PR: https://github.com/jemalloc/jemalloc/pull/529 Update: This has been merged upstream now, see: https://github.com/jemalloc/jemalloc/commit/77de5f27d848414a6d26e86e2812339ffe1062d3 Can we pull this last patch in as well?
Comment 62•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c2b253c9b16d https://hg.mozilla.org/mozilla-central/rev/079ab3960351 https://hg.mozilla.org/mozilla-central/rev/f9307a83e555 https://hg.mozilla.org/mozilla-central/rev/0cb0fe7e92f6 https://hg.mozilla.org/mozilla-central/rev/71d3800a2ef3 https://hg.mozilla.org/mozilla-central/rev/ed065d780ed3 https://hg.mozilla.org/mozilla-central/rev/c320f09ac65a https://hg.mozilla.org/mozilla-central/rev/21d692e82582
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Assignee | ||
Comment 63•7 years ago
|
||
(In reply to Carsten Book [:Tomcat] from comment #62) > https://hg.mozilla.org/mozilla-central/rev/c2b253c9b16d > https://hg.mozilla.org/mozilla-central/rev/079ab3960351 > https://hg.mozilla.org/mozilla-central/rev/f9307a83e555 > https://hg.mozilla.org/mozilla-central/rev/0cb0fe7e92f6 > https://hg.mozilla.org/mozilla-central/rev/71d3800a2ef3 > https://hg.mozilla.org/mozilla-central/rev/ed065d780ed3 > https://hg.mozilla.org/mozilla-central/rev/c320f09ac65a > https://hg.mozilla.org/mozilla-central/rev/21d692e82582 We're still missing this patch: https://bug1275204.bmoattachments.org/attachment.cgi?id=8818073
Comment 64•7 years ago
|
||
(In reply to John Paul Adrian Glaubitz from comment #63) > We're still missing this patch: > https://bug1275204.bmoattachments.org/attachment.cgi?id=8818073 Comment 42 said to submit this upstream as we don't patch the in-tree copy of jemalloc directly.
Comment 65•7 years ago
|
||
Probably best to open a new bug about pulling in the latest version of jemalloc4 in case the new version requires more local changes (I don't know when we last updated the in-tree version).
Assignee | ||
Comment 66•7 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #64) > (In reply to John Paul Adrian Glaubitz from comment #63) > > We're still missing this patch: > > https://bug1275204.bmoattachments.org/attachment.cgi?id=8818073 > > Comment 42 said to submit this upstream as we don't patch the in-tree copy > of jemalloc directly. Ok, that happened yesterday: https://github.com/jemalloc/jemalloc/commit/77de5f27d848414a6d26e86e2812339ffe1062d3 So, I'll just have to wait until Mozilla pulls in the updated jemalloc version.
Comment 67•7 years ago
|
||
We just updated to 4.4.0 recently. Looks like your commit landed on the dev branch currently tracking jemalloc5, which AFAIK doesn't have any sort of targeted release date. However, I see that a 4.4.1 milestone was recently created as well, so you might be able to ask Jason to backport your fix to that as well.
Assignee | ||
Comment 68•7 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #67) > We just updated to 4.4.0 recently. Looks like your commit landed on the dev > branch currently tracking jemalloc5, which AFAIK doesn't have any sort of > targeted release date. However, I see that a 4.4.1 milestone was recently > created as well, so you might be able to ask Jason to backport your fix to > that as well. I just asked: https://github.com/jemalloc/jemalloc/pull/529#issuecomment-271906286
You need to log in
before you can comment on or make changes to this bug.
Description
•