Investigate requiring JM+TI to keep a fully synced interpreter stack

RESOLVED FIXED in mozilla19

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

Other Branch
mozilla19
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(1 attachment)

Assignee

Description

7 years ago
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.
(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

7 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)
Assignee

Updated

7 years ago
Blocks: 778724
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+
(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

7 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.
Whiteboard: [js:t]
https://hg.mozilla.org/mozilla-central/rev/9a5191dfae8d
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Depends on: 806442
You need to log in before you can comment on or make changes to this bug.