Closed
Bug 941424
Opened 11 years ago
Closed 11 years ago
Build more of the JS engine in unified mode
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla28
People
(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)
References
Details
(Whiteboard: [qa-])
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
djvj
:
review+
|
Details | Diff | Splinter Review |
I needed to disambiguate some names but no other big changes were necessary.
Assignee | ||
Comment 1•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #8335811 -
Flags: review?(kvijayan)
Assignee | ||
Comment 2•11 years ago
|
||
Assignee | ||
Comment 3•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #8335811 -
Attachment is obsolete: true
Attachment #8335811 -
Flags: review?(kvijayan)
Assignee | ||
Updated•11 years ago
|
Attachment #8336000 -
Flags: review?(kvijayan)
Assignee | ||
Comment 4•11 years ago
|
||
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.
Assignee | ||
Comment 5•11 years ago
|
||
Assignee | ||
Comment 6•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #8336000 -
Attachment is obsolete: true
Attachment #8336000 -
Flags: review?(kvijayan)
Assignee | ||
Comment 7•11 years ago
|
||
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)
Assignee | ||
Comment 8•11 years ago
|
||
Assignee | ||
Comment 9•11 years ago
|
||
make check-vanilla-allocations finishes successfully for me on a local 32-bit build. :(
Assignee | ||
Comment 10•11 years ago
|
||
Pushed this to try again: https://tbpl.mozilla.org/?tree=Try&rev=612c89e78c3d
Assignee | ||
Comment 11•11 years ago
|
||
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 12•11 years ago
|
||
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+
Assignee | ||
Comment 13•11 years ago
|
||
(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•11 years ago
|
||
> 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)
Assignee | ||
Comment 15•11 years ago
|
||
That makes perfect sense! I'm trying that out right now: <https://tbpl.mozilla.org/?tree=Try&rev=941da51a7865>
Thanks for the tip!
Assignee | ||
Comment 16•11 years ago
|
||
Comment 17•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Comment 18•11 years ago
|
||
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•11 years ago
|
||
(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.
Assignee | ||
Comment 20•11 years ago
|
||
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•11 years ago
|
||
This change was also in the regression range for a V8 version 7 regression on Windows 8 / x64.
Comment 22•11 years ago
|
||
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•11 years ago
|
||
Bug 944247 show this bug as responsible of a crash.
Updated•11 years ago
|
Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•