Skip to content

Conversation

@dcreager
Copy link
Member

A minor cleanup that breaks up a HashMap of an enum into separate HashMaps for each variant. (These separate fields were already how this cache was being described in the big comment at the top of the file!)

@dcreager dcreager added internal An internal refactor or improvement ty Multi-file analysis & type inference labels Feb 20, 2025
@MichaReiser
Copy link
Member

Is there any other motivation other than that this is how it was described in the comment?

I assume the idea of using a single hash map is to reduce the need for allocations (and make the struct slightly smaller). I don't know if this is a worthwhile trade-off.

Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does seem to make codspeed hapy

@dcreager
Copy link
Member Author

Is there any other motivation other than that this is how it was described in the comment?

I assume the idea of using a single hash map is to reduce the need for allocations (and make the struct slightly smaller). I don't know if this is a worthwhile trade-off.

It was partly that and partly that the code seemed cleaner — the enum only existed to be able to fold both cases into a single hashmap, but they really are conceptually different caches that seemed better to store separately. (I would have considered this worthwhile if performance was a wash, but not if it regressed)

@dcreager dcreager merged commit 529950f into main Feb 20, 2025
21 checks passed
@dcreager dcreager deleted the dcreager/defs-by-defs branch February 20, 2025 14:47
@carljm
Copy link
Contributor

carljm commented Feb 20, 2025

Thank you! Yeah I was just trying to reduce the number of hashmaps, but if codspeed is happy with this, it's definitely nicer.

dcreager added a commit that referenced this pull request Feb 20, 2025
…raint-copies

* origin/main:
  [red-knot] Prevent cross-module query dependencies in `own_instance_member` (#16268)
  Specify the `wasm-pack` version for release workflows (#16278)
  [red-knot] Separate `definitions_by_definition` into separate fields (#16277)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal An internal refactor or improvement ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants