Closed
Bug 915735
Opened 11 years ago
Closed 10 years ago
Build ICU as a shared library where JS is built as a shared library
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla28
People
(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 15 obsolete files)
1.40 KB,
patch
|
bugzilla
:
review+
|
Details | Diff | Splinter Review |
29.64 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
So, we need to be able to use the ICU Collator API for bug 871846. As far as I can tell, on Windows, ICU is linked into the mozjs DLL which makes it impossible to use it from inside xul.dll. This bug is about modifying the ICU build system to export the symbols so that we can use them from xul.dll. This is not an issue on non-Windows platforms. As far as I understand things, we just need to replace U_STATIC_IMPLEMENTATION with U_COMBINED_IMPLEMENTATION in configure.in. I'll give that a shot when I get my hands on a Windows box.
Comment 1•11 years ago
|
||
I don't know about Windows, but iirc the symbols *are* exported on Linux.
I haven't gotten this to work yet but you also need to add "-DU_COMBINED_IMPLEMENTATION" and "-DUNISTR_FROM_STRING_EXPLICIT= " to ICU_CPPFLAGS.
Assignee | ||
Comment 3•11 years ago
|
||
Sorry, I meant to look at this today and didn't get the time at all. I'll try to look at it again tomorrow.
Assignee | ||
Comment 4•11 years ago
|
||
Let's see if we can re-merge mozjs into libxul: <https://tbpl.mozilla.org/?tree=Try&rev=fca3fe53c591>
Assignee | ||
Comment 5•11 years ago
|
||
So it turns out that gfx/2d/ScaledFontCairo.cpp #includes gfxFont.h, which is outside of gfx/2d (which is supposed to be a standalone library as far as I understand) and also pulls in jsapi.h, which makes us export a bunch of JS symbols from gkmedias.dll as well. I have removed that #include locally, and am now rebuilding to see if that fixes the linking problems I have here.... /cc Bas and Jeff. I will be removing this #include shortly, but I'm wondering how this never broke the standalone gfx2d builds?
Assignee | ||
Comment 6•11 years ago
|
||
Fixed the problem I talked about in comment 5: https://hg.mozilla.org/integration/mozilla-inbound/rev/73a73041e094 With that, I can build fine locally. I'll push to try next.
Whiteboard: [leave open]
Assignee | ||
Comment 7•11 years ago
|
||
Assignee | ||
Comment 8•11 years ago
|
||
Attachment #806872 -
Attachment is obsolete: true
Assignee | ||
Comment 9•11 years ago
|
||
Try run for Windows PGO: https://tbpl.mozilla.org/?tree=Try&rev=f10ab7dbe376 Try run for all platforms: https://tbpl.mozilla.org/?tree=Try&rev=0f44bff53050
Assignee | ||
Updated•11 years ago
|
Summary: Build ICU with exported symbols → Fold mozjs.dll back into xul.dll
Comment 10•11 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #9) > Try run for Windows PGO: https://tbpl.mozilla.org/?tree=Try&rev=f10ab7dbe376 Same failure I got :(
Comment 11•11 years ago
|
||
On the plus side, it's just crashing running xpcshell for startupcache population. On the minus side, that means you didn't even get to the final PGO link, so we have no idea whether that works.
Assignee | ||
Comment 13•11 years ago
|
||
So I can reproduce the crash locally, by running:
$ ./obj-ff-pgo/dist/bin/xpcshell.exe -g obj-ff-pgo/dist/bin/ -a obj-ff-pgo/dist/bin/ -f toolkit/mozapps/installer/precompile_cache.js -e 'precompile_startupcache("resource://gre/");'
When I look at the crash under the debugger, it's a stack overflow in PGO instrumentation code:
> pgort100.dll!PVPLELookup() + 0x2 bytes
0023070d()
pgort100.dll!_PogoCommitVirtualMemory@8() + 0x18 bytes
pgort100.dll!IbPGCGrowAllocation() + 0x98 bytes
pgort100.dll!PVPLELookup() + 0x56 bytes
0023070d()
pgort100.dll!_PogoCommitVirtualMemory@8() + 0x18 bytes
pgort100.dll!IbPGCGrowAllocation() + 0x98 bytes
pgort100.dll!PVPLELookup() + 0x56 bytes
0023070d()
pgort100.dll!_PogoCommitVirtualMemory@8() + 0x18 bytes
pgort100.dll!IbPGCGrowAllocation() + 0x98 bytes
pgort100.dll!PVPLELookup() + 0x56 bytes
0023070d()
pgort100.dll!_PogoCommitVirtualMemory@8() + 0x18 bytes
pgort100.dll!IbPGCGrowAllocation() + 0x98 bytes
pgort100.dll!PVPLELookup() + 0x56 bytes
0023070d()
pgort100.dll!_PogoCommitVirtualMemory@8() + 0x18 bytes
pgort100.dll!IbPGCGrowAllocation() + 0x98 bytes
pgort100.dll!PVPLELookup() + 0x56 bytes
0023070d()
Basically, infinite recursion. :(
Assignee | ||
Comment 14•11 years ago
|
||
Sorry, the stack overflow was a red herring: <http://mxr.mozilla.org/mozilla-central/source/xpcom/base/AvailableMemoryTracker.cpp#525> With that environment variable set, this is the stack I get: > xul.dll!js::ObjectImpl::compartment() Line 1174 + 0xe bytes C++ xul.dll!JSAutoCompartment::JSAutoCompartment(JSContext * cx=0x0365cac0, JSObject * target=0x11733673) Line 1043 + 0xe bytes C++ xul.dll!JSRuntime::initSelfHosting(JSContext * cx=0x0165cac0) Line 693 + 0x15 bytes C++ xul.dll!js::NewContext(JSRuntime * rt=0x03672fe8, unsigned int stackChunkSize=1637368) Line 201 + 0x10 bytes C++ xul.dll!JS_NewContext(JSRuntime * rt=0x03672fe8, unsigned int stackChunkSize=8192) Line 828 + 0x9 bytes C++ xpcshell.exe!NS_internal_main(int argc=3, char * * argv=0x0236b930, char * * envp=0x02362da8) Line 1652 + 0xe bytes C++ xpcshell.exe!wmain(int argc=37140864, wchar_t * * argv=0x0236c798) Line 112 C++ xpcshell.exe!__tmainCRTStartup() Line 552 + 0x17 bytes C kernel32.dll!@BaseThreadInitThunk@12() + 0xe bytes ntdll.dll!___RtlUserThreadStart@8() + 0x27 bytes ntdll.dll!__RtlUserThreadStart@8() + 0x1b bytes
Assignee | ||
Comment 15•11 years ago
|
||
This seems to be a compiler bug. Disabling optimizations in JSRuntime::initSelfHosting fixes this locally for me.
Comment 16•11 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #15) > This seems to be a compiler bug. Disabling optimizations in > JSRuntime::initSelfHosting fixes this locally for me. That should be fine. That function is called once during startup so it shouldn't be super hot.
Assignee | ||
Comment 17•11 years ago
|
||
New try run: https://tbpl.mozilla.org/?tree=Try&rev=9e4f622a4f6d
Attachment #806874 -
Attachment is obsolete: true
Assignee | ||
Comment 18•11 years ago
|
||
Comment on attachment 807367 [details] [diff] [review] WIP 2 Review of attachment 807367 [details] [diff] [review]: ----------------------------------------------------------------- Asking Bobby to officiate comment 16. :-)
Attachment #807367 -
Flags: review?(bobbyholley+bmo)
Assignee | ||
Comment 19•11 years ago
|
||
Comment on attachment 807367 [details] [diff] [review] WIP 2 Review of attachment 807367 [details] [diff] [review]: ----------------------------------------------------------------- I might as well ask for a review on the build-system stuff as well.
Attachment #807367 -
Flags: review?(ted)
Comment 20•11 years ago
|
||
Comment on attachment 807367 [details] [diff] [review] WIP 2 This is fine, but let's just make sure till is in the loop.
Attachment #807367 -
Flags: review?(bobbyholley+bmo)
Attachment #807367 -
Flags: review+
Attachment #807367 -
Flags: feedback?(till)
Assignee | ||
Comment 21•11 years ago
|
||
OK, bad news. I did a clobber build locally and the same crash happened again. Looking at the generated code, initSelfHosting is indeed deoptimized, but selfHostingGlobal_.shape.value is an invalid pointer and when we pass it to JSAutoCompartment::JSAutoCompartment and it tries to dereference it, we crash. Any idea who knows this code best? I think I will not be an effective person debugging this...
Assignee | ||
Comment 22•11 years ago
|
||
Actually, it's not just shape_... All of the fields on that JSObject seem to point to invalid addresses...
Comment 23•11 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #22) > Actually, it's not just shape_... All of the fields on that JSObject seem > to point to invalid addresses... That would imply that the JSObject has been GCed, or, given that it was created in the line before, that it was never properly set up to begin with. (In reply to :Ehsan Akhgari (needinfo? me!) from comment #21) > Any idea who knows this code best? I think I will not be an effective > person debugging this... Lots of JS hackers can grok this code. The more interesting question is who has experience debugging opt builds on windows.
Updated•11 years ago
|
Attachment #807367 -
Flags: review+
Attachment #807367 -
Flags: feedback?(till)
Assignee | ||
Comment 24•11 years ago
|
||
Hmm, so I may have nailed down the miscompiling. At the end of JS_NewGlobalObject, we return |global|: return global; 11283838 mov eax,dword ptr [global] 1128383B lea ecx,[global] 1128383E call JS::Rooted<js::GlobalObject *>::~Rooted<js::GlobalObject *> (0FAAFE90h) 11283843 mov ecx,dword ptr [hold] 11283846 mov byte ptr [ecx],0 } 11283849 pop edi 1128384A pop esi 1128384B pop ebx 1128384C mov esp,ebp 1128384E pop ebp 1128384F mov ecx,dword ptr [esp] 11283852 mov dword ptr [esp+8],ecx 11283856 add esp,8 11283859 ret Note how we grab the return value (in eax) before calling ~Rooted. Here is the code for ~Rooted: JS::Rooted<js::GlobalObject *>::~Rooted<js::GlobalObject *>: 0FAAFE90 pop eax 0FAAFE91 push 177A2E4Dh 0FAAFE96 push 0 0FAAFE98 push eax 0FAAFE99 jmp JS::Rooted<js::GlobalObject *>::~Rooted<js::GlobalObject *> (0FAAFE70h) ~Rooted() { 0FAAFE70 push ebp 0FAAFE71 mov ebp,esp 0FAAFE73 push 0AC40h 0FAAFE78 call dword ptr [__PogoRuntimeVector+10h (127CC510h)] #ifdef JSGC_TRACK_EXACT_ROOTS JS_ASSERT(*stack == reinterpret_cast<Rooted<void*>*>(this)); *stack = prev; #endif } 0FAAFE7E pop ebp 0FAAFE7F ret 8 Note that we first pop the stack entry into eax. Nothing else in this function modifies eax. When we return into JS_NewGlobalObject, the eax value is now clobbered, and we never try to set it to the value of |global| again, and therefore we end up returning the wrong value to the caller, which is then passed to JSAutoCompartment::JSAutoCompartment and dereferenced there. Now, if my analysis here is correct, deoptimizing JS_NewGlobalObject may fix this bug.
Comment 25•11 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #24) > Now, if my analysis here is correct, deoptimizing JS_NewGlobalObject may fix > this bug. Hm, but isn't the bug in the Rooted destructor? That's going to be much more problematic to deoptimize. Deoptimizing JS_NewGlobalObject is fine.
Assignee | ||
Comment 26•11 years ago
|
||
Looking at the generated code from a deoptimized JS_NewGlobalObject, that should fix the problem. But I need to do a clobber build to make sure.
Assignee | ||
Comment 27•11 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #25) > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #24) > > Now, if my analysis here is correct, deoptimizing JS_NewGlobalObject may fix > > this bug. > > Hm, but isn't the bug in the Rooted destructor? That's going to be much more > problematic to deoptimize. Deoptimizing JS_NewGlobalObject is fine. No, the bug is in the ordering of instructions in the caller, JS_NewGlobalObject.
Updated•11 years ago
|
Attachment #807367 -
Flags: review?(ted) → review+
Assignee | ||
Comment 28•11 years ago
|
||
OK, I have terrible news. What I suggested in comment 24 works great for preventing the crash that we encountered during the xpcshell run, but now the process just finishes without actually doing anything, and it prints an "allocation size overflow" error on the console encountered in <http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/loader/XPCOMUtils.jsm#177>. This looks like another miscompilation in the JS engine... :/
Assignee | ||
Comment 29•11 years ago
|
||
This is the stack:
> xul.dll!js_ReportAllocationOverflow(js::ThreadSafeContext * cxArg=0x03429920) Line 452 C++
xul.dll!js::TempAllocPolicy::reportAllocOverflow() Line 22 + 0xd bytes C++
xul.dll!mozilla::VectorBase<JS::Value,8,js::TempAllocPolicy,js::Vector<JS::Value,8,js::TempAllocPolicy> >::growStorageBy(unsigned int incr=292429249) Line 731 + 0x12 bytes C++
xul.dll!mozilla::VectorBase<JS::Value,8,js::TempAllocPolicy,js::Vector<JS::Value,8,js::TempAllocPolicy> >::growByUninitialized(unsigned int incr=292429249) Line 819 + 0x1a bytes C++
xul.dll!JS::AutoVectorRooter<JS::Value>::resize(unsigned int newLength=2545580) Line 274 + 0x16 bytes C++
xul.dll!js::InvokeArgs::init(unsigned int argc=292429247) Line 1125 + 0x11 bytes C++
xul.dll!js_fun_call(JSContext * cx=0x03429920, unsigned int argc=292429247, JS::Value * vp=0x04133168) Line 866 + 0x11 bytes C++
xul.dll!js_fun_apply(JSContext * cx=0x03429920, unsigned int argc=1, JS::Value * vp=0x04133168) Line 913 + 0x33 bytes C++
xul.dll!js::CallJSNative(JSContext * cx=0x03429920, bool (JSContext *, unsigned int, JS::Value *)* native=0x116e1740, const JS::CallArgs & args={...}) Line 218 + 0x27 bytes C++
xul.dll!js::Invoke(JSContext * cx=0x03429920, JS::CallArgs args={...}, js::MaybeConstruct construct=NO_CONSTRUCT) Line 478 + 0x2a bytes C++
xul.dll!Interpret(JSContext * cx=0x03429920, js::RunState & state={...}) Line 2454 + 0x1b bytes C++
xul.dll!js::RunScript(JSContext * cx=0x03429920, js::RunState & state={...}) Line 435 + 0x13 bytes C++
xul.dll!js::Invoke(JSContext * cx=0x03429920, JS::CallArgs args={...}, js::MaybeConstruct construct=NO_CONSTRUCT) Line 497 + 0x1a bytes C++
xul.dll!js::Invoke(JSContext * cx=0x03429920, const JS::Value & thisv={...}, const JS::Value & fval={...}, unsigned int argc=0, JS::Value * argv=0x0026e370, JS::MutableHandle<JS::Value> rval={...}) Line 528 + 0x1d bytes C++
xul.dll!js::DirectProxyHandler::call(JSContext * cx=0x0026e360, JS::Handle<JSObject *> proxy={...}, const JS::CallArgs & args={...}) Line 450 + 0x73 bytes C++
xul.dll!js::CrossCompartmentWrapper::call(JSContext * cx=0x03e40a20, JS::Handle<JSObject *> wrapper={...}, const JS::CallArgs & args={...}) Line 454 + 0x16 bytes C++
xul.dll!js::Proxy::call(JSContext * cx=0x03429920, JS::Handle<JSObject *> proxy={...}, const JS::CallArgs & args={...}) Line 2613 + 0x18 bytes C++
xul.dll!proxy_Call(JSContext * cx=0x03429920, unsigned int argc=0, JS::Value * vp=0x0026e360) Line 3132 + 0x35 bytes C++
xul.dll!js::CallJSNative(JSContext * cx=0x03429920, bool (JSContext *, unsigned int, JS::Value *)* native=0x10a6f1d0, const JS::CallArgs & args={...}) Line 218 + 0x27 bytes C++
xul.dll!js::Invoke(JSContext * cx=0x03429920, JS::CallArgs args={...}, js::MaybeConstruct construct=NO_CONSTRUCT) Line 471 + 0x1c bytes C++
xul.dll!js::Invoke(JSContext * cx=0x03429920, const JS::Value & thisv={...}, const JS::Value & fval={...}, unsigned int argc=0, JS::Value * argv=0x00000000, JS::MutableHandle<JS::Value> rval={...}) Line 528 + 0x1d bytes C++
xul.dll!js::InvokeGetterOrSetter(JSContext * cx=0x03429920, JSObject * obj=0x03e37f10, JS::Value fval={...}, unsigned int argc=0, JS::Value * argv=0x00000000, JS::MutableHandle<JS::Value> rval={...}) Line 599 + 0x31 bytes C++
xul.dll!js::Shape::get(JSContext * cx=0x03429920, JS::Handle<JSObject *> receiver={...}, JSObject * obj=0x0026e448, JSObject * pobj=0x0026e448, JS::MutableHandle<JS::Value> vp={...}) Line 69 + 0x33 bytes C++
xul.dll!NativeGetInline<1>(JSContext * cx=0x03429920, JS::Handle<JSObject *> obj={...}, JS::Handle<JSObject *> receiver={...}, JS::Handle<JSObject *> pobj={...}, JS::Handle<js::Shape *> shape={...}, unsigned int getHow=0, JS::MutableHandle<JS::Value> vp={...}) Line 4071 + 0x22 bytes C++
xul.dll!GetPropertyHelperInline<1>(JSContext * cx=0x03e37f10, JS::Handle<JSObject *> obj={...}, JS::Handle<JSObject *> receiver={...}, JS::Handle<int> id={...}, unsigned int getHow=0, JS::MutableHandle<JS::Value> vp={...}) Line 4242 + 0x4c bytes C++
xul.dll!js::baseops::GetProperty(JSContext * cx=0x03429920, JS::Handle<JSObject *> obj={...}, JS::Handle<JSObject *> receiver={...}, JS::Handle<int> id={...}, JS::MutableHandle<JS::Value> vp={...}) Line 4258 + 0x1b bytes C++
xul.dll!JSObject::getGeneric(JSContext * cx=0x03429920, JS::Handle<JSObject *> obj={...}, JS::Handle<JSObject *> receiver={...}, JS::Handle<int> id={...}, JS::MutableHandle<JS::Value> vp={...}) Line 917 + 0x26 bytes C++
xul.dll!js::DirectProxyHandler::get(JSContext * cx=0x03e37f10, JS::Handle<JSObject *> proxy={...}, JS::Handle<JSObject *> receiver={...}, JS::Handle<int> id={...}, JS::MutableHandle<JS::Value> vp={...}) Line 563 + 0x34 bytes C++
xul.dll!js::CrossCompartmentWrapper::get(JSContext * cx=0x03e059e0, JS::Handle<JSObject *> wrapper={...}, JS::Handle<JSObject *> receiver={...}, JS::Handle<int> id={...}, JS::MutableHandle<JS::Value> vp={...}) Line 318 + 0x51 bytes C++
xul.dll!js::Proxy::get(JSContext * cx=0x03429920, JS::Handle<JSObject *> proxy={...}, JS::Handle<JSObject *> receiver={...}, JS::Handle<int> id={...}, JS::MutableHandle<JS::Value> vp={...}) Line 2474 + 0x23 bytes C++
xul.dll!proxy_GetGeneric(JSContext * cx=0x03429920, JS::Handle<JSObject *> obj={...}, JS::Handle<JSObject *> receiver={...}, JS::Handle<int> id={...}, JS::MutableHandle<JS::Value> vp={...}) Line 2818 + 0x1e bytes C++
xul.dll!JSObject::getGeneric(JSContext * cx=0x03429920, JS::Handle<JSObject *> obj={...}, JS::Handle<JSObject *> receiver={...}, JS::Handle<int> id={...}, JS::MutableHandle<JS::Value> vp={...}) Line 914 + 0x1f bytes C++
xul.dll!GetPropertyOperation(JSContext * cx=0x03429920, js::StackFrame * fp=0x041330a8, JS::Handle<JSScript *> script={...}, unsigned char * pc=0x03429920, JS::MutableHandle<JS::Value> lval={...}, JS::MutableHandle<JS::Value> vp={...}) Line 278 + 0x63 bytes C++
xul.dll!Interpret(JSContext * cx=0x03429920, js::RunState & state={...}) Line 2293 + 0x2a bytes C++
xul.dll!js::RunScript(JSContext * cx=0x03429920, js::RunState & state={...}) Line 435 + 0x13 bytes C++
xul.dll!js::ExecuteKernel(JSContext * cx=0x03429920, JS::Handle<JSScript *> script={...}, JSObject & scopeChainArg={...}, const JS::Value & thisv={...}, js::ExecuteType type=EXECUTE_GLOBAL, js::AbstractFramePtr evalInFrame={...}, JS::Value * result=0x0026ea58) Line 619 + 0x12 bytes C++
xul.dll!js::Execute(JSContext * cx=0x0026d7ac, JS::Handle<JSScript *> script={...}, JSObject & scopeChainArg={...}, JS::Value * rval=0x0026ea58) Line 656 + 0x3c bytes C++
xul.dll!JS_ExecuteScript(JSContext * cx=0x03429920, JSObject * objArg=0x03e220d0, JSScript * scriptArg=0x03e26100, JS::Value * rval=0x0026ea58) Line 4864 + 0x3c bytes C++
xpcshell.exe!ProcessFile(JSContext * cx=0x03429920, JS::Handle<JSObject *> obj={...}, const char * filename=0x02786068, _iobuf * file=0x0026d7ac, bool forceTTY=false) Line 1093 + 0x14 bytes C++
xpcshell.exe!Process(JSContext * cx=0x03429920, JS::Handle<JSObject *> obj={...}, const char * filename=0x02786068, bool forceTTY=false) Line 1169 + 0x11 bytes C++
xpcshell.exe!ProcessArgs(JSContext * cx=0x03429920, JS::Handle<JSObject *> obj={...}, char * * argv=0x02787264, int argc=4, XPCShellDirProvider * aDirProvider=0x0026fb30) Line 1305 + 0xf bytes C++
xpcshell.exe!NS_internal_main(int argc=4, char * * argv=0x02787250, char * * envp=0x01f32da8) Line 1770 C++
xpcshell.exe!wmain(int argc=41447584, wchar_t * * argv=0x01f3c798) Line 112 C++
xpcshell.exe!__tmainCRTStartup() Line 552 + 0x17 bytes C
kernel32.dll!@BaseThreadInitThunk@12() + 0xe bytes
ntdll.dll!___RtlUserThreadStart@8() + 0x27 bytes
ntdll.dll!__RtlUserThreadStart@8() + 0x1b bytes
At a quick glance, this seems to be where the miscompilation happens:
// js_fun_apply(JSCOntext *, unsigned int, JS::Value *)
/* Step 2. */
if (argc < 2 || vp[3].isNullOrUndefined())
116E1D95 push 30DA20h
116E1D9A call dword ptr [__PogoRuntimeVector (12A83500h)]
return js_fun_call(cx, (argc > 0) ? 1 : 0, vp);
116E1DA0 test esi,esi
116E1DA2 jbe 116E1DB6
116E1DA4 push 30DA28h
116E1DA9 call dword ptr [__PogoRuntimeVector (12A83500h)]
116E1DAF mov eax,1
116E1DB4 jmp 116E1DB8
116E1DB6 xor eax,eax
116E1DB8 lea ecx,[ebp-7Ch]
116E1DBB call JS::Rooted<JS::Value>::~Rooted<JS::Value> (0FE9FBEEh)
116E1DC0 push ebx
116E1DC1 push eax
116E1DC2 push edi
116E1DC3 push 30DA30h
116E1DC8 call dword ptr [__PogoRuntimeVector (12A83500h)]
116E1DCE call js_fun_call (1167A5C0h)
116E1DD3 push 30DA38h
116E1DD8 call dword ptr [__PogoRuntimeVector (12A83500h)]
116E1DDE add esp,0Ch
}
Note the JS::Rooted<JS::Value>::~Rooted call there... :/ We end up passing a pointer as the size of the arguments to the function...
Comment 30•11 years ago
|
||
The MSVC optimizer does register-used analysis and apparently believes the ~Rooted doesn't modify EAX. I wonder if it's possible to remove ~Rooted from the PGO optimizer. Hard because it's a template...
Assignee | ||
Comment 31•11 years ago
|
||
(In reply to Benjamin Smedberg [:bsmedberg] from comment #30) > The MSVC optimizer does register-used analysis and apparently believes the > ~Rooted doesn't modify EAX. Yeah. Both of the bugs that I've looked at so far have been about the compiler assuming that. > I wonder if it's possible to remove ~Rooted from the PGO optimizer. Hard because it's a template... I just tried wrapping it in a #pragma optimize jail, hopefully the effect of those #pragmas is based on the lexical location of the code... I have a build in progress now.
Assignee | ||
Comment 32•11 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #31) > > I wonder if it's possible to remove ~Rooted from the PGO optimizer. Hard because it's a template... > > I just tried wrapping it in a #pragma optimize jail, hopefully the effect of > those #pragmas is based on the lexical location of the code... I have a > build in progress now. The generated code for ~Rooted in my new build is exactly the same as before, which means that it still clobbers eax. :(
Assignee | ||
Comment 33•11 years ago
|
||
I verified locally that letting the compiler emit the destructor itself is enough to work around this bug.
Attachment #808741 -
Flags: review?(sphink)
Assignee | ||
Comment 34•11 years ago
|
||
(And we won't need to deoptimize JS_NewGlobalObject)
Comment 35•11 years ago
|
||
Does this (In reply to :Ehsan Akhgari (needinfo? me!) from comment #33) > Created attachment 808741 [details] [diff] [review] > Part 1: Work around the PGO bug hitting the callers of ~Rooted Does this just kick the can down the road to when we want to enable exact rooting? Or is the destructor generated in that situation different enough?
Comment 36•11 years ago
|
||
Comment on attachment 808741 [details] [diff] [review] Part 1: Work around the PGO bug hitting the callers of ~Rooted Review of attachment 808741 [details] [diff] [review]: ----------------------------------------------------------------- Wow, gmail was really slow about showing me that review request. This seems fine, but it's gonna suck when we turn on generational GC if this comes back. Maybe it won't though; if there's some nontrivial code running in the destructor, maybe MSVC won't be so cavalier about eax or something?
Attachment #808741 -
Flags: review?(sphink) → review+
Assignee | ||
Comment 37•11 years ago
|
||
I still don't understand the reason why this bug exist, but in general you can't assume that eax will not be clobbered by a function, since that's where you return values. I don't think there is something very special about the Rooted destructor here, it's just that I was hoping it won't emit a function call in the hopes to bypass this bug. But this bug can happen across any function call from what I can tell... :/
Updated•11 years ago
|
Blocks: ExactRooting
Assignee | ||
Comment 38•11 years ago
|
||
Comment on attachment 808741 [details] [diff] [review] Part 1: Work around the PGO bug hitting the callers of ~Rooted https://hg.mozilla.org/integration/mozilla-inbound/rev/d33e1297c557
Attachment #808741 -
Flags: checkin+
Assignee | ||
Comment 39•11 years ago
|
||
New try run: <https://tbpl.mozilla.org/?tree=Try&rev=6c50d54fc0a4>
Assignee | ||
Comment 41•11 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #39) > New try run And now we get killed because the PGO link stage takes more than 2 hours (but at least we get to that stage now!!!) I changed link.py to "tickle" stdout every half an hour, hopefully that will work around this problem. https://tbpl.mozilla.org/?tree=Try&rev=144d7157a568
Assignee | ||
Comment 42•11 years ago
|
||
Mike, did you figure out what was causing the PGO bustages on inbound today? I can't reproduce the build failures in comment 41 locally at all (I've done a few successful PGO builds since yesterday and they seem to work fine.)
Flags: needinfo?(mh+mozilla)
Comment 43•11 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #42) > Mike, did you figure out what was causing the PGO bustages on inbound today? I haven't looked. We unfortunately don't have easy access to failed build objdirs. > I can't reproduce the build failures in comment 41 locally at all (I've done > a few successful PGO builds since yesterday and they seem to work fine.) This looks interestingly consistently failing. Which is a good thing, because it means it's somehow possible to get to a failing build binaries.
Flags: needinfo?(mh+mozilla)
Assignee | ||
Comment 44•11 years ago
|
||
OK, so here's another try run with a newer inbound base revision: <https://tbpl.mozilla.org/?tree=Try&rev=40add7f43b2c> This time it got to the second link step and died there because apparently my tickling trick didn't work... Ted, am I doing anything wrong here?
Flags: needinfo?(ted)
Assignee | ||
Comment 45•11 years ago
|
||
Here's three different tickling techniques. Let's see which one brings smiles to the try server's lips. https://tbpl.mozilla.org/?tree=Try&rev=cfb3083bc8dc https://tbpl.mozilla.org/?tree=Try&rev=efb9e42b8432 https://tbpl.mozilla.org/?tree=Try&rev=e26c3f6ccab8
Comment 46•11 years ago
|
||
The only thing I can think is that you're getting screwed by output buffering here. Try putting a sys.stdout.flush() after your print?
Flags: needinfo?(ted)
Assignee | ||
Comment 47•11 years ago
|
||
(In reply to comment #46) > The only thing I can think is that you're getting screwed by output buffering > here. Try putting a sys.stdout.flush() after your print? Yeah that does fix the problem. I finally got green on try in fact last night!
Assignee | ||
Comment 48•11 years ago
|
||
OK, so like I said, I finally got green on try! I've pushed <https://tbpl.mozilla.org/?tree=Try&rev=a79d6a091448> to get baseline perf numbers for a talos comparison. The linker memory usage is about 3.5 gigs, which is almost where we were in July <http://graphs.mozilla.org/graph.html#tests=[[205,63,8]]&sel=none&displayrange=90&datatype=running>. I hope that bug 921130 and the like can help us bring it down a bit. Do you guys think the linker memory usage is acceptable?
Assignee | ||
Comment 49•11 years ago
|
||
Attachment #810711 -
Flags: review?(ted)
Comment 50•11 years ago
|
||
It's a little scary, but with VS2013's x64-cross toolchain being the light at the end of the tunnel I think I feel okay with it. We should probably make sure we don't break --enable-shared-js in the interim so that we can flip it back as a safety valve if we need to.
Updated•11 years ago
|
Attachment #810711 -
Flags: review?(ted) → review+
Comment 51•11 years ago
|
||
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #50) > It's a little scary, but with VS2013's x64-cross toolchain being the light > at the end of the tunnel I think I feel okay with it. We should probably > make sure we don't break --enable-shared-js in the interim so that we can > flip it back as a safety valve if we need to. There are currently other reasons to not break --enable-shared-js, such as the fact that I'll chew off my own arm if I have to wait for the full libxul relink every time I want to test a spidermonkey change in the browser.
Assignee | ||
Comment 52•11 years ago
|
||
(In reply to comment #51) > (In reply to Ted Mielczarek [:ted.mielczarek] from comment #50) > > It's a little scary, but with VS2013's x64-cross toolchain being the light > > at the end of the tunnel I think I feel okay with it. We should probably > > make sure we don't break --enable-shared-js in the interim so that we can > > flip it back as a safety valve if we need to. > > There are currently other reasons to not break --enable-shared-js, such as the > fact that I'll chew off my own arm if I have to wait for the full libxul relink > every time I want to test a spidermonkey change in the browser. OK, so we have three reasons then, one is called PGO, the other Steve, and of cource, Boris. :-)
Assignee | ||
Comment 53•11 years ago
|
||
Comment on attachment 810711 [details] [diff] [review] Part 2: Tickle stdout when linking the PGOed xul.dll https://hg.mozilla.org/integration/mozilla-inbound/rev/c3fd83b98898
Attachment #810711 -
Flags: checkin+
Comment 54•11 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #52) > OK, so we have three reasons then, one is called PGO, the other Steve, and > of cource, Boris. :-) We'd better find something to avoid it breaking, them. Like enabling it by default on some branch.
Comment 55•11 years ago
|
||
(In reply to Steve Fink [:sfink] from comment #51) > There are currently other reasons to not break --enable-shared-js, such as > the fact that I'll chew off my own arm if I have to wait for the full libxul > relink every time I want to test a spidermonkey change in the browser. Note that incremental linking, that we should investigate, should help you there, without enable-shared-js.
Comment 56•11 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #55) > (In reply to Steve Fink [:sfink] from comment #51) > > There are currently other reasons to not break --enable-shared-js, such as > > the fact that I'll chew off my own arm if I have to wait for the full libxul > > relink every time I want to test a spidermonkey change in the browser. > > Note that incremental linking, that we should investigate, should help you > there, without enable-shared-js. Yes, I would be quite happy with something like that. I only care about the speed (and having debugging work on the resulting binaries.) For Boris, if SPS became at least as useful to him as Instruments is, we'd be all set...
Assignee | ||
Comment 57•11 years ago
|
||
(In reply to comment #54) > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #52) > > OK, so we have three reasons then, one is called PGO, the other Steve, and > > of cource, Boris. :-) > > We'd better find something to avoid it breaking, them. Like enabling it by > default on some branch. I can easily turn that option on on the Profiling branch which I maintain.
(In reply to Mike Hommey [:glandium] from comment #55) > (In reply to Steve Fink [:sfink] from comment #51) > > There are currently other reasons to not break --enable-shared-js, such as > > the fact that I'll chew off my own arm if I have to wait for the full libxul > > relink every time I want to test a spidermonkey change in the browser. > > Note that incremental linking, that we should investigate, should help you > there, without enable-shared-js. FWIW, we used to have incremental linking on Windows ... but it stopped working at some point.
Assignee | ||
Comment 59•11 years ago
|
||
Taras, we need this fixed in order for us to be able to make progress on bug 871846, so that we can use ICU form within Gecko for sorting strings. The concern here is that this will take us back to where we were in July with regards to the PGO linker's memory usage (~3.5GB) because we need to link more code. While we are working on the final fix for this issue by investigating the switch to VC12 (bug 914596 and friends) there's a slight chance that at some point before that happens we hit the PGO memory usage issue again. In case that happens, would you be able to allocate resources for somebody on your team to adjust the threshold for what code we PGO so that we can reopen the trees? Thanks!
Flags: needinfo?(taras.mozilla)
Assignee | ||
Comment 60•11 years ago
|
||
Let's see if bug 921130 moves the needle on the PGO memory usage: https://tbpl.mozilla.org/?tree=Try&rev=025d854c2e9b
Assignee | ||
Comment 62•11 years ago
|
||
There are a number of regressions which I can't explain, especially in Dromaeo and Kraken. Is it possible that the JS code is not being PGOed? Ted, Boris, any ideas? <http://compare-talos.mattn.ca/?oldRevs=a79d6a091448&newRev=e26c3f6ccab8&server=graphs.mozilla.org&submit=true>
Flags: needinfo?(ted)
Flags: needinfo?(bzbarsky)
Comment 63•11 years ago
|
||
Possible, I suppose, but the patches here have workarounds precisely for bits of JS _being_ PGOed, no?
Flags: needinfo?(bzbarsky)
Comment 64•11 years ago
|
||
Yeah, that doesn't seem super likely. Also, note: http://mxr.mozilla.org/mozilla-central/source/js/src/moz.build#351 And the build log shows JS objects being built with -GL: e:\builds\moz2_slave\try-w32-0000000000000000000000\build\js\src\config\rules.mk:1136:0$ e:/builds/moz2_slave/try-w32-0000000000000000000000/build/obj-firefox/_virtualenv/Scripts/python.exe -O e:/builds/moz2_slave/try-w32-0000000000000000000000/build/js/src/build/cl.py cl -FoArgumentsObject.obj -c -DENABLE_PARALLEL_JS -DENABLE_BINARYDATA -DNO_NSPR_10_SUPPORT -DEXPORT_JS_API -DJS_HAS_CTYPES -DDLL_PREFIX=\"\" -DDLL_SUFFIX=\".dll\" -DUSE_ZLIB -Ictypes/libffi/include -Ie:/builds/moz2_slave/try-w32-0000000000000000000000/build/js/src/../../mfbt/double-conversion -Ie:/builds/moz2_slave/try-w32-0000000000000000000000/build/js/src/../../intl/icu/source/common -Ie:/builds/moz2_slave/try-w32-0000000000000000000000/build/js/src/../../intl/icu/source/i18n -Ie:/builds/moz2_slave/try-w32-0000000000000000000000/build/js/src -I. -I./../../dist/include -Ie:/builds/moz2_slave/try-w32-0000000000000000000000/build/obj-firefox/dist/include/nspr -Ie:/builds/moz2_slave/try-w32-0000000000000000000000/build/js/src -MD -FI ./js-confdefs.h -DMOZILLA_CLIENT -wd4099 -TP -nologo -wd4345 -wd4351 -wd4800 -D_CRT_SECURE_NO_WARNINGS -W3 -Gy -Fdgenerated.pdb -wd4244 -wd4251 -we4553 -GR- -DNDEBUG -DTRIMMED -Zi -UDEBUG -DNDEBUG -GL -O2 -Oy- -DUSE_SYSTEM_MALLOC=1 -DENABLE_ASSEMBLER=1 -DENABLE_JIT=1 e:/builds/moz2_slave/try-w32-0000000000000000000000/build/js/src/vm/ArgumentsObject.cpp PGO is hard, let's go shopping.
Flags: needinfo?(ted)
Assignee | ||
Comment 65•11 years ago
|
||
(In reply to comment #64) > PGO is hard, let's go shopping. Hehe yeah! I chatted with glandium on irc about this. He suspected that perhaps the fact that we link js code first into a static library and then link that against xul.dll is confusing the compiler somehow. He had an idea for a hack to work around that behavior, which I am currently testing locally.
Assignee | ||
Comment 66•11 years ago
|
||
I did two try runs to get numbers on two approaches: Ted's suggestion: https://tbpl.mozilla.org/?tree=Try&rev=7b028bad1246 glandium's suggestion: https://tbpl.mozilla.org/?tree=Try&rev=6b1d8b2d7b7d
Comment 67•11 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #59) > Taras, we need this fixed in order for us to be able to make progress on bug > 871846, so that we can use ICU form within Gecko for sorting strings. > > The concern here is that this will take us back to where we were in July > with regards to the PGO linker's memory usage (~3.5GB) because we need to > link more code. While we are working on the final fix for this issue by > investigating the switch to VC12 (bug 914596 and friends) there's a slight > chance that at some point before that happens we hit the PGO memory usage > issue again. In case that happens, would you be able to allocate resources > for somebody on your team to adjust the threshold for what code we PGO so > that we can reopen the trees? I am not interesting in being the responsible party for ICU merge :) My vote is for making ICU a separate library, ideally one that is loaded dynamically.
Flags: needinfo?(taras.mozilla)
Comment 68•11 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #66) > I did two try runs to get numbers on two approaches: > > Ted's suggestion: https://tbpl.mozilla.org/?tree=Try&rev=7b028bad1246 > glandium's suggestion: https://tbpl.mozilla.org/?tree=Try&rev=6b1d8b2d7b7d Ted's fail because DIST_INSTALL makes js_static* not installed $(DIST)/lib, which is where linking xul.dll is looking for it. That would need a change to MOZ_JS_STATIC_LIBS in configure.in. Mine failed because MOZ_ZLIB_LIBS is on the link command line for unit tests, and it's also in mozglue (Ted's would have failed on that too if xul.dll had linked). That should be worked around by removing https://mxr.mozilla.org/mozilla-central/source/mozglue/build/Makefile.in#52. I guess I'll need to fix what i did in bug 763651, which doesn't allow to easily switch between static/shared.
Assignee | ||
Comment 69•11 years ago
|
||
New try push based on comment 68: https://tbpl.mozilla.org/?tree=Try&rev=0256045a99b5
Assignee | ||
Comment 70•11 years ago
|
||
(In reply to Taras Glek (:taras) from comment #67) > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #59) > > Taras, we need this fixed in order for us to be able to make progress on bug > > 871846, so that we can use ICU form within Gecko for sorting strings. > > > > The concern here is that this will take us back to where we were in July > > with regards to the PGO linker's memory usage (~3.5GB) because we need to > > link more code. While we are working on the final fix for this issue by > > investigating the switch to VC12 (bug 914596 and friends) there's a slight > > chance that at some point before that happens we hit the PGO memory usage > > issue again. In case that happens, would you be able to allocate resources > > for somebody on your team to adjust the threshold for what code we PGO so > > that we can reopen the trees? > > I am not interesting in being the responsible party for ICU merge :) I am not asking you to! The ICU library is already in our codebase, this bug is about merging mozjs.dll into libxul.dll (the ICU library is a part of mozjs.dll). > My vote > is for making ICU a separate library, ideally one that is loaded dynamically. I may look into doing that if this effort doesn't lead anywhere, but even then the ICU library will be loaded statically, and we'd end up with one more DLL which we need to load separately at startup, so that won't be great. But it's currently my second worst option here.
Assignee | ||
Comment 71•11 years ago
|
||
I tried to fix the try failure in comment 69 locally. I tried adding MOZ_ZLIB_LIBS to both the EXTRA_LIBS and EXTRA_DSO_OPTS variables in js/src/shell/Makefile.in, but none of them made the linker error go away. I'm not sure what else I should try here. :/
Assignee | ||
Comment 72•11 years ago
|
||
OK, I finally have a patch which seems to build reliably locally. Pushing to try to get numbers again: Baseline: https://tbpl.mozilla.org/?tree=Try&rev=2e0ea67c50cf mozjs folder into xul: https://tbpl.mozilla.org/?tree=Try&rev=47b41c1cd4c6 Fingers crossed...
Attachment #807367 -
Attachment is obsolete: true
Assignee | ||
Comment 73•11 years ago
|
||
This fixed the test builds <https://tbpl.mozilla.org/?tree=Try&rev=3f1c69590353> but the tickler didn't seem to work here for some reason. :(
Assignee | ||
Comment 74•11 years ago
|
||
Sigh... Hopefully this will build: https://tbpl.mozilla.org/?tree=Try&rev=a96f9f7d373f
Assignee | ||
Comment 75•11 years ago
|
||
OK, I know I've said this a few times, but now I think I do have a patch which actually build. Pushed to try to get Talos numbers: Baseline: https://tbpl.mozilla.org/?tree=Try&rev=ff699478f972 JS-in-XUL: https://tbpl.mozilla.org/?tree=Try&rev=3a79c1775830
Assignee | ||
Comment 76•11 years ago
|
||
OK, I finally got the Talos numbers with the new approach, and nothing has improved :( <http://compare-talos.mattn.ca/?oldRevs=ff699478f972&newRev=3a79c1775830&server=graphs.mozilla.org&submit=true> Time to shift gears I guess.
Summary: Fold mozjs.dll back into xul.dll → Build ICU as a shared library so that we can link to it both from mozjs.dll and xul.dll
I need this for locale-aware sorting on IndexedDB too...
Assignee | ||
Comment 78•11 years ago
|
||
Actually I really don't want us to have to build icu as a shared library. I asked gps for help to see if someone can figure out what's causing js to not be PGOed here...
Summary: Build ICU as a shared library so that we can link to it both from mozjs.dll and xul.dll → Fold mozjs.dll back into xul.dll
Assignee | ||
Comment 79•11 years ago
|
||
Attachment #813276 -
Attachment is obsolete: true
Comment 80•11 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #78) > Actually I really don't want us to have to build icu as a shared library. Note we may still want to, but not for release builds, to keep supporting --enable-shared-js, which, e.g. bz uses, and that I'm now starting to use because it makes linking faster.
Comment 81•11 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #75) > OK, I know I've said this a few times, but now I think I do have a patch > which actually build. Pushed to try to get Talos numbers: > > Baseline: https://tbpl.mozilla.org/?tree=Try&rev=ff699478f972 > JS-in-XUL: https://tbpl.mozilla.org/?tree=Try&rev=3a79c1775830 Looking at the log, it does look like js is properly pgoed (cpp files built with -GL), and the real static library is not created. So far for the static library theory.
Comment 82•11 years ago
|
||
Ehsan asked me to look into this. However, I personally cannot afford the time. joey: Is this something you could look at?
Flags: needinfo?(joey)
Comment 83•11 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #82) > Ehsan asked me to look into this. However, I personally cannot afford the > time. > > joey: Is this something you could look at? I'm working on the cross mac builds at the moment.
Flags: needinfo?(joey)
Assignee | ||
Comment 84•11 years ago
|
||
This is a WIP patch for building ICU as a shared library, and for me it compiles locally on Windows, Mac and Linux. I have no idea if I'm doing something that is not kosher, so it's best if you guys can vouch for this before I move further. Here's a try run: <https://tbpl.mozilla.org/?tree=Try&rev=5084a94d0a1a> Also, since I'll be away soon, I would *really* appreciate if somebody could take this patch and convert it into something that can be landed!
Attachment #822599 -
Flags: feedback?(mh+mozilla)
Attachment #822599 -
Flags: feedback?(gps)
Comment 85•11 years ago
|
||
Comment on attachment 822599 [details] [diff] [review] WIP for building ICU as a shared library Review of attachment 822599 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/installer/package-manifest.in @@ +132,5 @@ > +#elif defined(XP_UNIX) > +@BINPATH@/libicudata.so > +@BINPATH@/libicui18n.so > +@BINPATH@/libicutu.so > +@BINPATH@/libicuuc.so I'm pretty sure there are symbol conflicts with system icu waiting to happen on linux. I don't think we should move ICU to a separate lib on non-windows unless we're building --with-shared-js.
Attachment #822599 -
Flags: feedback?(mh+mozilla) → feedback-
Comment 87•11 years ago
|
||
Comment on attachment 822599 [details] [diff] [review] WIP for building ICU as a shared library I'm going to recuse myself from this. glandium seems to have it covered and he'll give a better review than me.
Attachment #822599 -
Flags: feedback?(gps)
Assignee | ||
Comment 88•11 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #85) > Comment on attachment 822599 [details] [diff] [review] > WIP for building ICU as a shared library > > Review of attachment 822599 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/installer/package-manifest.in > @@ +132,5 @@ > > +#elif defined(XP_UNIX) > > +@BINPATH@/libicudata.so > > +@BINPATH@/libicui18n.so > > +@BINPATH@/libicutu.so > > +@BINPATH@/libicuuc.so > > I'm pretty sure there are symbol conflicts with system icu waiting to happen > on linux. I don't think we should move ICU to a separate lib on non-windows > unless we're building --with-shared-js. So are you suggesting that I should just make everything in my patch only happen on Windows? Is the approach in this patch good? I'm asking because I have no idea what I'm doing here, and I'd really appreciate if somebody would give me pointers as to whether what I'm doing is on the right track and if not, what I should do in order to fix it. Thanks!
Flags: needinfo?(mh+mozilla)
Assignee | ||
Comment 89•11 years ago
|
||
glandium: ping?
Comment 90•11 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #88) > (In reply to Mike Hommey [:glandium] from comment #85) > > Comment on attachment 822599 [details] [diff] [review] > > WIP for building ICU as a shared library > > > > Review of attachment 822599 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: browser/installer/package-manifest.in > > @@ +132,5 @@ > > > +#elif defined(XP_UNIX) > > > +@BINPATH@/libicudata.so > > > +@BINPATH@/libicui18n.so > > > +@BINPATH@/libicutu.so > > > +@BINPATH@/libicuuc.so > > > > I'm pretty sure there are symbol conflicts with system icu waiting to happen > > on linux. I don't think we should move ICU to a separate lib on non-windows > > unless we're building --with-shared-js. > > So are you suggesting that I should just make everything in my patch only > happen on Windows? Sorry for the long delay. Essentially yes, but instead of making it windows or non-windows specific, it would be shared-js or non-shared-js (there are people, like bz, who do build with a shared js library on non-windows) > Is the approach in this patch good? Yes, except i'm not sure what the configure.in changes are for.
Flags: needinfo?(mh+mozilla)
Assignee | ||
Comment 91•11 years ago
|
||
The configure.in changes are needed because I need the following variables: +AC_SUBST(DBG_SUFFIX) +AC_SUBST(ICU_LIB_NAMES) +AC_SUBST(MOZ_ICU_LIBS) And I need to move that code to be before the AC_OUTPUT in that file.
Comment 92•11 years ago
|
||
Comment on attachment 819130 [details] [diff] [review] WIP 4 Review of attachment 819130 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/gc/Memory.cpp @@ +24,5 @@ > > +#ifdef _MSC_VER > +// Link with psapi.lib for GetProcessMemoryInfo > +#pragma comment(lib, "psapi.lib") > +#endif This blows my mind. How come we manage to build without this (or psapi.lib in EXTRA_DSO_LDOPTS in the appropriate makefiles) currently?
Comment 93•11 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #92) > Comment on attachment 819130 [details] [diff] [review] > WIP 4 > > Review of attachment 819130 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/gc/Memory.cpp > @@ +24,5 @@ > > > > +#ifdef _MSC_VER > > +// Link with psapi.lib for GetProcessMemoryInfo > > +#pragma comment(lib, "psapi.lib") > > +#endif > > This blows my mind. How come we manage to build without this (or psapi.lib > in EXTRA_DSO_LDOPTS in the appropriate makefiles) currently? https://mxr.mozilla.org/mozilla-central/source/js/src/configure.in#1664 I also see LIBS contains psapi.lib
Assignee | ||
Comment 94•11 years ago
|
||
Yeah, I don't remember exactly what makes the current code work.
Comment 95•11 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #94) > Yeah, I don't remember exactly what makes the current code work. Useless linkage of the js static real library does it. Linking the fake library makes all objects linked, while linking the real library just ignores all objects because no symbol they export are used. Bug 942031 takes care of that.
Depends on: 942031
Assignee | ||
Comment 96•11 years ago
|
||
Comment 97•11 years ago
|
||
<ehsan> $ ./mach run -P pb <ehsan> 0:00.16 /Users/ehsanakhgari/moz/mozilla-central/obj-ff-sharedjs/dist/NightlyDebug.app/Contents/MacOS/firefox -no-remote -foreground -P pb <ehsan> XPCOMGlueLoad error 4:0 for file /Users/ehsanakhgari/moz/mozilla-central/obj-ff-sharedjs/dist/NightlyDebug.app/Contents/MacOS/libicuuc.50.dylib: <ehsan> Library not loaded: libicudata.50.dylib <ehsan> Referenced from: /Users/ehsanakhgari/moz/mozilla-central/obj-ff-sharedjs/dist/NightlyDebug.app/Contents/MacOS/libicuuc.50.dylib <ehsan> Reason: image not found <ehsan> Couldn't load XPCOM. I think the problem is the lack of @executable_path. Libs need to be linked with -install_name @executable_path/lib_file_name.dylib.
Assignee | ||
Updated•11 years ago
|
Attachment #822599 -
Attachment is obsolete: true
Assignee | ||
Comment 98•11 years ago
|
||
Comment on attachment 8337090 [details] [diff] [review] Build ICU as a shared library where JS is built as a shared library Review of attachment 8337090 [details] [diff] [review]: ----------------------------------------------------------------- This is mostly correct except that it doesn't yet run on Mac. I get: $ ./mach run -P pb 0:00.16 /Users/ehsanakhgari/moz/mozilla-central/obj-ff-sharedjs/dist/NightlyDebug.app/Contents/MacOS/firefox -no-remote -foreground -P pb XPCOMGlueLoad error 4:0 for file /Users/ehsanakhgari/moz/mozilla-central/obj-ff-sharedjs/dist/NightlyDebug.app/Contents/MacOS/libicuuc.50.dylib: Library not loaded: libicudata.50.dylib Referenced from: /Users/ehsanakhgari/moz/mozilla-central/obj-ff-sharedjs/dist/NightlyDebug.app/Contents/MacOS/libicuuc.50.dylib Reason: image not found Couldn't load XPCOM. I'll look into that next week. https://tbpl.mozilla.org/?tree=Try&rev=5784e667c573
Attachment #8337090 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 99•11 years ago
|
||
I managed to push the old version of this patch to try! https://tbpl.mozilla.org/?tree=Try&rev=e748de483250
Assignee | ||
Comment 100•11 years ago
|
||
(In reply to comment #97) > <ehsan> $ ./mach run -P pb > <ehsan> 0:00.16 > /Users/ehsanakhgari/moz/mozilla-central/obj-ff-sharedjs/dist/NightlyDebug.app/Contents/MacOS/firefox > -no-remote -foreground -P pb > <ehsan> XPCOMGlueLoad error 4:0 for file > /Users/ehsanakhgari/moz/mozilla-central/obj-ff-sharedjs/dist/NightlyDebug.app/Contents/MacOS/libicuuc.50.dylib: > <ehsan> Library not loaded: libicudata.50.dylib > <ehsan> Referenced from: > /Users/ehsanakhgari/moz/mozilla-central/obj-ff-sharedjs/dist/NightlyDebug.app/Contents/MacOS/libicuuc.50.dylib > <ehsan> Reason: image not found > <ehsan> Couldn't load XPCOM. > > I think the problem is the lack of @executable_path. Libs need to be linked > with -install_name @executable_path/lib_file_name.dylib. Is that on the linker command line? I'm just setting EXTRA_DSO_LDOPTS in toolkit/library/Makefile.in, do I need to do anything else?
Comment 101•11 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #100) > Is that on the linker command line? I'm just setting EXTRA_DSO_LDOPTS in > toolkit/library/Makefile.in, do I need to do anything else? It's trickier than that. That needs to be done to the icu libs.
Comment 102•11 years ago
|
||
And I see intl/icu/source/config/mh-darwin has this: ifeq ($(ENABLE_RPATH),YES) LD_SONAME = -Wl,-compatibility_version -Wl,$(SO_TARGET_VERSION_MAJOR) -Wl,-current_version -Wl,$(SO_TARGET_VERSION) -install_name $(libdir)/$(notdir $(MIDDLE_SO_TARGET)) else LD_SONAME = -Wl,-compatibility_version -Wl,$(SO_TARGET_VERSION_MAJOR) -Wl,-current_version -Wl,$(SO_TARGET_VERSION) -install_name $(notdir $(MIDDLE_SO_TARGET)) endif So we'd need the ENABLE_RPATH version, with libdir=@executable_path
Comment 103•11 years ago
|
||
So giving --enable-rpath --libdir=@executable_path to configure on mac /might/ work.
Assignee | ||
Comment 104•11 years ago
|
||
Any idea how I can fix the build for the SpiderMonkey "r" and "ggc" builds on the try push? I think the right fix is to disable the ICU rename step in js/src/Makefile.in for SM-only builds, but I'm not sure what the right way to detect such a build is.
Flags: needinfo?(mh+mozilla)
Assignee | ||
Comment 105•11 years ago
|
||
Joel, can you please help me understand this Talos error: <https://tbpl.mozilla.org/php/getParsedLog.php?id=30991292&tree=Try>? My patch is adding one DLL file which Firefox loads and uses. Do I need to annotate that somewhere?
Flags: needinfo?(jmaher)
Comment 106•11 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #104) > Any idea how I can fix the build for the SpiderMonkey "r" and "ggc" builds > on the try push? I think the right fix is to disable the ICU rename step in > js/src/Makefile.in for SM-only builds, but I'm not sure what the right way > to detect such a build is. JS_STANDALONE
Flags: needinfo?(mh+mozilla)
Comment 107•11 years ago
|
||
BTW, you'll need to adjust JSSHELL_BINS.
Comment 108•11 years ago
|
||
:ehsan, this is easy to do, but I see 3 dlls being access: 18:18:20 INFO - TEST-UNEXPECTED-FAIL : xperf: File '{firefox}\icuuc50.dll' was accessed and we were not expecting it. DiskReadCount: 28, DiskWriteCount: 0, DiskReadBytes: 1835008, DiskWriteBytes: 0 18:18:20 INFO - TEST-UNEXPECTED-FAIL : xperf: File '{firefox}\icudt50.dll' was accessed and we were not expecting it. DiskReadCount: 276, DiskWriteCount: 0, DiskReadBytes: 18087936, DiskWriteBytes: 0 18:18:20 INFO - TEST-UNEXPECTED-FAIL : xperf: File '{firefox}\icuin50.dll' was accessed and we were not expecting it. DiskReadCount: 34, DiskWriteCount: 0, DiskReadBytes: 2228224, DiskWriteBytes: 0 This is easy to fix, we have a file that annotates the files accessed during the browser session: http://hg.mozilla.org/build/talos/file/9eddc69c813e/talos/xtalos/xperf_whitelist.json At the moment we only care about the file names, we found the read/write counting was a bit too variable for making a job orange. We do have the read/write counts and bytes added in the file so we have a baseline for future reference. I would like to confirm that we really are reading in 3 dlls and not one as indicated in comment 105. Is this desired and will the startup times be affected?
Flags: needinfo?(jmaher)
Assignee | ||
Comment 109•10 years ago
|
||
(In reply to comment #108) > :ehsan, this is easy to do, but I see 3 dlls being access: > 18:18:20 INFO - TEST-UNEXPECTED-FAIL : xperf: File '{firefox}\icuuc50.dll' > was accessed and we were not expecting it. DiskReadCount: 28, DiskWriteCount: > 0, DiskReadBytes: 1835008, DiskWriteBytes: 0 > 18:18:20 INFO - TEST-UNEXPECTED-FAIL : xperf: File '{firefox}\icudt50.dll' > was accessed and we were not expecting it. DiskReadCount: 276, DiskWriteCount: > 0, DiskReadBytes: 18087936, DiskWriteBytes: 0 > 18:18:20 INFO - TEST-UNEXPECTED-FAIL : xperf: File '{firefox}\icuin50.dll' > was accessed and we were not expecting it. DiskReadCount: 34, DiskWriteCount: > 0, DiskReadBytes: 2228224, DiskWriteBytes: 0 > > This is easy to fix, we have a file that annotates the files accessed during > the browser session: > http://hg.mozilla.org/build/talos/file/9eddc69c813e/talos/xtalos/xperf_whitelist.json > > At the moment we only care about the file names, we found the read/write > counting was a bit too variable for making a job orange. We do have the > read/write counts and bytes added in the file so we have a baseline for future > reference. > > I would like to confirm that we really are reading in 3 dlls and not one as > indicated in comment 105. Yes, reading three DLLs is expected. > Is this desired and will the startup times be affected? Well, I haven't investigated the startup time regression yet, but I do expect to see some regression, which we probably won't be able to avoid. But that's not relevant yet. The more interesting question is, how do I change this file which lives in a different repo at the same time as landing this patch?
Comment 110•10 years ago
|
||
we can land the talos changes effectively now and we won't complain if a file is in the whitelist but not referenced. I will be on holiday for U.S. thanksgiving- here are the steps: 1) patch talos 2) adjust testing/talos/talos.json to point to the new talos revision examples can be found here: https://wiki.mozilla.org/Buildbot/Talos/Misc#Steps_to_add_a_test_to_production I can r+ things as needed, or anybody else can, aklotz has been my partner in crime on the xperf stuff. I did a quick check on the results here and didn't see any large startup regressions.
Comment 111•10 years ago
|
||
Why not link icu with delayload on windows? That should make us not load it during startup.
Assignee | ||
Comment 112•10 years ago
|
||
(In reply to comment #111) > Why not link icu with delayload on windows? That should make us not load it > during startup. I suggested this to Vladan when I was talking to him about it, but I don't know how to do that in our build system unfortunately.
Comment 113•10 years ago
|
||
search for DELAYLOAD in toolkit/library/Makefile.in
Assignee | ||
Comment 114•10 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #103) > So giving --enable-rpath --libdir=@executable_path to configure on mac > /might/ work. --libdir requires an absolute path name and barfs if you don't give it one. I spent several hours today doing everything that I could think of to make this work, but I couldn't. Then I discovered that we now have patches to icu in intl/icu-patches, so I gave up and hacked the icu build config to make this work. At this point I no longer care what the right way to build ICU on Mac as a shared library is any more.
Assignee | ||
Comment 115•10 years ago
|
||
Let's see if this worked: https://tbpl.mozilla.org/?tree=Try&rev=a8bd6c404e87
Assignee | ||
Comment 116•10 years ago
|
||
Here is the correct try push: https://tbpl.mozilla.org/?tree=Try&rev=a5d582fbb763
Assignee | ||
Comment 117•10 years ago
|
||
Requesting review from both Joel and Aaron since Joel might be taking today off...
Attachment #8340355 -
Flags: review?(jmaher)
Attachment #8340355 -
Flags: review?(aklotz)
Assignee | ||
Comment 118•10 years ago
|
||
This is ready for review now.
Attachment #8337090 -
Attachment is obsolete: true
Attachment #8337090 -
Flags: review?(mh+mozilla)
Attachment #8340359 -
Flags: review?(mh+mozilla)
Updated•10 years ago
|
Attachment #8340355 -
Flags: review?(aklotz) → review+
Assignee | ||
Comment 119•10 years ago
|
||
Comment on attachment 8340355 [details] [diff] [review] Whitelist ICU DLLs in Talos https://hg.mozilla.org/build/talos/rev/93e170af571f
Attachment #8340355 -
Flags: review?(jmaher) → checkin+
Assignee | ||
Comment 120•10 years ago
|
||
OK, here's some more information on the patch: Try run for a baseline: <https://tbpl.mozilla.org/?tree=Try&rev=a467b550fcc6> Try run with final touches to fix the build on all platforms: <https://tbpl.mozilla.org/?tree=Try&rev=48fac9b010b4> Try run with delayloading ICU DLLs on Windows: <https://tbpl.mozilla.org/?tree=Try&rev=f492c5803035> Talos comparison on all platforms: <http://compare-talos.mattn.ca/?oldRevs=a467b550fcc6&newRev=a5d582fbb763&server=graphs.mozilla.org&submit=true> Talos comparison with delayloading on Windows: <http://compare-talos.mattn.ca/?oldRevs=a467b550fcc6&newRev=f492c5803035&server=graphs.mozilla.org&submit=true> It looks fairly safe to say that we're not going to take any regressions for the warm startup tests. Windows installer size difference: $ ls -la before after -rw-r--r-- 1 ehsan wheel 28326880 29 Nov 10:02 after -rw-r--r-- 1 ehsan wheel 28040992 28 Nov 22:17 before So we're increasing the download size by 279KB.
Assignee | ||
Comment 121•10 years ago
|
||
At Vladan's request, I measured the impact of this patch on our cold startups (which we don't measure on Talos). Here is the results breakdown for 10 runs of the browser before and after the change: <https://docs.google.com/spreadsheet/ccc?key=0AuDE0NKbf0EOdEVtdzJrUTNVd3ZRWkRyUHVOTzdVb3c&usp=sharing> The summary is that this patch reduces the time before we hit main() by about 30%.
Assignee | ||
Comment 122•10 years ago
|
||
This patch correctly delay-loads the ICU dlls from mozjs as well.
Attachment #808741 -
Attachment is obsolete: true
Attachment #810711 -
Attachment is obsolete: true
Attachment #819130 -
Attachment is obsolete: true
Attachment #8340359 -
Attachment is obsolete: true
Attachment #8340359 -
Flags: review?(mh+mozilla)
Attachment #8340556 -
Flags: review?(mh+mozilla)
Assignee | ||
Updated•10 years ago
|
Summary: Fold mozjs.dll back into xul.dll → Build ICU as a shared library where JS is built as a shared library
Comment 123•10 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #121) > At Vladan's request, I measured the impact of this patch on our cold > startups (which we don't measure on Talos). Here is the results breakdown > for 10 runs of the browser before and after the change: > <https://docs.google.com/spreadsheet/ > ccc?key=0AuDE0NKbf0EOdEVtdzJrUTNVd3ZRWkRyUHVOTzdVb3c&usp=sharing> The > summary is that this patch reduces the time before we hit main() by about > 30%. Which is not surprising, really, considering delay-load makes us not (pre)load them, and xul.dll decreased in size.
Assignee | ||
Comment 124•10 years ago
|
||
(In reply to comment #123) > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #121) > > At Vladan's request, I measured the impact of this patch on our cold > > startups (which we don't measure on Talos). Here is the results breakdown > > for 10 runs of the browser before and after the change: > > <https://docs.google.com/spreadsheet/ > > ccc?key=0AuDE0NKbf0EOdEVtdzJrUTNVd3ZRWkRyUHVOTzdVb3c&usp=sharing> The > > summary is that this patch reduces the time before we hit main() by about > > 30%. > > Which is not surprising, really, considering delay-load makes us not (pre)load > them, and xul.dll decreased in size. s/xul.dll/mozjs.dll/
Comment 125•10 years ago
|
||
Could you also push a try with --enable-shared-js on all platforms? (note b2g fails with that already because some b2g-specific dom code uses a private symbol, so you can skip b2g)
Assignee | ||
Comment 126•10 years ago
|
||
(In reply to comment #125) > Could you also push a try with --enable-shared-js on all platforms? (note b2g > fails with that already because some b2g-specific dom code uses a private > symbol, so you can skip b2g) Sure, here is a full normal try push: <https://tbpl.mozilla.org/?tree=Try&rev=9201d2654183> and here's the try push you requested: <https://tbpl.mozilla.org/?tree=Try&rev=e2e8f0a57208>.
Comment 127•10 years ago
|
||
Comment on attachment 8340556 [details] [diff] [review] Build ICU as a shared library where JS is built as a shared library Review of attachment 8340556 [details] [diff] [review]: ----------------------------------------------------------------- There should be an ICU_VERSION=50 somewhere and that used instead of all the hardcoded 50s, because otherwise, ICU version bumps are going to be painful (and I think we're considering 51 for soon) ::: browser/installer/package-manifest.in @@ +118,5 @@ > +@BINPATH@/libicuuci.50.dylib > +#elif defined(XP_UNIX) > +@BINPATH@/libicudata.so.50 > +@BINPATH@/libicui18n.so.50 > +@BINPATH@/libicuuc.so.50 Since we're not shipping this configuration, I'm not going to block this, but i'm sure this is an open can of worms with fun symbol conflicts with system icu if it happens to be the same version and if some other lib pulls the system one in. Let's just handle this if and when someone hits the problem. ::: config/system-headers @@ +1116,5 @@ > sys/user.h > kvm.h > spawn.h > err.h > xlocale.h You need to keep some form of condition here. One that is only true when building against system icu or when building shared js. ::: configure.in @@ +3908,5 @@ > MOZ_JS_STATIC_LIBS="$MOZ_JS_STATIC_LIBS $MOZ_ICU_LIBS" > fi > > AC_SUBST(MOZ_NATIVE_ICU) > +AC_DEFINE(MOZ_NATIVE_ICU) AC_DEFINE is not AC_SUBST, it doesn't use the value of the env variable. So you're effectively adding -DMOZ_NATIVE_ICU everywhere, in every case. @@ +8877,5 @@ > + *) > + AC_MSG_ERROR([ECMAScript Internationalization API is not yet supported on this platform]) > + esac > + fi > +fi Could you move this in a .m4 file under build/autoconf, and js/src/build/autoconf add it to aclocal.m4 and js/src/aclocal.m4, so that we avoid any desynchronization? ::: js/src/Makefile.in @@ +244,5 @@ > + endif > + else > + ifdef JS_SHARED_LIBRARY > + ICU_LIB_RENAME = $(foreach libname,$(ICU_LIB_NAMES),\ > + cp -p intl/icu/target/lib/$(DLL_PREFIX)$(libname)$(DLL_SUFFIX).50 $(DIST)/bin/$(DLL_PREFIX)$(libname)$(DLL_SUFFIX).50;) Please just use INSTALL_TARGETS for the cases where the copied file isn't renamed at the same time, which is the case for what you added. @@ +419,5 @@ > ifneq (,$(MOZ_ZLIB_LIBS)$(MOZ_GLUE_LDFLAGS)) > DEFINES += -DUSE_ZLIB > endif > > +ifdef JS_SHARED_LIBRARY ifneq (,$(JS_SHARED_LIBRARY)$(MOZ_NATIVE_ICU)), otherwise you're breaking --with-system-icu builds ::: js/src/gdb/Makefile.in @@ +9,5 @@ > LIBS = $(DEPTH)/$(LIB_PREFIX)js_static.$(LIB_SUFFIX) $(NSPR_LIBS) $(MOZ_ZLIB_LIBS) > > LOCAL_INCLUDES += -I$(topsrcdir) -I.. > > +ifdef JS_SHARED_LIBRARY ifneq (,$(JS_SHARED_LIBRARY)$(MOZ_NATIVE_ICU)) ::: js/src/jsapi-tests/Makefile.in @@ +7,5 @@ > LIBS = $(DEPTH)/$(LIB_PREFIX)js_static.$(LIB_SUFFIX) $(NSPR_LIBS) $(MOZ_ZLIB_LIBS) > > LOCAL_INCLUDES += -I$(topsrcdir) -I.. > > +ifdef JS_SHARED_LIBRARY ifneq (,$(JS_SHARED_LIBRARY)$(MOZ_NATIVE_ICU)) ::: js/src/shell/Makefile.in @@ +14,5 @@ > LIBS = $(NSPR_LIBS) $(EDITLINE_LIBS) $(DEPTH)/$(LIB_PREFIX)js_static.$(LIB_SUFFIX) $(MOZ_ZLIB_LIBS) > ifdef MOZ_NATIVE_FFI > EXTRA_LIBS += $(MOZ_FFI_LIBS) > endif > +ifdef JS_SHARED_LIBRARY ifneq (,$(JS_SHARED_LIBRARY)$(MOZ_NATIVE_ICU)) ::: toolkit/library/Makefile.in @@ +129,5 @@ > $(SQLITE_LIBS) \ > $(NULL) > > +ifdef ENABLE_INTL_API > +ifdef JS_SHARED_LIBRARY ifneq (,$(JS_SHARED_LIBRARY)$(MOZ_NATIVE_ICU)) Note it might be worth to add a MOZ_ICU_IS_SHARED variable that'd be true if JS_SHARED_LIBRARY or MOZ_NATIVE_ICU is.
Attachment #8340556 -
Flags: review?(mh+mozilla) → review-
Assignee | ||
Comment 128•10 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #127) > There should be an ICU_VERSION=50 somewhere and that used instead of all the > hardcoded 50s, because otherwise, ICU version bumps are going to be painful > (and I think we're considering 51 for soon) OK, makes sense. Where do you want me to add this variable? I can put it in /configure.in, but then I don't know how to move it js/src/configure.in, and also how to make it available in browser/installer/package-manifest.in similar to @BINPATH@ there. > ::: browser/installer/package-manifest.in > @@ +118,5 @@ > > +@BINPATH@/libicuuci.50.dylib > > +#elif defined(XP_UNIX) > > +@BINPATH@/libicudata.so.50 > > +@BINPATH@/libicui18n.so.50 > > +@BINPATH@/libicuuc.so.50 > > Since we're not shipping this configuration, I'm not going to block this, > but i'm sure this is an open can of worms with fun symbol conflicts with > system icu if it happens to be the same version and if some other lib pulls > the system one in. Let's just handle this if and when someone hits the > problem. OK. > ::: config/system-headers > @@ +1116,5 @@ > > sys/user.h > > kvm.h > > spawn.h > > err.h > > xlocale.h > > You need to keep some form of condition here. One that is only true when > building against system icu or when building shared js. Hmm, so something like |#if MOZ_NATIVE_ICU != 1 && JS_SHARED_LIBRARY != 1|? > ::: configure.in > @@ +3908,5 @@ > > MOZ_JS_STATIC_LIBS="$MOZ_JS_STATIC_LIBS $MOZ_ICU_LIBS" > > fi > > > > AC_SUBST(MOZ_NATIVE_ICU) > > +AC_DEFINE(MOZ_NATIVE_ICU) > > AC_DEFINE is not AC_SUBST, it doesn't use the value of the env variable. So > you're effectively adding -DMOZ_NATIVE_ICU everywhere, in every case. Right... Looking through the patch, I can't see anywhere that I'm using that -D, so I guess I'll take it out. I don't remember why I added it in the first place (could be just thinking that I was adding AC_SUBST.) > @@ +8877,5 @@ > > + *) > > + AC_MSG_ERROR([ECMAScript Internationalization API is not yet supported on this platform]) > > + esac > > + fi > > +fi > > Could you move this in a .m4 file under build/autoconf, and > js/src/build/autoconf add it to aclocal.m4 and js/src/aclocal.m4, so that we > avoid any desynchronization? As discussed on IRC, I will file a follow-up for this. > ::: js/src/Makefile.in > @@ +244,5 @@ > > + endif > > + else > > + ifdef JS_SHARED_LIBRARY > > + ICU_LIB_RENAME = $(foreach libname,$(ICU_LIB_NAMES),\ > > + cp -p intl/icu/target/lib/$(DLL_PREFIX)$(libname)$(DLL_SUFFIX).50 $(DIST)/bin/$(DLL_PREFIX)$(libname)$(DLL_SUFFIX).50;) > > Please just use INSTALL_TARGETS for the cases where the copied file isn't > renamed at the same time, which is the case for what you added. I don't understand what you mean here (and I don't understand what INSTALL_TARGETS does.) Can you please clarify what you're asking me to do here? > @@ +419,5 @@ > > ifneq (,$(MOZ_ZLIB_LIBS)$(MOZ_GLUE_LDFLAGS)) > > DEFINES += -DUSE_ZLIB > > endif > > > > +ifdef JS_SHARED_LIBRARY > > ifneq (,$(JS_SHARED_LIBRARY)$(MOZ_NATIVE_ICU)), otherwise you're breaking > --with-system-icu builds > > ::: js/src/gdb/Makefile.in > @@ +9,5 @@ > > LIBS = $(DEPTH)/$(LIB_PREFIX)js_static.$(LIB_SUFFIX) $(NSPR_LIBS) $(MOZ_ZLIB_LIBS) > > > > LOCAL_INCLUDES += -I$(topsrcdir) -I.. > > > > +ifdef JS_SHARED_LIBRARY > > ifneq (,$(JS_SHARED_LIBRARY)$(MOZ_NATIVE_ICU)) > > ::: js/src/jsapi-tests/Makefile.in > @@ +7,5 @@ > > LIBS = $(DEPTH)/$(LIB_PREFIX)js_static.$(LIB_SUFFIX) $(NSPR_LIBS) $(MOZ_ZLIB_LIBS) > > > > LOCAL_INCLUDES += -I$(topsrcdir) -I.. > > > > +ifdef JS_SHARED_LIBRARY > > ifneq (,$(JS_SHARED_LIBRARY)$(MOZ_NATIVE_ICU)) > > ::: js/src/shell/Makefile.in > @@ +14,5 @@ > > LIBS = $(NSPR_LIBS) $(EDITLINE_LIBS) $(DEPTH)/$(LIB_PREFIX)js_static.$(LIB_SUFFIX) $(MOZ_ZLIB_LIBS) > > ifdef MOZ_NATIVE_FFI > > EXTRA_LIBS += $(MOZ_FFI_LIBS) > > endif > > +ifdef JS_SHARED_LIBRARY > > ifneq (,$(JS_SHARED_LIBRARY)$(MOZ_NATIVE_ICU)) > > ::: toolkit/library/Makefile.in > @@ +129,5 @@ > > $(SQLITE_LIBS) \ > > $(NULL) > > > > +ifdef ENABLE_INTL_API > > +ifdef JS_SHARED_LIBRARY > > ifneq (,$(JS_SHARED_LIBRARY)$(MOZ_NATIVE_ICU)) > > Note it might be worth to add a MOZ_ICU_IS_SHARED variable that'd be true if > JS_SHARED_LIBRARY or MOZ_NATIVE_ICU is. OK, should I add that in top level configure.in, or the js/src one, or both?
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(mh+mozilla)
Updated•10 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Comment 129•10 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #128) > (In reply to Mike Hommey [:glandium] from comment #127) > > There should be an ICU_VERSION=50 somewhere and that used instead of all the > > hardcoded 50s, because otherwise, ICU version bumps are going to be painful > > (and I think we're considering 51 for soon) > > OK, makes sense. Where do you want me to add this variable? I can put it > in /configure.in, but then I don't know how to move it js/src/configure.in, > and also how to make it available in browser/installer/package-manifest.in > similar to @BINPATH@ there. > > > ::: browser/installer/package-manifest.in > > @@ +118,5 @@ > > > +@BINPATH@/libicuuci.50.dylib > > > +#elif defined(XP_UNIX) > > > +@BINPATH@/libicudata.so.50 > > > +@BINPATH@/libicui18n.so.50 > > > +@BINPATH@/libicuuc.so.50 > > > > Since we're not shipping this configuration, I'm not going to block this, > > but i'm sure this is an open can of worms with fun symbol conflicts with > > system icu if it happens to be the same version and if some other lib pulls > > the system one in. Let's just handle this if and when someone hits the > > problem. > > OK. > > > ::: config/system-headers > > @@ +1116,5 @@ > > > sys/user.h > > > kvm.h > > > spawn.h > > > err.h > > > xlocale.h > > > > You need to keep some form of condition here. One that is only true when > > building against system icu or when building shared js. > > Hmm, so something like |#if MOZ_NATIVE_ICU != 1 && JS_SHARED_LIBRARY != 1|? Might be worth defining something with this meaning at configure.in level and AC_SUBST it, considering how broadly this is used. Then add if CONFIG['that_value']: DEFINES['that_value'] = True in config/moz.build. (for both top-level and js/src) > > ::: js/src/Makefile.in > > @@ +244,5 @@ > > > + endif > > > + else > > > + ifdef JS_SHARED_LIBRARY > > > + ICU_LIB_RENAME = $(foreach libname,$(ICU_LIB_NAMES),\ > > > + cp -p intl/icu/target/lib/$(DLL_PREFIX)$(libname)$(DLL_SUFFIX).50 $(DIST)/bin/$(DLL_PREFIX)$(libname)$(DLL_SUFFIX).50;) > > > > Please just use INSTALL_TARGETS for the cases where the copied file isn't > > renamed at the same time, which is the case for what you added. > > I don't understand what you mean here (and I don't understand what > INSTALL_TARGETS does.) Can you please clarify what you're asking me to do > here? Instead of doing manual cp, add something like: ICU_FILES = ... ICU_DEST = $(DIST)/bin INSTALL_TARGETS += ICU See "Install/copy rules" in config/rules.mk for more information This needs to go before including rules.mk. You'll also need to add $(ICU_FILES): buildicu ICU_TARGET = $(if $(MOZ_PSEUDO_DERECURSE),compile,export) to make this work with how icu is built. > OK, should I add that in top level configure.in, or the js/src one, or both? In both (which is why a common m4 would be better). If you're not afraid of shell scripting, I'd advise getting the version number the same way intl/icu/configure.in does (around line 44), so that the version number is always right.
Flags: needinfo?(mh+mozilla)
Assignee | ||
Comment 130•10 years ago
|
||
I filed bug 946687 as a follow-up to factor out more code into icu.m4.
Attachment #8340556 -
Attachment is obsolete: true
Attachment #8343047 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 131•10 years ago
|
||
Fixed a couple of small problems.
Attachment #8343047 -
Attachment is obsolete: true
Attachment #8343047 -
Flags: review?(mh+mozilla)
Attachment #8343148 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 132•10 years ago
|
||
More fixes for --enable-shared-js on Windows.
Attachment #8343148 -
Attachment is obsolete: true
Attachment #8343148 -
Flags: review?(mh+mozilla)
Attachment #8343252 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 133•10 years ago
|
||
Attachment #8343252 -
Attachment is obsolete: true
Attachment #8343252 -
Flags: review?(mh+mozilla)
Attachment #8343281 -
Flags: review?(mh+mozilla)
Comment 134•10 years ago
|
||
Comment on attachment 8343281 [details] [diff] [review] Build ICU as a shared library where JS is built as a shared library Review of attachment 8343281 [details] [diff] [review]: ----------------------------------------------------------------- I'll review next iteration ;)
Attachment #8343281 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 135•10 years ago
|
||
(In reply to comment #134) > I'll review next iteration ;) FWIW the reason I asked review on this patch was that I am trying to expedite things by greening up try and addressing your comments at the same time, because I basically needed to redo all of the testing that I did before I came up with the patch I submitted on Friday since then, and I'm trying to avoid having to do that once again. :(
Assignee | ||
Comment 136•10 years ago
|
||
(And once you're satisfied with a revision of the patch, I can always post interdiffs so that you don't have to review those parts again.)
Assignee | ||
Comment 137•10 years ago
|
||
Attachment #8343281 -
Attachment is obsolete: true
Attachment #8343420 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 138•10 years ago
|
||
Here's a bunch of try runs: With my patch: <https://tbpl.mozilla.org/?tree=Try&rev=0be8794cf727> With my patch and --enable-shared-js: <https://tbpl.mozilla.org/?tree=Try&rev=34520e3d0acd> Without my patch and with --enable-shared-js: <https://tbpl.mozilla.org/?tree=Try&rev=50ed93a04b6c>
Whiteboard: [leave open]
Comment 139•10 years ago
|
||
Comment on attachment 8343420 [details] [diff] [review] Build ICU as a shared library where JS is built as a shared library Review of attachment 8343420 [details] [diff] [review]: ----------------------------------------------------------------- ::: configure.in @@ +7742,1 @@ > JS_SHARED_LIBRARY=1 Obviously, JS_SHARED_LIBRARY is already 1 if you get here :) @@ +8831,5 @@ > + > +dnl Settings for the implementation of the ECMAScript Internationalization API > +if test -n "$ENABLE_INTL_API"; then > + AC_DEFINE(ENABLE_INTL_API) > + You might as well put the MOZ_CONFIG_ICU() here. ::: js/src/configure.in @@ +4208,4 @@ > dnl Settings for the implementation of the ECMAScript Internationalization API > if test -n "$ENABLE_INTL_API"; then > AC_DEFINE(ENABLE_INTL_API) > MOZ_CONFIG_ICU() could go here
Attachment #8343420 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 140•10 years ago
|
||
Comment on attachment 8343420 [details] [diff] [review] Build ICU as a shared library where JS is built as a shared library Review of attachment 8343420 [details] [diff] [review]: ----------------------------------------------------------------- ::: configure.in @@ +7742,1 @@ > JS_SHARED_LIBRARY=1 Haha yeah, sorry, I just did a blind s/ENABLE_SHARED_JS/JS_SHARED_LIBRARY/! @@ +8831,5 @@ > + > +dnl Settings for the implementation of the ECMAScript Internationalization API > +if test -n "$ENABLE_INTL_API"; then > + AC_DEFINE(ENABLE_INTL_API) > + OK, will do.
Assignee | ||
Comment 141•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/099f65a712a8
Comment 142•10 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #141) > https://hg.mozilla.org/integration/mozilla-inbound/rev/099f65a712a8 had to back out this changes for build bustages like https://tbpl.mozilla.org/php/getParsedLog.php?id=31559032&tree=Mozilla-Inbound
Assignee | ||
Comment 143•10 years ago
|
||
This probably requires clobber, relanded: https://hg.mozilla.org/integration/mozilla-inbound/rev/b0678affef03
Assignee | ||
Comment 144•10 years ago
|
||
Filed bug 947226 for the clobber issue.
Comment 145•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b0678affef03
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Comment 146•10 years ago
|
||
We still need some more makefile magic along the lines of http://mxr.mozilla.org/mozilla-central/source/js/src/Makefile.in#186 to be able to #include icu headers in core source files, right?
Flags: needinfo?(ehsan)
Assignee | ||
Comment 147•10 years ago
|
||
(In reply to Simon Montagu :smontagu from comment #146) > We still need some more makefile magic along the lines of > http://mxr.mozilla.org/mozilla-central/source/js/src/Makefile.in#186 to be > able to #include icu headers in core source files, right? Yes, that's correct.
Flags: needinfo?(ehsan)
Updated•10 years ago
|
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•