Stream: t-compiler/rust-analyzer

Topic: Per request scratch spaces (via bumpallo)


matklad (Apr 26 2021 at 11:00, on Zulip):

Discussion thread for https://github.com/rust-analyzer/rust-analyzer/issues/8663

Florian Diebold (Apr 26 2021 at 11:36, on Zulip):

what kind of thing would get put into the scratch space? I'd think the biggest parts of the work happen behind salsa queries. Maybe that's not true for things like highlighting

matklad (Apr 26 2021 at 11:39, on Zulip):

Nothing in particular, but it feels like we might be doing some allocs:

λ rg collect crates/ide/ | wc -l
40

matklad (Apr 26 2021 at 11:39, on Zulip):

assists often allocate a buch for the final edit

matklad (Apr 26 2021 at 11:41, on Zulip):

but yeah, threading this to the salsa would be nice. Although it's unclear how to best to that

matklad (Apr 26 2021 at 11:41, on Zulip):

a naive apporach is to use one scratch space for the whole end-to-end query, but I fear that that might be too much

matklad (Apr 26 2021 at 11:42, on Zulip):

like, the root query might be "typecheck the world", and, in that situation, not freeing anything at all until the very end seems to be excessive.

matklad (Apr 26 2021 at 11:48, on Zulip):

I guess, in an ideal world this should look like this:

In this scheme, unlike "a scratch space for top-level query", we only keep scrats space for the stack of active queries, and that's bounded

matklad (Apr 26 2021 at 11:50, on Zulip):

So, eg, this Vec looks like it could ideally be allocated onto some arena: https://github.com/matklad/rust-analyzer/blob/7bb9c147c0e4815ff8ed48dfee1d8267133bbef7/crates/hir_ty/src/method_resolution.rs#L904

matklad (Apr 26 2021 at 11:51, on Zulip):

Although maybe what we need here is just alloca and huge stacks?

Florian Diebold (Apr 26 2021 at 11:51, on Zulip):

I was about to ask, why not just create a new arena owned by the query function? (or maybe those arenas could be pooled)

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

Oh, does this show up in profiles?

matklad (Apr 26 2021 at 12:01, on Zulip):

Yeah, that's a better way to do that :)

matklad (Apr 26 2021 at 12:01, on Zulip):

@Jonas Schievink [he/him] this = allocations?

Jonas Schievink [he/him] (Apr 26 2021 at 12:01, on Zulip):

yeah

matklad (Apr 26 2021 at 12:02, on Zulip):

Not really, and that's why I am conflicted here

matklad (Apr 26 2021 at 12:02, on Zulip):

Doing per-request arena is obviously the right way to approach this on the one hand, and it has significant architectural implications (retro-fitting arenas is painful).

matklad (Apr 26 2021 at 12:03, on Zulip):

But it is probably not the biggest perf sink at the moment for us

Jeremy Kolb (Apr 26 2021 at 16:25, on Zulip):

I wonder if you could hide this with TLS or something for each request.

bjorn3 (Apr 26 2021 at 16:30, on Zulip):

No borrows of anything inside of a thread local can escape the MY_TLS_VAR.with(|my_tls_var| /*...*/) closure without unsafe code. That includes anything allocated inside the arena. I think it won't be easy to make the unsafe code sound.

Last update: Jul 26 2021 at 14:30UTC