feat: Improve unstable size analysis support

[ChayimFriedman2]

1. Include an option panic_if_missing that will panic if there is an ingredient with no heap_size() defined, to ensure coverage.
2. Add heap_size() to tracked structs, interneds an inputs.

[netlify]

✅ Deploy Preview for salsa-rs canceled.

Name	Link
🔨 Latest commit	d0ceeb4
🔍 Latest deploy log	https://app.netlify.com/projects/salsa-rs/deploys/6872f3beef22be000857eba8

[codspeed-hq]

CodSpeed Performance Report

Merging #935 will degrade performances by 5.56%

_{Comparing ChayimFriedman2:size-analysis-improved (d0ceeb4) with master (d28d66b)}

Summary

❌ 1 regressions
✅ 11 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	BASE	HEAD	Change
❌	amortized[InternedInput]	3.4 µs	3.6 µs	-5.56%

[ChayimFriedman2]

How exactly does this regress performance?

[MichaReiser]

I think it would be great if we can split this PR. 3. is fairly uncontroversial. The use cases for 1 and 2 are less clear to me.

For 1. can you give me an example on what this is uesd for? I'm asking because adding extra arguments makes using it in combination with getsize2 more annoying because it's no longer possible to use the trait method directly

For 2. It prefer an enum as argument rather than a bool. It makes it more expressive what true or false means.

[ChayimFriedman2]

For 1. can you give me an example on what this is uesd for? I'm asking because adding extra arguments makes using it in combination with getsize2 more annoying because it's no longer possible to use the trait method directly

