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)

defect
Not set
normal

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.
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.
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.
Let's see if we can re-merge mozjs into libxul: <https://tbpl.mozilla.org/?tree=Try&rev=fca3fe53c591>
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?
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]
Attached patch WIP (obsolete) — Splinter Review
Attached patch WIP (obsolete) — Splinter Review
Attachment #806872 - Attachment is obsolete: true
Summary: Build ICU with exported symbols → Fold mozjs.dll back into xul.dll
(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 :(
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.
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. :(
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
This seems to be a compiler bug.  Disabling optimizations in JSRuntime::initSelfHosting fixes this locally for me.
(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.
Attached patch WIP 2 (obsolete) — Splinter Review
New try run: https://tbpl.mozilla.org/?tree=Try&rev=9e4f622a4f6d
Attachment #806874 - Attachment is obsolete: true
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)
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 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)
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...
Actually, it's not just shape_...  All of the fields on that JSObject seem to point to invalid addresses...
(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.
Attachment #807367 - Flags: review+
Attachment #807367 - Flags: feedback?(till)
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.
(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.
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.
(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.
Attachment #807367 - Flags: review?(ted) → review+
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... :/
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...
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...
(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.
(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.  :(
I verified locally that letting the compiler emit the destructor itself is enough to work around this bug.
Attachment #808741 - Flags: review?(sphink)
(And we won't need to deoptimize JS_NewGlobalObject)
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 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+
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... :/
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+
(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
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)
(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)
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)
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
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)
(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!
Depends on: 921130
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?
Attachment #810711 - Flags: review?(ted)
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.
Attachment #810711 - Flags: review?(ted) → review+
(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.
(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.  :-)
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+
(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.
(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.
(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...
(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.
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)
Let's see if bug 921130 moves the needle on the PGO memory usage: https://tbpl.mozilla.org/?tree=Try&rev=025d854c2e9b
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)
Possible, I suppose, but the patches here have workarounds precisely for bits of JS _being_ PGOed, no?
Flags: needinfo?(bzbarsky)
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)
(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.
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
(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)
(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.
(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.
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. :/
Attached patch WIP 3 (obsolete) — Splinter Review
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
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. :(
Sigh... Hopefully this will build: https://tbpl.mozilla.org/?tree=Try&rev=a96f9f7d373f
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
Depends on: 925002
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
Blocks: 679220
I need this for locale-aware sorting on IndexedDB too...
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
Attached patch WIP 4 (obsolete) — Splinter Review
Attachment #813276 - Attachment is obsolete: true
(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.
(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.
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)
(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)
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 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-
Blocks: 933631
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)
(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)
glandium: ping?
(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)
Blocks: 922912
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 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?
(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
Depends on: 942043
Yeah, I don't remember exactly what makes the current code work.
(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
<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.
Attachment #822599 - Attachment is obsolete: true
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)
I managed to push the old version of this patch to try! https://tbpl.mozilla.org/?tree=Try&rev=e748de483250
(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?
(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.
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
So giving --enable-rpath --libdir=@executable_path to configure on mac /might/ work.
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)
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)
(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)
BTW, you'll need to adjust JSSHELL_BINS.
: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)
(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?
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.
Why not link icu with delayload on windows? That should make us not load it during startup.
(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.
search for DELAYLOAD in toolkit/library/Makefile.in
(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.
Let's see if this worked: https://tbpl.mozilla.org/?tree=Try&rev=a8bd6c404e87
Here is the correct try push: https://tbpl.mozilla.org/?tree=Try&rev=a5d582fbb763
Requesting review from both Joel and Aaron since Joel might be taking today off...
Attachment #8340355 - Flags: review?(jmaher)
Attachment #8340355 - Flags: review?(aklotz)
This is ready for review now.
Attachment #8337090 - Attachment is obsolete: true
Attachment #8337090 - Flags: review?(mh+mozilla)
Attachment #8340359 - Flags: review?(mh+mozilla)
Attachment #8340355 - Flags: review?(aklotz) → review+
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+
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.
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%.
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)
Summary: Fold mozjs.dll back into xul.dll → Build ICU as a shared library where JS is built as a shared library
(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.
(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/
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)
(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 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-
(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?
Flags: needinfo?(mh+mozilla)
OS: Mac OS X → All
Hardware: x86 → All
(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)
Depends on: 946687
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)
Fixed a couple of small problems.
Attachment #8343047 - Attachment is obsolete: true
Attachment #8343047 - Flags: review?(mh+mozilla)
Attachment #8343148 - Flags: review?(mh+mozilla)
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)
Attachment #8343252 - Attachment is obsolete: true
Attachment #8343252 - Flags: review?(mh+mozilla)
Attachment #8343281 - Flags: review?(mh+mozilla)
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)
(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. :(
(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.)
Attachment #8343281 - Attachment is obsolete: true
Attachment #8343420 - Flags: review?(mh+mozilla)
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 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+
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.
This probably requires clobber, relanded:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b0678affef03
Filed bug 947226 for the clobber issue.
Depends on: 947299
Depends on: 947402
https://hg.mozilla.org/mozilla-central/rev/b0678affef03
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
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)
(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)
No longer depends on: 947683
Depends on: 947981
Depends on: 948876
Depends on: 948301
No longer depends on: 951587
No longer depends on: 956931
Depends on: 960505
Blocks: 960505
No longer depends on: 960505
Depends on: 970123
Depends on: 978784
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.