Created attachment 487464 [details] [diff] [review] Patch Andreas suggests this either for b7 or for trunk. I could go either way, but looking at where some of these checks lie, I wonder at the performance impact of doing so in a beta release where we're aiming to make quite a perf-impact splash. I haven't checked, but I'd be willing to bet these checks, scattered and ubiquitous as they are, would be enough to take us from in the lead on SunSpider to at least a few milliseconds back from the leaders. (Sure, we'll get to the same place -- in the lead -- in the end, but we can make more hay from a win than we can from a not-quite win plus a later, much-smaller improvement.) Worth doing only for trunk builds then, maybe? *shrug* We report, drivers decide.
Attachment #487464 - Flags: review?(gal)
Comment on attachment 487464 [details] [diff] [review] Patch Thanks Waldo. Lets put this onto trunk after b7 branched.
Attachment #487464 - Flags: review?(gal) → review+
I pushed this to TM -- next m-c merge should pick it up. http://hg.mozilla.org/tracemonkey/rev/9e5cd5815d4d Although, hm, that means it'll show up on awfy and stuff. Pushing just to m-c will have it show up as an issue at merge time. I don't see a clean way to keep up appearances ;-) yet get testing with this. If someone has particular thoughts on this, speak up -- easy enough to do something different than I've done so far.
Target Milestone: mozilla2.0b7 → mozilla2.0b8
Hm, backed this out -- was hitting orange, oddly enough, trying to do the assertion underneath JS_IdToValue for the nsDOMClassInfo::GetArrayIndexFromId call spot. Some tinderboxen just show it as a crash in one of the inlined compartment methods, but at least one box is showing a crash from within the assert I made always-fatal: http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1289269411.1289269510.20772.gz&fulltext=1#err1
By the way, the regress graphs on awfy seem to show this as a huge regression for TM (but not for JM or JM+TM?).
I was surprised to see that too, but I think I know the answer: the checks affect the interpreter only, and we spend substantial time in the interpreter in TM-only mode.
Yeah, I roughly expected a perf regression from this, most probably from doing same-compartment assertions for every single interpreter stack push. (Yes, really.) That's why this would be an in-during-development, out-for-betas thing. I don't have time to investigate the orange that tinderboxen were sometimes picking up on this, unfortunately. I'm busy on the last big-ticket part of strict mode, and without that I'm not even sure if I'd want to ship strict mode without it.
We need this to go into the tree right after b8 ships, and we have to get the tree green, before landing bug 605662 (per-compartment gc).
These are the remaining crashes. All of them include the JS_IdToValue call at the end. http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1292411833.1292412035.11011.gz&fulltext=1#err0 http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1292411862.1292412587.14079.gz&fulltext=1#err0 http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1292420737.1292421340.25741.gz&fulltext=1#err4 http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1292407812.1292408309.24718.gz&fulltext=1#err4
Removing the inline for the checks solves the problem on my machine. I pushed to try again.
Us crashing whether its "inline" or not is super scary. We need a deeper understand what happens there.
Created attachment 498420 [details] [diff] [review] Version with JS_NEVER_INLINE this patch fails make check for an optimized build shell for /js/src/jit-test/tests/jaeger/bug549602.js The address looks similar to the browser cause: Program received signal EXC_BAD_ACCESS, Could not access memory. Reason: KERN_INVALID_ADDRESS at address: 0x00007ffffffff000 0x000000010001de79 in js::assertSameCompartment<unsigned long long> () (gdb) bt #0 0x000000010001de79 in js::assertSameCompartment<unsigned long long> () #1 0x0000000100019882 in JS_ValueToString () (gdb)
Whiteboard: [compartments] → [compartments][softblocker]
As per today's meeting, beta 9 will be a time-based release. Marking these all betaN+. Please move it back to beta9+ if you believe it MUST be in the next beta (ie: trunk is in an unshippable state without this)
blocking2.0: beta9+ → betaN+
The remaining crashes with this patch all have a similar stack: 1 libxul.so!JS_Assert [jsutil.cpp : 83 + 0xb] eip = 0x01bfd54e esp = 0xbf8a1540 ebp = 0xbf8a1598 Found by: previous frame's frame pointer 2 libxul.so!js::CompartmentChecker::check [jscntxtinlines.h : 541 + 0x1f] eip = 0x01adf5e4 esp = 0xbf8a1560 ebp = 0xbf8a1598 ebx = 0x0214101c esi = 0xbf8a1598 Found by: call frame info 3 libxul.so!JS_IdToValue [jscntxtinlines.h : 557 + 0xb] eip = 0x01ad4cc0 esp = 0xbf8a1580 ebp = 0xbf8a1598 ebx = 0x0214101c esi = 0xafd19fc0 Found by: call frame info 4 libxul.so!nsDOMClassInfo::GetArrayIndexFromId [nsDOMClassInfo.cpp : 4235 + 0xe] eip = 0x0135cfc2 esp = 0xbf8a15b0 ebp = 0xbf8a1628 ebx = 0x0214101c esi = 0xafd19fc0 edi = 0x995cda60 Found by: call frame info *** Compartment mismatch 0xb42fe000 vs. 0x565101, defaultComp: 0xb5d98000 I don't think we have a compartment mismatch here because the actual object doesn't seem to be heap allocated. 0xb42fe000 is the expected compartment and 0x565101 is the compartment pointer for the current object. That's definitely not a compartment pointer. http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1294298969.1294299572.30791.gz&fulltext=1#err4
** PRODUCT DRIVERS PLEASE NOTE ** This bug is one of 19 being moved from blocking2.0:betaN+ to blocking2.0:- as we reached the endgame of Firefox 4. The rationale for the move is: - the bug had been identified as a "soft" blocker which could be fixed in some follow up release - the bug had been identified as one requiring beta coverage, thus is not appropriate for a ".x" stability & security release The owner of the bug may wish to renominate for .x consideration.
blocking2.0: betaN+ → .x+
(er updating flag to "-" as per previous comment!)
blocking2.0: .x+ → -
I looked at this a while ago and it seems to trigger a compiler bug. Jesse asked me to add a dependency on the MSVC 2010 upgrade, since that would probably fix it.
Depends on: 563318
Whiteboard: [compartments][softblocker] → [sg:want][compartments][softblocker]
Do we use strict aliasing on windows? This looks a bit like 684526.
I thought strict aliasing was a gcc-only thing...
You need to log in before you can comment on or make changes to this bug.