For example, I want to analyze memory usage in rust-analyzer per type and not per query (it is more convenient to draw conclusions this way). This provides a way to do that and more (e.g. what I'm doing now, checking the effect of rust-lang/rust-analyzer#18403) with minimum effort. The disturbance you described is minimal because you can have a single method fn heap_size<T: GetSize>(value: &T, _data: &mut dyn Any) -> usize { ... } and pass it everywhere.

For 2. It prefer an enum as argument rather than a bool. It makes it more expressive what true or false means.

Fair enough.

[ChayimFriedman2]

Edited to use an enum instead of bool.

[MichaReiser]

For example, I want to analyze memory usage in rust-analyzer per type and not per query (it is more convenient to draw conclusions this way). This provides a way to do that and more (e.g. what I'm doing now, checking the effect of rust-lang/rust-analyzer#18403) with minimum effort. The disturbance you described is minimal because you can have a single method

I don't think I fully understand the use case. The table in the linked issue is grouped by query? I think we also have an API to iterate over all memos. Could your impelmentation use that instead?

[ChayimFriedman2]

I want e.g. if I have struct Foo(Vec<Bar>), to not have all memory attributed to Foo, but the Vec to be attributed to Bar. This is because the type hierarchy in rust-analyzer is complex and query-level filtering doesn't tell me a lot.

Currently I have nowhere to store this attribute information (besides a static, which is really ugly). This is what I want to change.

[ChayimFriedman2]

When using this in rust-analyzer I noticed there is still no way to have heap_size for the automatically-generated interned for memos with more than one parameter, but I'll fix that in another PR.

[MichaReiser]

Currently I have nowhere to store this attribute information (besides a static, which is really ugly). This is what I want to change.

I see. This seems very specific. Have you considered using a thread local in your code? That should give you the same without that we need to change salsa for this less common case.

[ChayimFriedman2]

I can do that, but it will be cleaner if it will be a parameter to the function. And this enables a lot of patterns (basically, you can apply any operation to the whole database, you can even select the operation at runtime should there be such need), and the cost is minimal - if you don't use this feature, you need to replace heap_size with a generic function that has the extra parameter, and pass &mut (). And also from a maintenance of Salsa POV, the cost is only an additional parameter in few places.

And FWIW, even with get-size2 itself you may want to share the tracker between queries (e.g. if queries share Arcs) - that's not possible to do without this PR.

And in general, passing an addition data parameter to a callback is a common pattern (especially in C, where there are no closures, but here we can't use closures so it's back to the C idiom).

[MichaReiser]

I can do that, but it will be cleaner if it will be a parameter to the function. And this enables a lot of patterns (basically, you can apply any operation to the whole database, you can even select the operation at runtime should there be such need), and the cost is minimal - if you don't use this feature, you need to replace heap_size with a generic function that has the extra parameter, and pass &mut (). And also from a maintenance of Salsa POV, the cost is only an additional parameter in few places.

I'm not oposed to having such a function that allows analysing the entire db. It only feels to me that this goes beyond what heap_size is intended for and we should maybe consider using a different design for it / generalizing the functionality (and renaming it!) instead of making those changes in place

[ChayimFriedman2]

The design seems perfect to me. The alternative design will be to have a trait for that analysis that is called automatically (if it is implemented, via autoref specialization or explicitly specifying) but that means it'll be foreign for the Salsa user, at least for my framework of size analysis this will cause major trouble, as I need a lot of impls for third-party types.

The naming... I agree it is not ideal. In fact, I originally renamed this function to size_of() (to signify it may do more work than just returning the heap size), but I reverted that change because that name didn't look better for me. In my local prototypes of such feature (before it was implemented in Salsa) I was calling it size_analysis(). But maybe it should have an even more general name, as it can be used for non-size things too (although I have hard time imagining what they could look like). There is also a tension between the fact that it can be used like a simple size counter by returning a number, or it can ignore that completely and do other things. Maybe that suggests we should split the feature into a simple heap_size() and a more complicated function that can do other things, but then I don't see why the simpler function needs to exist, as the only thing that cannot be implemented with the more general framework is calculating the Salsa overhead. And maybe that's indeed what we should do - return the Salsa overhead and only it, and also provide a way to run a custom analysis on ingredient values.

[MichaReiser]

Either way. I suggest splitting the PR. I'm on board with 2 and 3 but still think that 1 is looking for a generic traversal logic rather than what get size provides but I'm interested to hear from others.

[ChayimFriedman2]

Okay. I've omitted 1 from the PR and will open a Zulip discussion.

[MichaReiser]

@ibraheemdev would you mind taking a look at the changes?

[ibraheemdev]

This looks fine to me, but it could use some tests for both the panicking behaviour and the new heap_size attribute on structs.

[ibraheemdev]

Instead of threading the PanicIfHeapSizeMissing everywhere, you could add a has_heap_size method to Ingredient and do a single check that all ingredients return true if PanicIfHeapSizeMissing::Yes is set. You'd need a HAS_HEAP_SIZE constant on all the configurations, but that should be simple to add.

[ibraheemdev]

Or you could have Ingredient::memory_usage return whether or not the heap size was accounted for, and have Configuration::heap_size default to None instead of 0. Either way seems fine.

[ChayimFriedman2]

I actually like the current way more. It is simpler (especially if we want to show the name of the problematic ingredient), and the threading is not that bad. Do you have a preference?

[ibraheemdev]

It's fine if you think that's easier, just an idea. It might be nice to add an is_yes helper method to convert to a boolean though.

[MichaReiser]

Or you could have Ingredient::memory_usage return whether or not the heap size was accounted for,

I'd prefer this approach as it's more flexible (if I understand it correctly). It leaves the decision of what to do if heap size isn't implemented to the caller. ty includes the memory analysis in production builts where panicking isn't an option, but we probably want to log a message if a heap size implementation is missing instead. This isn't possible with PanicIfHeapSizeMissing but is possible if the function returns Option

[ChayimFriedman2]

I don't think @ibraheemdev meant to expose that, just as an internal implementation detail.

If we do expose that, how do we? Do we change IngredientInfo::size_of_fields to Option<usize>?

[MichaReiser]

That's what I had in mind but I see the API works slightly different than I thought. My assumption was that it returns an Iteratror over all memos without doing any aggregation but MemoInfo does a first level of aggregation. @ibraheemdev is this something we could change (return a list of SlotInfo instead).

I also think this requires splitting size_of_fields into two:

• fields_stack_size which returns usize
• fields_heap_size which returns Option<usize>

[ibraheemdev]

Why is the memo aggregation a problem here? If we split size_of_fields I think that should be sufficient to detect whether or not the heap size was tracked for a given memo type.

[MichaReiser]

Why is the memo aggregation a problem here?

The API technically allows for one memo to return Some but None for another (both of the same ingredient) which is a bit awkward to handle. This shouldn't be possible but it isn't something we can express easily in the current API.

[ChayimFriedman2]

I added tests, but I can't make sense of the numbers.

[ibraheemdev]

You should make the fields different to ensure they aren't all interned to the same ID (the interned/tracked structs store the field not the entire input).

[ChayimFriedman2]

Okay I did that, but I still can't make sense of the numbers.

[ibraheemdev]

Which part doesn't make sense? It looks fine to me. The reason for the metadata size increase is alignment (previously the u32 field could fit into the padding).

[ChayimFriedman2]

Why is it 522? It should be 400, no?

[MichaReiser]

capacity can be larger than the String::length. You Can use shrink_to_fit to shrink the String to its length which should get you closer to the expected value

[ibraheemdev]

The capacity is actually exact here, it just also includes the stack size of the strings: 250+150+50+(24*3) = 522.

[ChayimFriedman2]

Ah, got it.