if (inPlaceTarget is null && !used)

Would it make sense to move this check above the previousif? #Closed

emitAsCachedArrayFromBlob

It doesn't look like extraction of code into this local function is necessary. The only call site is above its definition. #Closed

The main benefit of this extraction is that it makes the codeflow in the main body of the method clearer. The cases become clearer and in particular this last branch is a single case. I've made sure to keep the diff very clean to minimize review overhead.

emitAsCachedArrayFromBlob

Minor: PerhapstryEmitAsCachedArrayFromBlob, to match withtryEmitAsCachedArrayOfConstants, etc. #Closed

return false;

Are we going to get here forobject? Is this intentional? #Resolved

Yes, we get here forobject (seeReadOnlySpanFromArrayOfConstants_Null) and it's intentional

init.ConstantValueOpt == null

I am not sure if caching an array of strings is such a good idea. This will hold on to strings forever, at the same time this could be the only time they are used. #Resolved

Tagging@stephentoub for thoughts

This will hold on to strings forever, at the same time this could be the only time they are used.

As string literals, won't they already be held on to forever as part of interning?

Even without that, this doesn't seem different to me from, say, lambda/delegate caching, where the first time a static lambda is used we cache a delegate to it, and we'll hold onto that delegate forever even if we never use it again.

As string literals, won't they already be held on to forever as part of interning?

To be honest, I do not know the answer. And whether the answer is the same for all flavors of frameworks out there.

this doesn't seem different to me from, say, lambda/delegate caching

Strings could be quite big though. And there could be a lot of them in a single initialization. Also, we do not cache delegates when they are created by usingnew. So, there is some control over that form of caching.

To be honest, I do not know the answer. And whether the answer is the same for all flavors of frameworks out there.

It should be true for both .NET Framework and .NET Core, every time we encounter a string literal we add it to a global hashmap (string interning Stephen mentioned above) where it's essentially rooted forever. Except the cases with unloadable ALCs but I guess it should not be a problem here as well. E.g.

voidTest(boolcond){if(cond)Console.WriteLine("true!!");elseConsole.WriteLine("false!!");}

When JIT compiles this method (on its first execution) it will create string objects for both literals even if one of them (e.g.false!!!) will never be used - we might make it more efficient in future, but it's a current behavior of .NET Framework and .NET Core.

I would even say that the behavior change caused by extraction of a local, or an inline of a local, might come as a big surprise.

There are plenty of situations where that's the case, including in the brand new collection expressions feature.

We would need to revisit every single use of this in dotnet/runtime where the source might be compiled downlevel, as this could regress all of them. Are you planning to do that?

If we decide that the behavior change, which, If remember correctly, was introduced without much discussion at the time (and likely specifically for the benefit of a single component in development at the time) was a mistake, and should be changed, then we will have to decide what to do with the component.

There are plenty of situations where that's the case, including in the brand new collection expressions feature.

Doesn't mean it is a good thing. The nature of differences is not the same, and the impact is quite different. Each situation is somewhat unique.

I agree there is a non-zero chance this causes unexpected behavior for a customer. There are definitely customers out there that generate largestring[] for initialization purposes that are effectively single use. I cannot specifically remember a case where it would combined withReadOnlySpan<string> such that it would trigger this optimization but it's certainly possible.

There are other optimizations we've taken in the past that had the potential to negatively impact customer scenarios. Even simple optimizations like increased method group to delegate caching broke partner teams. It is always going to be a trade off.

The criteria I usually consider is:

1. Is this on the whole going to be an improvement? In this case I believe the answer is yes it's overall going to produce significant wins compared to the potential downside.
2. Is there a reasonable and documented way the user canundo the optimization if it's found to be negative? Consider as an example for method group caching theundo operation was just make the delegate allocation explicit:= new Action(Method) vs.= Method. What is theundo operation here? I believe assigning to an intermediate local would subvert the optimization. Is that the way we want to document? Whatever the answer is I would like for it to be explicitly listed in the PR / issue for customers to see.
3. Are we violating anything in the language specification? For method group to delegate allocation we had to go back and confirm with LDM that they were okay with this change.

Assuming we have resolutions for (2) and (3) I would overall be in favor of moving forward. I tihnk we should consider an entry in the breaking change list though.

Was asked if I could make it clearer which optimization path I preferred: weak reference or not.

I would lean towards starting the non-weak reference approach. My rationale is

1. Customers who find the behavior undesirable can use theundo mechanism
2. If this does produce enough negative customer feedback we could flip to theWeakReference approach in an update / servicing fix.

return false;

Is this code path covered by tests? #Closed

if (constants.IsEmpty)

It looks like we don't needrosArrayCtor to handle this case #Closed

emitEmptyReadonlySpan

Does it mean that empty spans are optimized regardless of element type? If so, consider doing this optimization earlier and in one place, rather than having two different places checking for the same condition. #Closed

Moving that optimization earlier in the existing code affects some scenarios. Since that is not the purpose of the PR, I kept the original optimization in it's existing location.

Moving that optimization earlier in the existing code affects some scenarios.

It doesn't look this way to me. We can discuss offline in more details