941424 - Build more of the JS engine in unified mode

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[(no longer active)]

I needed to disambiguate some names but no other big changes were necessary.

Comment 1

[(no longer active)]

Attachment: Build more of the JS engine in unified mode; r=djvj

Comment 2

[(no longer active)]

https://tbpl.mozilla.org/?tree=Try&rev=fd6e676c977b

Comment 3

[(no longer active)]

Attachment: Build more of the JS engine in unified mode; r=djvj

Comment 4

[(no longer active)]

I'm fixing up the build on try, I'll re-ask for review if I do something substantial to the patch.  So far I've had to explicitly use the js::NullPtr name in three more places.

Comment 5

[(no longer active)]

https://tbpl.mozilla.org/?tree=Try&rev=3c567429eed3

Comment 6

[(no longer active)]

Attachment: Build more of the JS engine in unified mode; r=djvj

Comment 7

[(no longer active)]

Comment on attachment 8336096 [details] [diff] [review]
Build more of the JS engine in unified mode; r=djvj

OK, this should build on Windows too.

I'm getting a bunch of test failures on try for Linux 32-bit:

TEST-UNEXPECTED-FAIL | check_vanilla_allocations.py | 'operator new(unsigned' isn't used as expected in jsutil.cpp
TEST-UNEXPECTED-FAIL | check_vanilla_allocations.py | 'operator new[](unsigned' isn't used as expected in jsutil.cpp
TEST-UNEXPECTED-FAIL | check_vanilla_allocations.py | 'memalign' isn't used as expected in jsutil.cpp
TEST-UNEXPECTED-FAIL | check_vanilla_allocations.py | 'valloc' isn't used as expected in jsutil.cpp
TEST-UNEXPECTED-FAIL | check_vanilla_allocations.py | 'strdup' isn't used as expected in jsutil.cpp
make[1]: *** [check-vanilla-allocations] Error 1

Any idea what these mean?

Comment 8

[(no longer active)]

Windows: https://tbpl.mozilla.org/?tree=Try&rev=83fe1bf13d0c

Comment 9

[(no longer active)]

make check-vanilla-allocations finishes successfully for me on a local 32-bit build. :(

Comment 10

[(no longer active)]

Pushed this to try again: https://tbpl.mozilla.org/?tree=Try&rev=612c89e78c3d

Comment 11

[(no longer active)]

Nick, any idea how I can reporduce these check-vanilla-allocations failures locally on Linux so that I can debug them?

Comment 12

[Kannan Vijayan [:djvj]]

Comment on attachment 8336096 [details] [diff] [review]
Build more of the JS engine in unified mode; r=djvj

Review of attachment 8336096 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jscrashreport.cpp
@@ +49,5 @@
>  #if defined(_MSC_VER) && defined(_M_IX86)
>      /* ASM version for win2k that doesn't support RtlCaptureContext */
>      uint32_t vip, vsp, vbp;
>      __asm {
> +    MyLabel:

What's the reason for the change from Label to MyLabel?

Is some other cpp macro hogging Label?

Comment 13

[(no longer active)]

(In reply to comment #12)
> Comment on attachment 8336096 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=8336096
> Build more of the JS engine in unified mode; r=djvj
> 
> Review of attachment 8336096 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jscrashreport.cpp
> @@ +49,5 @@
> >  #if defined(_MSC_VER) && defined(_M_IX86)
> >      /* ASM version for win2k that doesn't support RtlCaptureContext */
> >      uint32_t vip, vsp, vbp;
> >      __asm {
> > +    MyLabel:
> 
> What's the reason for the change from Label to MyLabel?
> 
> Is some other cpp macro hogging Label?

It's not a macro, it's another name (perhaps a function name) which causes the assembler to not know which address to take.  I did't bother figuring out where that name comes from since that doesn't matter.

Comment 14

[Nicholas Nethercote [inactive]]

> make check-vanilla-allocations finishes successfully for me on a local
> 32-bit build. :(

Is that a unified build?

check_vanilla_allocations.py looks at the output of 'nm' and expects to find certain symbols (memalign, etc) coming from jsutil.cpp.  I suspect that with unified builds the name of the file that gets compiled is something else, and that's the cause of the failure.

I just added this checking, it's important for ensuring that SpiderMonkey doesn't use vanilla allocators -- SM should always use js_malloc, js_calloc, etc.

Does it fix the problem if you just omit jsutil.cpp from the UNIFIED_SOURCES?

Comment 15

[(no longer active)]

That makes perfect sense!  I'm trying that out right now: <https://tbpl.mozilla.org/?tree=Try&rev=941da51a7865>

Thanks for the tip!

Comment 16

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b9e118ded0f

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3b9e118ded0f

Comment 18

[Nicolas B. Pierron [:nbp]]

I know this is likely to be caused by code locality, and we cannot do much against it.  Still, it seems that moving to the unified mode caused some regressions on octane-Box2D (~8%), kraken-parse-financial (~10% or more with the xpconnect patch) reported by AWFY on B2G.

Do we order these files based on the frequency of jumps between translation units?

http://arewefastyet.com/#machine=14&view=breakdown&suite=kraken
http://arewefastyet.com/#machine=14&view=breakdown&suite=octane

Comment 19

[Andrew McCreight [:mccr8]]

(In reply to Nicolas B. Pierron [:nbp] from comment #18)
> Do we order these files based on the frequency of jumps between translation
> units?

IME they are ordered based on the alphabet.

Comment 20

[(no longer active)]

Yes they are ordered alphabetically.  This is kind of surprising to me, and I'm not sure if I can read those graphs accurately (they seem very noisy to me).  Can you try backing out this patch locally to see if it's indeed the culprit?

Comment 21

[Matt Brubeck (:mbrubeck)]

This change was also in the regression range for a V8 version 7 regression on Windows 8 / x64.

Comment 22

[Frederic Bezies]

This change seems to be responsible for breaking xpcshell when it is built with gcc 4.8.2.

See https://bugzilla.mozilla.org/show_bug.cgi?id=942421#c6 and https://bugzilla.mozilla.org/show_bug.cgi?id=942421#c10

Comment 23

[Hubert Figuiere [:hub]]

Bug 944247 show this bug as responsible of a crash.