Simplify and optimize ItemTree
#19982
Conversation
e16bf7b to
1780e48
Compare
|
FWIW, I thought about collapsing all item tree arenas into one, and using an enum in the contents, to reduce memory usage. #19837 was a requirement to that because otherwise it would have hurt incrementality. |
|
Ye figured, your PR is totally a requirement for all the changes here. Sorry if I was stealing planned follow up work from you, I just noticed this and couldn't resist :) |
|
No this is fine :) |
1780e48 to
8930c58
Compare
|
Neat this undoes the memory regression |
2c8f597 to
87083eb
Compare
87083eb to
93da36a
Compare
e18080b to
c1d7c53
Compare
crates/hir-def/src/item_tree.rs
Outdated
| attrs: FxHashMap<FileAstId<ast::Item>, RawAttrs>, | ||
| vis: ItemVisibilities, | ||
| // FIXME: They values store the key, turn this into a FxHashSet<ModItem> instead? | ||
| data: FxHashMap<FileAstId<ast::Item>, ModItem>, |
There was a problem hiding this comment.
Do we actually need a hashmap? I think we can just store an arena, and the ModItemId will be the Idx.
There was a problem hiding this comment.
ModItemId is a typed enum around FileAstId now. Introducing an arena will give us effectively back what FileItemTreeId was
There was a problem hiding this comment.
And what is the advantage of having a wrapper around FileAstId? The advantage of an arena is that it saves more memory.
There was a problem hiding this comment.
Well, if we add back the sequential indices than item re-shuffling will invalidate things here.
There was a problem hiding this comment.
Invalidate what? Nobody depends on the item tree anymore except for the def map, and it'll be invalidated anyway.
There was a problem hiding this comment.
Well, I prefer the current setup as its a lot simpler given we have one less layer of IDs to handle.
The advantage of an arena is that it saves more memory.
I am also not so sure about this, if we turn this into an IndexSet and use the keys of the values contained themselves I do not believe there to be a big memory difference at all.
There was a problem hiding this comment.
Actually we can drop the ast id field from the mod items entirely, you only have access to it if you have they key, which is the ast id already
There was a problem hiding this comment.
Having worked on the more recent commits I am confident that adding another ID layer won't be helpful for us. I highly doubt it will have a observable memory impact and it would also require adding back a resolution indirection to go from that ID back to the AstId while also adding back a good chunk of code I was able to cut entirely.
c1d7c53 to
d8a6aae
Compare
|
I can run this myself shortly, but does this have an impact on cache priming? |
|
Shouldn't. I meant optimize as far as memory is concerned fwiw, I don't think this will have a execution time impact |
#19837 could have an effect on cache priming, due to less things in the item tree! You can try and see. |
|
65e2e2c saved another 9mb |
|
|
63fb37b to
9b78c1f
Compare
9b78c1f to
16ebd29
Compare
Follow up to #19837, closes #9023
There is a bunch of things in the item tree we can replace now as item tree ids are superfluous, we can just replace them with an ast id mapping.