Stream: t-compiler/wg-rls-2.0

Topic: Table Memory Usage


matklad (Jul 22 2020 at 15:42, on Zulip):

@Jonas Schievink did a quick patch to drop salsa's tables to see how much space those occupy. That accounts for 200 mb of unaccounted for space:

https://gist.github.com/matklad/adac544d39b540f90c83c7169b1caa46

Jonas Schievink (Jul 22 2020 at 15:44, on Zulip):

Oof

Jonas Schievink (Jul 22 2020 at 15:44, on Zulip):

Not unexpected though

Jonas Schievink (Jul 22 2020 at 15:45, on Zulip):

What if we just collect slot keys instead of the entire map? As in, what https://github.com/salsa-rs/salsa/pull/196 does.
Or is that already how things work on master? I forgot.

matklad (Jul 22 2020 at 15:51, on Zulip):

I think if we can just collect slots with rc=1

matklad (Jul 22 2020 at 15:52, on Zulip):

Hm, or is it a bad idea due to slots being re-used...

matklad (Jul 22 2020 at 15:52, on Zulip):

s/re-used/referenced to from many tables/

matklad (Jul 22 2020 at 16:46, on Zulip):
matklad (Jul 22 2020 at 16:47, on Zulip):

This also clears input memory, so we finally see all the things

matklad (Jul 22 2020 at 16:47, on Zulip):

https://gist.github.com/matklad/d738eedb04152a96c957f3d341400306

Jonas Schievink (Jul 22 2020 at 16:49, on Zulip):

Cool!

matklad (Jul 22 2020 at 16:50, on Zulip):

Heh, I am forgetting that sweep thing is intended for real garbage collection and not for, you know, measuring memory :D

Laurențiu Nicola (Jul 22 2020 at 16:52, on Zulip):

Is MacroArgQuery supposed to be that large?

matklad (Jul 22 2020 at 16:53, on Zulip):

I think we could make it more compact, but, in general, it will be large

matklad (Jul 22 2020 at 16:53, on Zulip):

as in, a lot of code is generated by macros,

Laurențiu Nicola (Jul 22 2020 at 16:54, on Zulip):

It looks like it's a token tree representation of the macro parameters?

Laurențiu Nicola (Jul 22 2020 at 16:54, on Zulip):

(as opposed to macro-generated code)

matklad (Jul 22 2020 at 16:54, on Zulip):

yes

Laurențiu Nicola (Jul 22 2020 at 16:57, on Zulip):

And it's worth caching it instead of computing it on the fly (since we also store the macro expansion result)?

matklad (Jul 22 2020 at 16:57, on Zulip):

It is worth caching because it's a firewall

matklad (Jul 22 2020 at 16:57, on Zulip):

Most syntax changes leave macro args in place

matklad (Jul 22 2020 at 16:58, on Zulip):

though, these all shold be rechecked, I expect ItemTree work might have changed the calculus here

matklad (Jul 22 2020 at 16:59, on Zulip):

Hm....

matklad (Jul 22 2020 at 16:59, on Zulip):

I think I see something which makes zero sense to me

matklad (Jul 22 2020 at 16:59, on Zulip):
   252mb ItemTreeQuery
   121mb CrateDefMapQueryQuery
    59mb BodyQuery
    38mb InferQueryQuery
matklad (Jul 22 2020 at 17:00, on Zulip):

It's unreasonable that body data occupies less space then top-level data

Jonas Schievink (Jul 22 2020 at 17:09, on Zulip):

Indeed :thinking:

matklad (Jul 22 2020 at 17:14, on Zulip):

note that this is without --with-deps, but still

Jonas Schievink (Jul 22 2020 at 17:17, on Zulip):

Hmm, in that case my expectation is:

So maybe this makes sense?

matklad (Jul 22 2020 at 17:30, on Zulip):

A bit, for smaller crates with with deps the ordering is right, but the order of magnitude seems wrong (2x difference or something)

matklad (Jul 22 2020 at 17:30, on Zulip):

this definitelly worth looking into and explaining

Laurențiu Nicola (Jul 22 2020 at 18:03, on Zulip):

matklad said:

Most syntax changes leave macro args in place

Could we track only the text instead? So we don't cache the tt (but recompute it when needed) and cache only the text and the expansion.

matklad (Jul 22 2020 at 18:08, on Zulip):

That sounds like an excellent idea!

Laurențiu Nicola (Jul 22 2020 at 18:12, on Zulip):

How would that work? A new macro_arg_syntax query that returns the SyntaxNode and a #[transparent] macro_arg_tt that does the conversion?

Laurențiu Nicola (Jul 22 2020 at 18:29, on Zulip):

Hmm, that doesn't seem to work, something about SyntaxNode not being Send (but I put it in an Arc, so dunno)

matklad (Jul 22 2020 at 18:30, on Zulip):

you can use GreenNode instead of SyntaxNode

matklad (Jul 22 2020 at 18:30, on Zulip):

SyntaxNodes are thread local

Laurențiu Nicola (Jul 22 2020 at 18:31, on Zulip):

So SyntaxNode::green? But that returns a reference, so..

matklad (Jul 22 2020 at 18:31, on Zulip):

.clone

Laurențiu Nicola (Jul 22 2020 at 18:32, on Zulip):

And it's private

Laurențiu Nicola (Jul 22 2020 at 18:36, on Zulip):

Well, let's see

