Closed Bug 941424 Opened 7 years ago Closed 7 years ago

Build more of the JS engine in unified mode

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 2 obsolete files)

I needed to disambiguate some names but no other big changes were necessary.
Attachment #8335811 - Flags: review?(kvijayan)
Attachment #8335811 - Attachment is obsolete: true
Attachment #8335811 - Flags: review?(kvijayan)
Attachment #8336000 - Flags: review?(kvijayan)
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.
Attachment #8336000 - Attachment is obsolete: true
Attachment #8336000 - Flags: review?(kvijayan)
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?
Attachment #8336096 - Flags: review?(kvijayan)
make check-vanilla-allocations finishes successfully for me on a local 32-bit build. :(
Pushed this to try again: https://tbpl.mozilla.org/?tree=Try&rev=612c89e78c3d
Nick, any idea how I can reporduce these check-vanilla-allocations failures locally on Linux so that I can debug them?
Flags: needinfo?(n.nethercote)
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?
Attachment #8336096 - Flags: review?(kvijayan) → review+
(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.
> 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?
Flags: needinfo?(n.nethercote)
That makes perfect sense!  I'm trying that out right now: <https://tbpl.mozilla.org/?tree=Try&rev=941da51a7865>

Thanks for the tip!
https://hg.mozilla.org/mozilla-central/rev/3b9e118ded0f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
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
(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.
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?
This change was also in the regression range for a V8 version 7 regression on Windows 8 / x64.
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
Depends on: 942421
Bug 944247 show this bug as responsible of a crash.
Depends on: 944247
Depends on: 943463
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.