Stream: t-compiler/rust-analyzer

Topic: Field resolution in hir_ty


Jonas Schievink [he/him] (Apr 06 2021 at 15:07, on Zulip):

I'm looking at

    /// For each field in record literal, records the field it resolves to.
    record_field_resolutions: FxHashMap<ExprId, FieldId>,

in InferenceResult and I'm not sure why this is stored in the inference result. Don't we know the full set of fields when we know the struct or enum variant that is being constructed?

Florian Diebold (Apr 06 2021 at 15:10, on Zulip):

hmm. resolving the variant itself also currently happens during inference, but for both I think there's currently nothing that really requires inference

Florian Diebold (Apr 06 2021 at 15:10, on Zulip):

ah although, not sure if associated types need to be resolved there? we don't do that currently, but there's a fixme there

Florian Diebold (Apr 06 2021 at 15:12, on Zulip):

yeah...

Jonas Schievink [he/him] (Apr 06 2021 at 15:19, on Zulip):

yeah, I was expecting that part to happen in hir_ty

Jonas Schievink [he/him] (Apr 06 2021 at 15:19, on Zulip):

but not resolving individual fields

Jonas Schievink [he/him] (Apr 06 2021 at 15:20, on Zulip):

we could instead change that map to record the VariantId for each Expr::RecordLit

Jonas Schievink [he/him] (Apr 06 2021 at 15:21, on Zulip):

that should slightly reduce memory usage and lets me play around with how we represent field shorthand syntax

Florian Diebold (Apr 06 2021 at 15:22, on Zulip):

oh, yeah ok, that should be fine

Jonas Schievink [he/him] (Apr 06 2021 at 15:24, on Zulip):

oh wait that's already stored

    /// For each struct literal, records the variant it resolves to.
    variant_resolutions: FxHashMap<ExprOrPatId, VariantId>,
Jonas Schievink [he/him] (Apr 06 2021 at 15:59, on Zulip):

opened https://github.com/rust-analyzer/rust-analyzer/pull/8376 to remove it

Jonas Schievink [he/him] (Apr 06 2021 at 16:00, on Zulip):

I wonder if we should remove record_pat_field_resolutions too

Florian Diebold (Apr 06 2021 at 16:12, on Zulip):

that would make sense then I think

Jonas Schievink [he/him] (Apr 06 2021 at 16:13, on Zulip):

though I guess I just turned O(1) lookup into O(n) search over all fields, I kinda expect that to become a problem for people that use structs with thousands of fields

matklad (Apr 06 2021 at 16:21, on Zulip):

huh

matklad (Apr 06 2021 at 16:21, on Zulip):

something regressed benchmark_syntax_highlighting_long_struct a bunch

Jonas Schievink [he/him] (Apr 06 2021 at 16:22, on Zulip):

1000 fields, yeah, that'd do it

matklad (Apr 06 2021 at 16:23, on Zulip):
19:21:34|~/projects/rust-analyzer|HEAD⚡*
λ git rev-parse HEAD
61f15b72ac52c23148038b3867198597b345e2f6

19:21:38|~/projects/rust-analyzer|HEAD⚡*
λ cargo test -q --package ide --lib -- syntax_highlighting::tests::benchmark_syntax_highlighting_long_struct --exact --nocapture

running 1 test
syntax highlighting long struct: 904.13ms, 4161minstr
.
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 437 filtered out; finished in 0.93s

19:21:55|~/projects/rust-analyzer|HEAD✓
λ git switch master
Previous HEAD position was 61f15b72a Add parsing benchmark
Switched to branch 'master'
Your branch is up to date with 'upstream/master'.

19:21:56|~/projects/rust-analyzer|master✓
λ git rev-parse HEAD
7dd7017547c83bb3d33a785047e6da0a1464c0ad

19:22:00|~/projects/rust-analyzer|master✓
λ cargo test -q --package ide --lib -- syntax_highlighting::tests::benchmark_syntax_highlighting_long_struct --exact --nocapture

running 1 test
syntax highlighting long struct: 34.23s, 116ginstr
.
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 525 filtered out; finished in 34.27s
Jonas Schievink [he/him] (Apr 06 2021 at 16:23, on Zulip):

so this does need to be cached, but InferenceResult is the wrong location for that

matklad (Apr 06 2021 at 16:24, on Zulip):

I wonder if that was me actually..... I remember touching something about fields not so long ago. I don't think @Jonas Schievink [he/him] PR is the culprit though.

Jonas Schievink [he/him] (Apr 06 2021 at 16:24, on Zulip):

oh okay, I thought this just happened

matklad (Apr 06 2021 at 16:25, on Zulip):

https://github.com/rust-analyzer/rust-analyzer/issues/8377

matklad (Apr 06 2021 at 16:26, on Zulip):

I didn't bisect this properly, it might have happend just now? I am out of my quant for rust-analyzer for today, won't be able to come back to this until the next week. @Jonas Schievink [he/him] would you like to take a look?

Jonas Schievink [he/him] (Apr 06 2021 at 16:29, on Zulip):

38.18s before my PR, 39.81s after, I guess that didn't cause it

matklad (Apr 06 2021 at 16:31, on Zulip):

900ms after https://github.com/rust-analyzer/rust-analyzer/commit/f7156cb0aeaba8fe32c381a2d676b35d2c86f46f so it isn't me either :P

Jonas Schievink [he/him] (Apr 06 2021 at 18:14, on Zulip):

Hmm, I don't understand how https://github.com/rust-analyzer/rust-analyzer/commit/636de3c709a7c86a1d3a870dc5dc3566310e9d92 has caused this. It must be because of the doc comments on the 1000 fields, but after that commit we still use field_attrs to cache them, just like before.

Laurențiu (Apr 06 2021 at 18:34, on Zulip):

rowan::cursor::NodeData::next_sibling_or_token is pretty high up in the profile, around 14%

Jonas Schievink [he/him] (Apr 06 2021 at 18:49, on Zulip):

oh, we do call source_map from that benchmark

Jonas Schievink [he/him] (Apr 06 2021 at 18:50, on Zulip):

so we end up calling field.parent.child_source() for every field, which is O(n²)

Jonas Schievink [he/him] (Apr 06 2021 at 18:54, on Zulip):

ah, and before that commit I think we never even computed it for fields at all

Jonas Schievink [he/him] (Apr 06 2021 at 18:55, on Zulip):

ah, no, that's not true, we did

matklad (Apr 06 2021 at 19:00, on Zulip):

https://github.com/rust-analyzer/rust-analyzer/pull/8384 -- a draft of the test for this.

matklad (Apr 06 2021 at 19:01, on Zulip):

We are now using machine learning in rust-analyzer (linear regression is clearly ML)

Jonas Schievink [he/him] (Apr 06 2021 at 19:23, on Zulip):

I guess to fix the issue we can use the same trick as variants_attrs and fields_attrs, just for the source map

Jonas Schievink [he/him] (Apr 06 2021 at 20:14, on Zulip):

After that it seems to finish in 2.52s (now in O(n log m) due to AstPtr conversion)

Jonas Schievink [he/him] (Apr 06 2021 at 20:32, on Zulip):

(I'd love to see these benchmarks tracked in the metrics)

matklad (Apr 22 2021 at 11:23, on Zulip):

linear plot-twist: I am now reviewing this code: https://github.com/near/nearcore/pull/4231/files#r618276633

Last update: Jul 27 2021 at 22:45UTC