Laurențiu Nicola (Jul 22 2020 at 18:41, on Zulip):
   186mb MacroArgQuery
    60mb MacroArgSyntaxQuery
Laurențiu Nicola (Jul 22 2020 at 18:46, on Zulip):
Item Collection: 18.076997307s, 829mb allocated 0b resident
Total expressions: 187802
Expressions of unknown type: 1480 (0%)
Expressions of partially unknown type: 700 (0%)
Type mismatches: 466
Inference: 24.968460198s, 1430mb allocated 0b resident
Total: 43.045673099s, 1430mb allocated 0b resident

Item Collection: 18.373088111s, 658mb allocated 0b resident
Total expressions: 187798
Expressions of unknown type: 1480 (0%)
Expressions of partially unknown type: 700 (0%)
Type mismatches: 466
Inference: 25.376398801s, 1234mb allocated 0b resident
Total: 43.74958371s, 1234mb allocated 0b resident
Laurențiu Nicola (Jul 22 2020 at 18:47, on Zulip):

I guess it works?

Jonas Schievink (Jul 22 2020 at 18:49, on Zulip):

Nice!

matklad (Jul 23 2020 at 13:34, on Zulip):

Idle thought: unresolved items (which store paths) should be heavier than resolved items (which store ids), because a path is a path, and an id is u32.

I wonder if we need one-step macro expansion and name resolution lowring step. And whether that would hit cyclic queries, where, to resolve associated types, you'd invoke type inference machinery which needs lowred impls...

Florian Diebold (Jul 23 2020 at 14:13, on Zulip):

resolving associated types is a separate step

matklad (Jul 23 2020 at 14:19, on Zulip):

Well yes, that's exactly the issue.

Like, I imagine that we'd want to lower Self::Item to something which is infailable, and not to a type & Name pair.

matklad (Jul 23 2020 at 14:19, on Zulip):

I mean, lower in a single step, from ItemTree straight to "we know everything is it to know, except fro child items"

Florian Diebold (Jul 23 2020 at 14:32, on Zulip):

well, it needs to be lowered to <Self as SomeTrait>::Item, but that doesn't need type inference

matklad (Jul 23 2020 at 14:42, on Zulip):

Where SomeTrair and Item are ids, right?

matklad (Jul 23 2020 at 14:43, on Zulip):

Hm, I guess that makes sense, Foo::Bar::Baz paths are forbidden without disambiguation

matklad (Jul 23 2020 at 14:43, on Zulip):

This makes the idea slightly more realizable...

Florian Diebold (Jul 23 2020 at 16:29, on Zulip):

yes

Florian Diebold (Jul 23 2020 at 16:29, on Zulip):

and it's just based on the where clauses in scope

Florian Diebold (Jul 23 2020 at 16:30, on Zulip):

that's basically what happens during lowering to HIR in rustc as well, as I understand it

matklad (Jul 23 2020 at 16:32, on Zulip):

Uhu. I think we'll need to be careful with laziness here, but I think a simple rule like "child names are lowered with parent, children are lowered lazy" should work

matklad (Jul 23 2020 at 16:32, on Zulip):

Ie, we should know functions names in impls, but not necessary functions

matklad (Jul 24 2020 at 09:16, on Zulip):

Could we track only the text instead? So we don't cache the tt (but recompute it when needed) and cache only the text and the expansion.

It's interesting that the PR doesn't do exactly that -- it uses green trees, and not token trees. I think memory savings come from the fact that green trees deduplicate nodes, which token trees can't do, because they have identities.

matklad (Jul 24 2020 at 09:16, on Zulip):

Which invites an interesting question -- do we actually need token identities?

Laurențiu Nicola (Jul 24 2020 at 09:17, on Zulip):

We were caching token trees before, now we're only caching green nodes (text?) and computing the token trees right before macro expansion.

Laurențiu Nicola (Jul 24 2020 at 09:18, on Zulip):

But I see your point about green nodes not being necessarily smaller than token trees

Laurențiu Nicola (Jul 24 2020 at 09:19, on Zulip):

Still, 60 MB is a lot. Are you sure that storing the text wouldn't be better?

matklad (Jul 24 2020 at 09:19, on Zulip):

We were caching token trees before, now we're only caching green nodes (text?) and computing the token trees right before macro expansion.

Exactly, we replaced token trees with green trees. Naively, I'd expect that to have little impact on perf (as both are essentially the same tree structure). I think the diff is due to deduplication of green nodes

matklad (Jul 24 2020 at 09:20, on Zulip):

Text would be better, yeah

matklad (Jul 24 2020 at 09:20, on Zulip):

So, I wonder if we can change the repr of token trees to place hygine info on subtrees in a sidetable, and have nodes in a subtree be identified using only position (w deduplication)

Laurențiu Nicola (Jul 24 2020 at 09:24, on Zulip):

matklad said:

Text would be better, yeah

Um, how do I parse the text into a SyntaxNode? That feels a bit wrong.

matklad (Jul 24 2020 at 09:26, on Zulip):

It might even be impossible

matklad (Jul 24 2020 at 09:26, on Zulip):

For recursive macro expansions, there's no text

matklad (Aug 05 2020 at 13:50, on Zulip):

Latests results (without memory unnacounted for): https://github.com/rust-analyzer/rust-analyzer/pull/5494#issuecomment-669204979

Last update: Sep 27 2020 at 13:45UTC