📝 I'm not 100% sure about this part. Without it, we leave something on the stack which results in an assertion when the subsequent statement gets emitted. #Resolved

I'm not 100% sure about this part.

When we are relying on a constructor, I think we can handleinPlaceTarget case the same way it is handled inif (specialElementType.SizeInBytes() == 1) above. We also should be able to handle it inemitAsCachedArrayFromBlob, but we never reach it due to a similar early check. Might be worth adjusting that as well.

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.

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

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.

if (constants.IsEmpty)

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

ByteSequenceComparer.Equals(x.Constants, y.Constants);

I am not sure what method is called here. Could you clarify please? #Closed

That was just wrong :-/ We're dealing withConstantValue not bytes here. This was definitely not doing what I intended... Thanks for catching this.

return _singleZeroByteArray;

Is this branch reachable? #Closed

Yes, for null constants. I'll add a comment

for null constants. I'll add a comment

Consider usingConstantValueTypeDiscriminator.Null instead. "Nothing" is VB term.

It doesn't look like we depend on this condition below. If we are not handling a case, the code is going to throw below regardless of underlying value ofConstantValueTypeDiscriminator.DateTime. Instead, it might be better to add a comment inConstantValueTypeDiscriminator that new values are likely to need a special treatment in this function. #Closed

Sure, I'll shift to a comment.
That said this assertion would fail for existing tests as soon as a newConstantValueTypeDiscriminator enum entry was added, before the right test was crafted. That would have also raised the right alarm.

BitConverter.GetBytes(constant.Int32Value)

The order of bytes in the array returned by the GetBytes method depends on whether the computer architecture is little-endian or big-endian. So, build on different machines might produce different bytes for the same value. Is this a concern given the purpose of this helper? #Closed

return BitConverter.GetBytes(constant.DateTimeValue.Ticks);

Is this code path covered by tests? #Closed

CompileAndVerify(compilation, verify: Verification.Skipped);

Consider verifying content of the array at runtime. #Closed

WorkItem("#69472")

Did the behavior of this test or the next test change with this PR? #Resolved

No, these two tests behave the same as before. This was to ensure there was no impact on existing scenarios. We probably have these two covered already in some fashion, but it was easier this way

}

Consider testingnew int?[] { null }. #Resolved

verify: Verification.Skipped

Why is verification skipped for this scenario? #Closed

PEVerify failed for assembly 'C:\Users\jcouv\AppData\Local\Temp\RoslynTests':[ : C::MString][mdToken=0x6000006][offset 0x00000010] Cannot change initonly field outside its .ctor.[ : C::MObject][mdToken=0x6000007][offset 0x00000010] Cannot change initonly field outside its .ctor.[ : C::MC][mdToken=0x6000008][offset 0x00000010] Cannot change initonly field outside its .ctor.[ : C::MC][mdToken=0x6000008][offset 0x00000015][found ref array mdarray 'System.Object[]'][expected ref array mdarray 'C[]'] Unexpected array type on the stack.

and

System.Exception : IL Verify failed unexpectedly:[MString]: Cannot change initonly field outside its .ctor. { Offset = 0x10 }[MString]: Return type is ByRef, TypedReference, ArgHandle, or ArgIterator. { Offset = 0x1a }[MObject]: Cannot change initonly field outside its .ctor. { Offset = 0x10 }[MObject]: Return type is ByRef, TypedReference, ArgHandle, or ArgIterator. { Offset = 0x1a }[MC]: Cannot change initonly field outside its .ctor. { Offset = 0x10 }[MC]: Unexpected type on the stack. { Offset = 0x15, Found = ref 'object[]', Expected = ref '[02d07b78-95a0-409e-bff4-1cbc39fcff23]C[]' }[MC]: Return type is ByRef, TypedReference, ArgHandle, or ArgIterator. { Offset = 0x1a }

[ : C::MString][mdToken=0x6000006][offset 0x00000010] Cannot change initonly field outside its .ctor.

This looks like a real problem. It looks like we shouldn't be marking the fieldreadonly.

[ : C::MC][mdToken=0x6000008][offset 0x00000015][found ref array mdarray 'System.Object[]'][expected ref array mdarray 'C[]'] Unexpected array type on the stack.

This is concerning as well.

1. At the very least, it looks like we should pay attention toIsPeVerifyCompatEnabled after all (and test that as well).
2. Are we confident that we are not going to run in trouble trying to consume the span. Could that trigger some hard runtime type check failure in some scenarios, For example, when we try to store an element reference in aref readonly local, etc. ?

It looks like we should verify expected verification failure reasons in the tests targeting scenarios affected by this PR.

Made the field writable and removed arbitrary reference type scenario, so there's no PEVerify regression (so didn't add any tests targetingIsPeVerifyCompatEnabled case).
I added some usage of the ROS with aref readonly local.

MString2

Why the second versions are necessary? Should we verify IL for them too? #Closed

{{type}}

Looking at this scenario again, I do think there's a problem. The type of thePrivateImplementationDetails field should beobject[] for all references. But it seems order-dependent. I'll take a look #Resolved

📝 Note this affects the caching field for blob wrappers too (existing logic, seetryEmitAsCachedArrayFromBlob which involves an assignment to the caching field, so it cannot be read only). But somehow those writes were not flagged by ILVerify warnings (seeMultipleArrays_InPlaceAndUsed).

Verification.Fails with { ILVerifyMessage = ilVerifyMessage, PEVerifyMessage = peVerifyMessage });

Are we expecting verify to fail here?

return false;

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

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