O(log n) lookup of associated items by name

[ecstatic-morse]

Resolves #68957, in which compile time is quadratic in the number of associated items. This PR makes name lookup use binary search instead of a linear scan to improve its asymptotic performance. As a result, the pathological case from that issue now runs in 8 seconds on my local machine, as opposed to many minutes on the current stable.

Currently, method resolution must do a linear scan through all associated items of a type to find one with a certain name. This PR changes the result of the associated_items query to a data structure that preserves the definition order of associated items (which is used, e.g., for the layout of trait object vtables) while adding an index of those items sorted by (unhygienic) name. When doing name lookup, we first find all items with the same Symbol using binary search, then run hygienic comparison to find the one we are looking for. Ideally, this would be implemented using an insertion-order preserving, hash-based multi-map, but one is not readily available.

Someone who is more familiar with identifier hygiene could probably make this better by auditing the uses of the AssociatedItems interface. My goal was to preserve the current behavior exactly, even if it seemed strange (I left at least one FIXME to this effect). For example, some places use comparison with ident.modern() and some places use tcx.hygienic_eq which requires the DefId of the containing impl. I don't know whether those approaches are equivalent or which one should be preferred.

[ecstatic-morse]

Hopefully the up-front cost of sorting the associated items by name is outweighed by more efficient lookup. If this is not the case, I'll need to switch to an adaptive data structure or consider an out-of-band index that is interned separately.

[ecstatic-morse]

@bors try
@rust-timer queue

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit eb27aaca6450605ed78c29224c4351157357ffd8 with merge 8dc7de89edae6b4f434280089e1a14a36a1ab405...

[bors]

☀️ Try build successful - checks-azure
Build commit: 8dc7de89edae6b4f434280089e1a14a36a1ab405 (8dc7de89edae6b4f434280089e1a14a36a1ab405)

[rust-timer]

Queued 8dc7de89edae6b4f434280089e1a14a36a1ab405 with parent 3f32e30, future comparison URL.

[rust-timer]

Finished benchmarking try commit 8dc7de89edae6b4f434280089e1a14a36a1ab405, comparison URL.

[mati865]

FYI syn-opt is nondeterministic right now: #69060

[eddyb]

Ideally, this would be implemented using an insertion-order preserving, hash-based multi-map, but one is not readily available.

It's not a multi-map, but IndexMap<K, V> is insertion-order preserving and I think we should use it more (see #60608).

[ecstatic-morse]

Looking at check builds, this PR is a slight regression. However, I think it is an acceptable one. I think we should merge this as-is to fix #68957.

Those perf results suggest that name lookup is not a significant component of compile time for most crates, so I probably won't spend much time optimizing this PR. There are many improvements to be made if others are interested. Since the purpose of in_definition_order was to serve as a greppable marker of code that relied on associated item definition order and I don't plan to try to eliminate that code anytime soon, maybe I should replace in_definition_order and its documentation with an iter method?

[Mark-Simulacrum]

I would not replace in_definition_order (an incredibly useful name!) with iter at this point.

[estebank]

I'm +1 on the approach: there's trivial perf degradation but the behavior in pathological cases is much more sane than it is now. I'll review more in depth soon.

cc @rust-lang/compiler for visibility.

[estebank]

Can this be accomplished by actually randomizing the order of items_in_def_order and see what breaks?

[ecstatic-morse]

edited for clarity

I added an in_arbitrary_order method. For now, it's only used in a few places where it cannot possibly affect error message order.

I would like to only use in_definition_order for code that relies on the order for correctness, not just good diagnostics. Many of these places are looking up associated items by index on various lang items. IMO, we should make these methods and associated types lang items as well. AFAICT, the current machinery does not support this.

To migrate away from in_definition_order, we could have in_arbitrary_order return things in non-definition order only during testing. That way, users would continue to get errors in definition order, but we could find out if code was implicitly depending on the definition order of associated items. UI tests complicate this, however.

[Mark-Simulacrum]

We do have -Zui-testing I believe for the ability to change to "deterministic" mode.

Most places that would actually break if the definition order changed are looking up associated items on lang items. IMO, these associated items should themselves be lang items, but AFAICT we don't support that yet.

cc @Centril -- this seems like something we could O(n) process in lang item collection and store HirIds or indices (whatever identified an associated item, I forget) like that for.

[Centril]

cc @Centril -- this seems like something we could O(n) process in lang item collection and store HirIds or indices (whatever identified an associated item, I forget) like that for.

cc @matthewjasper who has taken over my work on lang items in lowering and might have this more in cache.

[ecstatic-morse]

Some unrelated changes have dramatically sped up the pathological case in #68957 on recent nightlies. A local build of a recent master now takes 30 seconds, down from several minutes on 1.41.0. This PR is now just a 2x speedup instead of a 10x one.

[petrochenkov]

FWIW, I prefer in_definition_order, since it's self-documenting.

Ok, it's still a single method whether it's called iter or in_definition_order.

r=me with the nit (#69072 (comment)) addressed and green CI.

[petrochenkov]

@bors r+

[bors]

📌 Commit a0212ba has been approved by petrochenkov

[bors]

⌛ Testing commit a0212ba with merge d3ebd59...

[bors]

☀️ Test successful - checks-azure
Approved by: petrochenkov
Pushing d3ebd59 to master...

[rust-highfive]

📣 Toolstate changed by #69072!

Tested on commit d3ebd59.
Direct link to PR: #69072

💔 clippy-driver on windows: test-fail → build-fail (cc @mcarton @oli-obk @Manishearth @flip1995 @yaahc @phansch @llogiq, @rust-lang/infra).
💔 clippy-driver on linux: test-fail → build-fail (cc @mcarton @oli-obk @Manishearth @flip1995 @yaahc @phansch @llogiq, @rust-lang/infra).