Remove ast-ty-to-ty cache #41733
Remove ast-ty-to-ty cache #41733
Conversation
|
FYI: The perf impact will be measured for this specific PR as long as it isn't rolled up, so I wouldn't be too concerned with measuring separately. |
| @@ -115,15 +117,18 @@ impl<'l, 'tcx: 'l, 'll, D: Dump + 'll> DumpVisitor<'l, 'tcx, 'll, D> { | |||
| where F: FnOnce(&mut DumpVisitor<'l, 'tcx, 'll, D>) | |||
| { | |||
| let item_def_id = self.tcx.hir.local_def_id(item_id); | |||
| let old_item_cx = mem::replace(&mut self.save_ctxt.item_cx, | |||
| ItemCtxt::new(self.tcx, item_def_id)); | |||
There was a problem hiding this comment.
Hah I didn't expect this. I'm not sure we should have this at all. That is, ItemCtxt is just a (TyCtxt, DefId) pair at this point IIRC, and the item_def_id seems to be the important thing to keep?
There was a problem hiding this comment.
Yeah, I was debating about stripping it out too -- I was thinking I could just make a pub fn in librustc_typeck that takes a def-id and constructs a "fresh itemctxt". I think when I wrote this patch I was anticipating having a cache per ItemCtxt (but then I checked and found that said cache had zero hits).
There was a problem hiding this comment.
I was thinking I could just make a
pub fninlibrustc_typeckthat takes a def-id and constructs a "fresh itemctxt"
That's... what ItemCtxt::new is, isn't it?
src/librustdoc/clean/mod.rs
Outdated
| @@ -53,51 +55,71 @@ use html::item_type::ItemType; | |||
| pub mod inline; | |||
| mod simplify; | |||
|
|
|||
| pub struct CleanContext<'a, 'tcx: 'a> { | |||
| doc_cx: &'a DocContext<'a, 'tcx>, | |||
| item_cx: ItemCtxt<'a, 'tcx>, | |||
There was a problem hiding this comment.
This is... unnecessary? Same thing as before, we really don't need to inflate the diff like this.
Also, remove a lot of `pub` things from `librustc_typeck`.
nikomatsakis
force-pushed
the
incr-comp-remove-ast-ty-to-ty-cache
branch
from
c2ee24d to
3f2dd4d
Compare
|
@eddyb ok, take another look. |
|
@bors r+ |
|
📌 Commit 3f2dd4d has been approved by |
…he, r=eddyb Remove ast-ty-to-ty cache As discussed on IRC, this basically just removes the cache, and rewrites rustdoc and save-analysis so call into the astconv code. It *might* make sense for this to be a more fine-grained query, but that would (at least) require us to be using `HirId` and not `NodeId`. (Perhaps I should open a FIXME?) I didn't measure perf impact (yet?). I did observe that the cache seems to hit *rarely* -- and only in between items (I experimented with a cache "per def-id", but that had zero hits). In other words, every single hit on the cache is a dependency bug, since it is "shuttling" information between items without dependency edges. r? @eddyb
|
💔 Test failed - status-travis |
…he, r=eddyb Remove ast-ty-to-ty cache As discussed on IRC, this basically just removes the cache, and rewrites rustdoc and save-analysis so call into the astconv code. It *might* make sense for this to be a more fine-grained query, but that would (at least) require us to be using `HirId` and not `NodeId`. (Perhaps I should open a FIXME?) I didn't measure perf impact (yet?). I did observe that the cache seems to hit *rarely* -- and only in between items (I experimented with a cache "per def-id", but that had zero hits). In other words, every single hit on the cache is a dependency bug, since it is "shuttling" information between items without dependency edges. r? @eddyb
|
☀️ Test successful - status-appveyor, status-travis |
As discussed on IRC, this basically just removes the cache, and rewrites rustdoc and save-analysis so call into the astconv code. It might make sense for this to be a more fine-grained query, but that would (at least) require us to be using
HirIdand notNodeId.(Perhaps I should open a FIXME?)
I didn't measure perf impact (yet?). I did observe that the cache seems to hit rarely -- and only in between items (I experimented with a cache "per def-id", but that had zero hits). In other words, every single hit on the cache is a dependency bug, since it is "shuttling" information between items without dependency edges.
r? @eddyb