change stackalloc T[CONST] to stackalloc T[len] if that CONST is >= 512 bytes in order to consume less stack.

I don't know how much of a win it is to use less stack,but in general I think it is preferable to keep a const in for the stack size. It makes it much easier to reason about "how much is going to be stack allocated" during code review and static analysis.

cc @GrabYourPitchforks Since we were just chatting about this.

change stackalloc T[CONST] to stackalloc T[len] if that CONST is >= 512 bytes in order to consume less stack.

I don't know how much of a win it is to use less stack,but in general I think it is preferable to keep a const in for the stack size. It makes it much easier to reason about "how much is going to be stack allocated" during code review and static analysis.

cc @GrabYourPitchforks Since we were just chatting about this.

I don't have a strong preference here, but in some cases it's possible that e.g. len is 4, but we still unconditionally allocate 1024 bytes for it

Related discussion: #97895

#110843 is still not fully fixed. It changes a potential stack overflow in to an access violation. This computation is done unchecked:

Since it is unsigned and unchecked, can overflow to a small positive value

When we try to access memory here:

The address will be invalid.

Can be reproduced on this PR with

[Fact]
public static void BigNumberOfCounters()
{
    CounterSet counterSet = new(Guid.NewGuid(), Guid.NewGuid(), CounterSetInstanceType.Single);

    for (int i = 0; i <= 134_217_726; i++)
    {
        counterSet.AddCounter(i, CounterType.ElapsedTime);
    }

    counterSet.CreateCounterSetInstance("potato");
}

#110843 is still not fully fixed. It changes a potential stack overflow in to an access violation. This computation is done unchecked:

Thanks. Just wrapping the compution into checked context should be enough? There are many patterns where stackalloc size is computed like that, presumably, we should checked them all?

Just wrapping the computation into checked context should be enough?

Seems reasonable.

There are many patterns where stackalloc size is computed like that, presumably, we should checked them all?

I don't see a problem with it. Many will probably be unnecessary, but I don't think it introduces any overhead that is problematic.

For patterns like len > CONST : stackalloc T[CONST] : new T[len] we change stackalloc T[CONST] to stackalloc T[len] if that CONST is >= 1024 bytes in order to consume less stack.

Hardcoded ad-hoc policies like this are never going be "right". The proper fix would be an API like #52065 so that one can just ask for a scratch buffer and leave the choice of strategy to the system.

I don't have a strong preference here, but in some cases it's possible that e.g. len is 4, but we still unconditionally allocate 1024 bytes for it

I think this is a case where we should be ensuring our code is correctly tuned.

If we know the vast majority of cases will be <= n bytes then that's a much better threshold to use than the default max threshold (which is generally 1024, which comes from the typical threshold used by native malloca implementations). If we have effectively two thresholds to handle (most are small, some are slightly larger), then I think that doing 2 explicit stackalloc thresholds are better (if (size <= 64) { stackalloc byte[64]; } else if (size <= 1024) { stackalloc byte[1024]; } else { RentOrAllocateNewArray; }).

Dynamic stackalloc is also "expensive", it introduces a loop that has to probe pages. All our usages will be a single page probe, which means that will be mispredicted by the CPU in the default scenario, incurring an approx 20 cycle penalty. Since stackalloc should not be getting used in known recursive functions or directly in loops, it is unlikely the predictor will ever get that trained correctly and this will be a "permanent" cost. Dynamic stackalloc also means the JIT cannot easily optimize around it, like it currently does with some small fixed allocation sizes.

I also don't think the JIT implicitly doing things to the primitive stackalloc pattern is goodness, it makes code that is likely being used in lowlevel/tuned scenarios less predictable, meaning it can't be used for it's intended scenario. I'd rather instead see us invest in actually exposing our own StackallocOrRent helper that can then do such optimizations itself, giving users the option of convenience vs control.

Notably dynamic length stackallocs are often considered dangerous as well. The pattern we're using here is safe (relatively speaking), but as the pattern gets copied around and used its also easy for people to misunderstand and do the wrong thing. Since we're trying to reduce unsafe code, I think being explicit and adding documentation around these areas and why certain thresholds are used/correct is a better investment; as is pushing for the helper intrinsic that generally simplifies the pattern and abstracts away the "right" sizes to use so that it can be correctly tuned, potentially participate in PGO, etc.

Reverted that part. Added checked everywhere where length is calculated near the stackalloc.
Happy to replace all places with a helper call, but seems like it won't be there soon.

This is ready for review. Here is the code analyzer I used to find all unbound stackallocs: https://github.com/EgorBo/UnsafeCodeAnalyzer/blob/main/src/Analyzer/UnboundStackallocAnalyzer.cs (reports 263 places in Libraries and Corelib excluding tests)