Closed Bug 1501787 Opened 1 year ago Closed 1 year ago

Reset entry counters for IC chain when adding a new stub

Categories

(Core :: JavaScript Engine: JIT, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: mgaudet, Assigned: mgaudet)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

In Bug 1494473, Comment 11, I described the limitations of reasoning that we have with IC entry counters. 

Essentially, because we add stubs at the beginning of the chain, reasoning about relative frequency is challenging. Quoting that comment (edited):

> The output for a particular IC chain looks like this:
> 
>         box2d.js:174:250 (sub)       1009 -> 772 -> (fb) 2
> 
> Which is read like this: This sub opcode has two ICs attached. The first IC was
> entered 1009 times, the second 772 and the fallback stub was hit twice.
> 
> - We cannot however say necessarily that the first IC failed to provide a value
>   771 times.
> 
>   Since new ICs are added at the front, it is possible that there was a phase
>   transition that happened, and the first 771 requests were served by the first
>   IC, followed by a transition to a new type and addition of a new IC, and
>   subsequently 1009 queries were served by the newly added IC.

In Bug 1494473, Comment 12, I proposed a solution: Whenever a new IC is added to the front of the chain, walk the rest of the ICs, and reset the entry counter to 1. This restores the ability to reason about the execution of the chain. If the above chain statistics had been found with that policy in place, we -would- be able to conclude that the first stub provided a value for only 237 out of 1009 queries, which would be the queries since the chain missed and hit the fallback path that produced the frontmost stub. 

Similarly, when stubs are collected, you'd reset the chain counters. 

My initial hesitance for doing this was that I wanted a more global viewpoint of the execution. However, I think overall, it's more important to have an accurate mechanism of reasoning based on the IC chain.
Re-reading the code to addNewStub [1], we are actually inserting new stubs preceding the fallback, but after all the existing stubs. 

This makes this bug invalid.

[1]: https://searchfox.org/mozilla-central/rev/17f55aee76b7c4610a974cffd3453454e0c8de7b/js/src/jit/BaselineIC.h#705-713
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → INVALID
The reasoning in Comment 1 is wrong, but after some discussion about interpreting IC chains entry counters, I came to the conclusion that we still do want to do this.
Assignee: nobody → mgaudet
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Resetting the entered count on an IC stub chain makes interpretation of the
counter values easier. For example, if we see an IC chain like this

    A (1000) -> B(800) -> C (400) -> D (200) -> FB (100)

We can say that there have been 100 cases not handled by chain, (and we did
not attach new stubs for those cases), and B handled the most (400) queries
to the IC chain.
Pushed by mgaudet@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c376952fc919
Reset entered counts on stub attachment r=djvj
https://hg.mozilla.org/mozilla-central/rev/c376952fc919
Status: REOPENED → RESOLVED
Closed: 1 year ago1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.