Closed
Bug 781657
Opened 12 years ago
Closed 12 years ago
Investigate requiring JM+TI to keep a fully synced interpreter stack
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: bhackett1024, Assigned: bhackett1024)
References
Details
(Whiteboard: [js:t])
Attachments
(1 file)
(deleted),
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
JM+TI's codegen will currently leave torn values on the interpreter stack when they represent dead locals. This has been an ongoing source of pain for exact scanning of the VM stack, and requires complexity in other areas as well. e.g. bug 778724 needs to clone liveness info in a new structure when purging analysis data, so that stack scanning will continues to work.
With Ion coming, there is much less pressure for JM+TI to generate quality code, and it might be worth simplifying this aspect of the compiler, which would allow simplifying and speeding up stack scanning.
Comment 1•12 years ago
|
||
(In reply to Brian Hackett (:bhackett) from comment #0)
> This has been an ongoing source of pain for exact scanning of the VM stack,
I expect that eventually we'll want to use a much simpler compiler as a baseline, but that will be a while. In the meantime, making the GC work easier sounds like a good thing.
Assignee | ||
Comment 2•12 years ago
|
||
With all the complexity bug 778724 adds here it would be better to just not require liveness information at all when scanning the stack. This patch doesn't keep a fully synced interpreter stack, but ensures that type tags are correct and that payloads of objects and strings are correct. This doesn't affect SS -m -n, but dings v8bench by about 2% for me (noisy). That should go away with IM, but I'd like to get this change in first to unblock bug 778724 (which either needs to land soon or be delayed to November).
Assignee: general → bhackett1024
Attachment #653933 -
Flags: review?(wmccloskey)
Comment on attachment 653933 [details] [diff] [review]
ensure types and object/string payloads are synced
Review of attachment 653933 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/methodjit/FrameState.cpp
@@ +389,3 @@
> continue;
>
> + JS_ASSERT(regstate(reg).type() == RematInfo::DATA);
Could you add a comment about this?
Attachment #653933 -
Flags: review?(wmccloskey) → review+
Comment 4•12 years ago
|
||
(In reply to Brian Hackett (:bhackett) from comment #2)
> This doesn't affect SS -m -n, but dings v8bench by about 2% for me (noisy).
> That should go away with IM, but I'd like to get this change in first to unblock
> bug 778724 (which either needs to land soon or be delayed to November).
Hmmm. That mostly seems OK, but I'd really rather not ship a regression if possible. I would suggest landing this patch just after the 18 branch opens up, so that IM can land on top of it right away, but that would mean delaying bug 778724, which also seems bad. Can you say more about exactly what the benefits of this bug are against the 2% V8 regression?
Assignee | ||
Comment 5•12 years ago
|
||
There aren't really any immediate benefits to weigh against the regression here. Another option which I'm leaning towards now is to land bug 778724 first (hopefully tomorrow), without the new liveness logic, and just disable purging until this one lands. Part 2 of bug 778724 in particular will improve compilation behavior / cut recompilations even without being able to purge.
Also, this bug would put an end to all the security critical regressions stemming from bug 723313.
Updated•12 years ago
|
Whiteboard: [js:t]
Assignee | ||
Comment 7•12 years ago
|
||
Comment 8•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in
before you can comment on or make changes to this bug.
Description
•