perf: Do not really expand builtin derives, instead treat them specifically #21200
Conversation
rustbot
added
the
S-waiting-on-review
|
Nice, besides huge memory savings this also save a nice amount of time (5s/60s on self, 30s/410s on omicron). |
ChayimFriedman2
force-pushed
the
fake-impls
branch
5 times, most recently
from
e2c99f4 to
e8e48c2
Compare
ChayimFriedman2
force-pushed
the
fake-impls
branch
5 times, most recently
from
d17c961 to
9d90c88
Compare
rustbot
removed
the
S-waiting-on-review
ChayimFriedman2
force-pushed
the
fake-impls
branch
from
9d90c88 to
d12cf2b
Compare
rustbot
added
the
S-waiting-on-review
|
This is now ready, after #21295 was merged. |
ChayimFriedman2
force-pushed
the
fake-impls
branch
2 times, most recently
from
cfe6d56 to
6181d54
Compare
…tore them unexpanded
ChayimFriedman2
force-pushed
the
fake-impls
branch
from
6181d54 to
15ef317
Compare
Veykril
left a comment
Veykril
left a comment
There was a problem hiding this comment.
The amount of extra code this introduces does make me sad I have to say. but I can't really argue against the memory savings here :)
|
|
||
| @non_lang_core_traits: | ||
| core::default, Default; | ||
| core::fmt, Debug; | ||
| core::hash, Hash; | ||
| core::cmp, Ord; | ||
| core::cmp, Eq; |
There was a problem hiding this comment.
I am not a fan of this change. These are not lang items, so we should really not record them as lang item variants imo
There was a problem hiding this comment.
I can understand that, but resolving them each time will be expensive - this is a common operation (getting the TraitRef requires their ID). We can of course put them on a different query, but I don't see why. Or we can rename this module/struct, but it'll have some churn.
| // FIXME: This file is commented out because due to the fast path for builtin derives, | ||
| // the macros do not really get expanded, and we cannot check their expansion. | ||
| // It's not removed because we still need to find some way to do that nevertheless. | ||
| // Maybe going through the list of registered derive calls in the def map? |
There was a problem hiding this comment.
We could just have a hacky test flag to toggle behavior maybe?
There was a problem hiding this comment.
Hmm, not a fan of this idea. Also then how will we test that the actual code works?
There was a problem hiding this comment.
Well I mean toggling it just for this expansion test here specifically, not for the rest of the test infra.
Most of the new code is when trying to expose the new impls to the IDE layer, we can not do it if we give up on the idea of making the new impls seem like regular impl to the IDE layer, which will include that navigating to their function will navigate to the trait instead of the derive. |
|
Yea I get that, don't think of this comment as something we need to resolve (I don't think we can really shave any code down here), so all good. |
ChayimFriedman2
force-pushed
the
fake-impls
branch
from
15ef317 to
7888e27
Compare
It sees them as regular impls; the details are abstracted. It's beautiful for the IDE layer, and less beautiful for `hir`, so this is a big change. Some small differences still exist: - We show builtin derives impl (to the IDE layer) as if they have had no generic parameters. It is possible to show the parameters, but that means also having to handle fake impls in `TypeParam` etc., and the benefit is questionable. - Getting the fn *def* type of a method of a builtin derive impl is not supported, as there is no real `FunctionId`, therefore no `CallableDefId`. The trait method is returned instead. Note: getting the fn *ptr* type of the method is supported well. - Builtin derive impls and their methods do not fully support `HasSource`, because, well, they have no source (at least, not in the form of `ast::Impl` and `ast::Fn`). To support them, we use the derive's `TextRange` where possible, and the trait method's source when not. It's important to note that the def map still records the `MacroCallId`. I have doubts over this, as this means it's very easy to create the queries we don't want to create, but it does make things more convenient. In particular, a nicety of this setup is that even "Expand macro recursively" works (it creates the macro input/output query, but given that they will only be created when the user invokes the command, that does not seem to be a problem).
Via a hack to disable their fast path.
ChayimFriedman2
force-pushed
the
fake-impls
branch
from
7888e27 to
6ed7084
Compare
|
@Veykril addressed comments. |
rustbot
removed
the
S-waiting-on-review
3 participants
That gives us some very nice memory gains, as I explained on Zulip.