Page MenuHomePhabricator

[LVI] Restructure caching
AcceptedPublic

Authored by nikic on Nov 18 2019, 12:59 AM.

Details

Summary

Variant on D70103. The caching is switched to always use a BB to cache entry map, which then contains per-value caches. A separate set contains value handles with a deletion callback. This allows us to properly invalidate overdefined values.

A possible alternative would be to always cache by value first and have per-BB maps/sets in the each cache entry. In that case we could use a ValueMap and would avoid the separate value handle set. I went with the BB indexing at the top level to make it easier to integrate D69914, but possibly that's not the right choice.

Diff Detail

Event Timeline

nikic created this revision.Nov 18 2019, 12:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 18 2019, 12:59 AM
efriedma accepted this revision.Nov 18 2019, 4:36 PM

Simpler is good, I think. And I like the use of AssertingVH. LGTM.

It would be nice to have a testcase for PR43909? But not critical, I think; the expanded use of AssertingVH should catch any future issues here.

This revision is now accepted and ready to land.Nov 18 2019, 4:36 PM
nikic added a subscriber: reames.

@efriedma When using AssertingVH a bunch of the existing JT/CVP tests start to fail, so I think we don't need a test case that produces an actual collision in the map.

Adding @reames in case he has some thoughts on how we handle caching/invalidation.

uenoku added a subscriber: uenoku.Nov 19 2019, 4:59 AM
fhahn added a subscriber: fhahn.Nov 19 2019, 8:40 AM
jmorse added a subscriber: jmorse.Nov 28 2019, 2:06 AM
reames added a comment.Dec 2 2019, 5:15 PM

Adding @reames in case he has some thoughts on how we handle caching/invalidation.

This looks very reasonable to me. I ran through it and didn't spot anything obviously problematic, so also LGTM.

nikic updated this revision to Diff 232150.Dec 4 2019, 8:38 AM

Don't use AssertingVH for BasicBlock. See D29006 for why this needs to be a PoisoningVH. I believe using AssertingVH should be fine for the Value case though, as have a CallbackVH that should *always* remove these.

This revision was automatically updated to reflect the committed changes.

We're seeing a large memory increase in compilations as a result of this patch: 4.7G -> 6.0G (25%)

Would it be possible to revert this patch while we investigate? I'm trying to upstream a reproduction in the meantime.

nikic added a comment.Dec 20 2019, 1:04 AM

@rupprecht Sure. You'll also have to revert rG21fbd5587cdf.

It's expected that this patch increases memory usage, but that does seem a bit excessive.

Thanks -- the code in question seems to be heavily templated, so it may be a corner case with how those are handled. I'll revert shortly and follow up with some kind of repro, but may be delayed due to holidays.

nikic reopened this revision.Dec 20 2019, 11:28 AM
This revision is now accepted and ready to land.Dec 20 2019, 11:28 AM
nikic added a comment.Sun, Mar 22, 4:04 PM

@rupprecht Ping regarding a reproducing case. I just double checked that this patch is memory usage neutral on ctmark, so I don't really have a starting point on how to improve this. I'm also wondering whether you are using the legacy PM or the new PM?