Add support for self-ref fields to the interned macro

[Veykril]

Completely forgot to add it to the macro since r-a had this hand rolled for 2 years now

[netlify]

✅ Deploy Preview for salsa-rs canceled.

Name	Link
🔨 Latest commit	f61e348
🔍 Latest deploy log	https://app.netlify.com/projects/salsa-rs/deploys/6a6dd7a5a75e680008d6c435

[Veykril]

Turns out this feature was very bugged with the LRU we added (fortunately r-a disabled LRU for this)

[Veykril]

Also unsure about the name of this tbh. These late fields are similar to untracked ones in that they do not add to the identity of the interned.

[MichaReiser]

What's the reason that we need to disable LRU collection for self referential structs?

[Veykril]

Ah I think this is leftover from the previous iteration. I think LRU ought to be fine for self-refs

[MichaReiser]

Can you say more why this is necessary? How is it different from any other field that stores another interned struct? I'd expect that the 'db lifetime prevents you from holding a reference to the interned struct, which also prevents you from reading the field. If the value's different, the interned should get a different id.

After chatting with codex. The main issue seems to be that we exclude the self_ref field from hashing, but users can also provide Some(value), where there's no guarantee that value is indeed self. What's the reason that r-a needs the Some(value) constructor where value != self?

[Veykril]

We use this for modeling hygiene which is effectively a graph where the only cycles can be self-referential

See https://github.com/rust-lang/rust-analyzer/blob/master/crates/span/src/hygiene.rs
and the constructor call site https://github.com/rust-lang/rust-analyzer/blob/33815d5957836f6eff67f91aced8420ec5a377d3/crates/hir-expand/src/hygiene.rs#L110-L143

Only allowing the field to actually point to itself would make the feature kind of pointless

[MichaReiser]

Lol, right. If it's always self, then what's the point of it :)

If the field is != self, shouldn't it then be part of the interning key? Or what's the use case where two interned structs with different self_ref (or an instance where self_ref == self and one where self_ref != self). I think that would fix the LRU issue. I think that also suggest that Debug should print the self_ref unless it is self (in which case we can print <self>).

[Veykril]

Well, it just happens to be the case that for our purposes here, only the non-self ref keys are supposed to contribute to the identity. Or rather, the self-ref fields have no need to contribute as due to the way we construct these the self-ref fields will always be the same for the given non-self ref fields.

I can take a look at the key contribution later, I don't remember whether there was an implementation reason or whether it's just that we didn't need it. Though it would be nice if we could keep the key contribution excluded somehow for that minor perf improvement. I think hygiene accesses can be hot in r-a

[MichaReiser]

Couldn't you solve this in r-a by having a wrapper type that overrides Eq and Hash to always be true/0?

[codspeed-hq]

Merging this PR will degrade performance by 5.39%

❌ 1 regressed benchmark
✅ 73 untouched benchmarks
⏩ 1 skipped benchmark¹

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

	Mode	Benchmark	BASE	HEAD	Efficiency
❌	WallTime	scan_resistance[NoEviction]	840 µs	887.8 µs	-5.39%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.

---

_{Comparing Veykril:lukaswirth/push-mqrrrwrpzrnl (f61e348) with master (d312ef8)}

Footnotes

1. 1 benchmark was skipped, so the baseline result was used instead. If it was deleted from the codebase, click here and archive it to remove it from the performance reports. ↩

[MichaReiser]

Sorry for the many questions. We can also move this to Zulip if it's easier. How's the self_ref different from:

• Storing a parent: Option<Node<'db>> on Node<'db>
• Having a public accessor method on Node<'db> that basically is self.parent(db).unwrap_or(self)
• R-a overrides/customizes Hash/Eq so that parent is excluded from hashing?

[Veykril]

no worries

Storing a parent: Option<Node<'db>> on Node<'db>

We used to do this, but it required a lot of annoying branching everywhere iirc (it's been a while).
If I recall, this also results in some extra database hits / accesses (for something that can be fairly hot).

[MichaReiser]

We used to do this, but it required a lot of annoying branching everywhere iirc (it's been a while).

It should be limited to the one method that does the parent or self lookup. All callsites should remain unchanged. But yes, the compiled code will contain more branching.

If I recall, this also results in some extra database hits / accesses (for something that can be fairly hot).

I don't think that should be the case. Both implementations read exactly one field.

[Veykril]

I can try cooking up a branch on r-a that tries to go the previous route again (and also read up on why I wanted this change). I can tell you'd like to get rid of the support code here :^) (which is fair if this feature turns out to not be necessary after all)

[MichaReiser]

I don't mind the feature on its own, if there are cases where it simplifies downstream code. But if it's something that can be implemented downstream without much complexity, it seems desirable not to add more complexity to salsa

[ChayimFriedman2]

Instead of doing all of this, isn't it better to just expose intern() that takes a callback somehow?

(Also why doesn't it play well with interneds GC?)