Cache reflection portion of SettingSource lookups

[peppy]

Fixes the mod-related overhead portion of #14638. There's still a significant performance issue that will be addressed separately. Still not sure if this is 100% correct direction but seems to work well enough.

Before:

After:

(gone)

[peppy]

To expand on why I'm not sure about this direction: it doesn't change the fact that in the GetAllMods calls we are constructing mods for throwaway purposes. This is constructing bindables (and in the case of difficulty adjust it's also binding them together) which is not free.

If we go with this cache, it's still something to keep in mind.

[bdach]

This can't work as written, sorry for misleading (as evidenced by test failures). The binding to the generic method happens at compile time, so all readers that call this on Mods (rather than concrete derived types) won't see any properties. Probably have to fall back to a more conventional Type-keyed dictionary setup.

[peppy]

Yeah, was just looking into this, unfortunate.

[peppy]

This............. doesn't work. GetType() carries the same overhead. Send help.

[bdach]

Umm, right... I should have seen that coming but somehow didn't...

The next best thing to use as a cache key (that doesn't touch reflection) would be a (rulesetId, acronym) pair, I guess? Which is awkward since it's not currently easily possible to get the first cheaply from a Mod instance...

Maybe RulesetStore could initialise this setting cache once? Not sure, this is getting into real dicey territory. I really don't want to see the reflection get replaced for some sort of handrolling but it seems to be increasingly inevitable...

[frenzibyte]

The problem with using such way of caching is that SettingsSourceAttribute would then be mods-only code, while I believe it is planned to be used for skin editor and other places (if not used already).

Though looking at it, the method ResolveTypeHandleInternal seems to be one that could come from GetCustomAttribute instead, as that's the stack trace to it (taking Drawable.IsLongRunning as an example source):

It's also noticable there as getSettingsSourceProperties is what shows up in the trace, and the two other methods both call GetCustomAttribute as well.

Still not sure why the overhead of GetCustomAttribute would remain even when caching the properties though, as Object.GetType() is extern and calls an internal method implemented within the CLR, so it shouldn't actually lead to ResolveTypeHandleInternal as far as I'm seeing here.

[bdach]

upon checking again, GetType() should indeed be free.

Though looking at it, the method ResolveTypeHandleInternal seems to be one that could come from GetCustomAttribute instead

still doesn't change the fact that the cache initialisation (so first call to .ToAPIMod() for a given mod type) will still incur this overhead. so if the goal is to remove it always then the cache may, again, need to be eagerly populated.

[peppy]

it’s not the initialization. my profiling was of course done after ensuring the cache was initialized.

[smoogipoo]

As I said in Discord, it is very likely that we're being misled here and it's not actually reflection that's taking a long time but the fact that we're allocating a lot during this.

[Benchmark]
public void GetSettingSources() => mod.GetOrderedSettingsSourceProperties().Consume(new Consumer());

Master:
|            Method |     Mean |    Error |   StdDev |  Gen 0 | Allocated |
|------------------ |---------:|---------:|---------:|-------:|----------:|
| GetSettingSources | 27.91 us | 0.076 us | 0.067 us | 0.0916 |      9 KB |

This PR:
|            Method |     Mean |    Error |   StdDev |  Gen 0 | Allocated |
|------------------ |---------:|---------:|---------:|-------:|----------:|
| GetSettingSources | 27.99 us | 0.095 us | 0.088 us | 0.0916 |      9 KB |

[peppy]

Attempting to drill down into allocations, while there is some overhead on our side, the majority is still coming from GetCustomAttribute internals.

I'll dig deeper and try to isolate a repro case. As you mentioned, it may be async/threading specific, or potentially something to do with the size of the json being parsed here.

[peppy]

Closing this as neither solution is valid. Am investigating today, if I don't come up with a solution I'll open an issue tracking it.

[peppy]

THERE'S NO DIFFERENCE they say....

[peppy]

This is actually valid with the embarrassing fix that none of us caught:

base

Method	Mean	Error	StdDev	Gen 0	Allocated
BenchmarkToModDoubleTime	27,098.0 ns	183.44 ns	171.59 ns	1.0986	12,858 B
BenchmarkToModDifficultyAdjust	120,852.7 ns	946.96 ns	839.46 ns	3.1738	37,118 B
BenchmarkGetAllMods	951.8 ns	5.85 ns	5.19 ns	0.0381	440 B

this PR:

Method	Mean	Error	StdDev	Gen 0	Allocated
BenchmarkToModDoubleTime	6,082.8 ns	58.38 ns	54.61 ns	0.3510	4,089 B
BenchmarkToModDifficultyAdjust	22,210.1 ns	117.00 ns	97.70 ns	0.7935	9,425 B
BenchmarkGetAllMods	968.0 ns	4.97 ns	4.41 ns	0.0381	440 B

[peppy]

Updated benchmarks (with GetAllMods actually being consumed):

master:

Method	Mean	Error	StdDev	Gen 0	Allocated
BenchmarkToModDoubleTime	26.37 us	0.096 us	0.090 us	1.0681	12 KB
BenchmarkToModDifficultyAdjust	118.66 us	0.885 us	0.784 us	3.1738	36 KB
BenchmarkGetAllMods	65.10 us	0.479 us	0.448 us	2.5635	29 KB

this pr:

Method	Mean	Error	StdDev	Gen 0	Allocated
BenchmarkToModDoubleTime	6.176 us	0.0307 us	0.0272 us	0.3510	4 KB
BenchmarkToModDifficultyAdjust	21.568 us	0.1429 us	0.1267 us	0.7935	9 KB
BenchmarkGetAllMods	15.669 us	0.1784 us	0.1669 us	1.3428	15 KB