i#4881: Fix LRU counter bug in drcachesim #4883
Conversation
|
@derekbruening Simple fix, expect your review, thanks |
There was a problem hiding this comment.
Thank you for the patch! We'll leave a unit test as separate work once infrastructure is in place under #4842.
|
The behavior change slightly changes the miss counts and breaks the two tests |
|
run arm tests |
@EagleEyeKestrel if you could update clients/drcachesim/tests/offline-legacy.templatex and clients/drcachesim/tests/offline-snappy-serial.templatex so that they pass -- easiest by running locally, or look at the log from the failure link above. |
let me try |
…eKestrel/dynamorio into i4881-drcachesim-lru-counter-fix
| @@ -1,5 +1,5 @@ | |||
| Cache simulation results: | |||
| .* | |||
| LL stats: | |||
| Hits: 58,699 | |||
| Hits: 78,312 | |||
There was a problem hiding this comment.
(Wow this is a non-trivial change. The other "legacy" test had very minor changes. Wondering if this LRU issue caused other noticeable result differences on other workloads.)
There was a problem hiding this comment.
Yep. When I looked at this, I was thinking I made something wrong... But it really changes a lot. (BTW, I think this bug in LRU really could result in great difference.
There was a problem hiding this comment.
oh it failed again
There was a problem hiding this comment.
Actually I think that is an unrelated flaky test...checking
There was a problem hiding this comment.
It is #3213 which unfortunately has happened before but nobody has reproduced locally.
There was a problem hiding this comment.
Read log and see some timeout.
There was a problem hiding this comment.
Read log and see some timeout.
If you could paste a link to what you are seeing? Only the Windows test was red.
|
run arm tests |
|
As noted in the thread the failure is win32.setthreadcontext #3213. |
|
Thanks again for finding and fixing this bug! Adding better unit tests (#4842) should be a higher priority to catch things like this. |
Fixes a bug where the LRU counters were never incremented beyond 2, resulting in the incorrect line being replaced in some circumstances. Updates the two stored-trace tests whose precise miss stats are now different. The legacy tests has minor changes; but the snappy test has a big difference in LLC hits, so this bug may have caused non-trivial behavior changes for some workloads. Co-authored-by: Derek Bruening <[email protected]> Fixes #4881
No description provided.