Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace all non-test uses of new T[0] with Array.Empty<T> #2344

Merged
merged 1 commit into from Jul 16, 2015

Conversation

@jamesqo
Copy link
Contributor

@jamesqo jamesqo commented Jul 13, 2015

I ran a little shell command to find and replace most of the occurrences of new T[0] in the repo:

cd corefx
grep -rl "new [^ (]*\[0\]" . | grep -vi test | xargs start notepad++

There were about 18 matches, with an additional match I spotted in Queue<T>.ToArray.

Additional notes:

  • Removed caching of EmptySequence in XElement and XAttribute; this is already done by Array.Empty.
  • The build should fail at first because I forgot to add using statements to the classes I changed. Will fix once the compiler tells where it can't recognize Array.
@@ -513,7 +513,7 @@ public T[] ToArray()
{
// Short path if the bag is empty
if (_headList == null)
return new T[0];
return Array.Empty<T>();

This comment has been minimized.

@stephentoub

stephentoub Jul 13, 2015
Member

I explicitly avoided changing this one in the past (and others in ToArray methods) because the docs for it are explicit about it creating a "new" array.

This comment has been minimized.

@JonHanna

JonHanna Jul 13, 2015
Collaborator

@stephentoub if that's considered important, should we have a test for Assert.NotSame(new ConcurrentBag<string>().ToArray(), ConcurrentBag<string>().ToArray())? And commented as such too. This change seems like a reasonable optimisation. If it's rejected here, someone is going to come along and do it again later.

This comment has been minimized.

@stephentoub

stephentoub Jul 13, 2015
Member

@hackcraft, yeah, I think either we should take this change, or add such a test. Same for other collections.

This comment has been minimized.

@JonHanna

JonHanna Jul 13, 2015
Collaborator

On the flip side. If this change is taken then a test of Assert.Same(new ConcurrentBag<string>().ToArray(), ConcurrentBag<string>().ToArray()) would mean any change back would at least have to justify why they removed they performance gain by justifying removing the now-breaking test.

This comment has been minimized.

@stephentoub

stephentoub Jul 13, 2015
Member

Yes, there are already some of both kinds in the repo, for various things (including some that verify this for ToArray, e.g. there's a Queue.ToArray test that verifies not-sameness and which fails with this PR.)

This comment has been minimized.

@jamesqo

jamesqo Jul 13, 2015
Author Contributor

@stephentoub To stay consistent, then, I'll remove the changes to Queue as well.


Edit: I just noticed that the code for non-generic Queue looks like this (I haven't modified it):

public virtual Object[] ToArray()
{
    if (_size == 0)
        return Array.Empty<Object>();

    Object[] arr = new Object[_size];

    // ...

I'm wondering whether I should just keep the changes to stay in line with the non-generic version, then.

@@ -382,7 +382,7 @@ public override DynamicMetaObject BindDeleteIndex(DeleteIndexBinder binder, Dyna

private delegate DynamicMetaObject Fallback(DynamicMetaObject errorSuggestion);

private readonly static Expression[] s_noArgs = new Expression[0]; // used in reference comparison, requires unique object identity
private readonly static Expression[] s_noArgs = Array.Empty<Expression>(); // used in reference comparison, requires unique object identity

This comment has been minimized.

@stephentoub

stephentoub Jul 13, 2015
Member

See the comment; this shouldn't be changed.

This comment has been minimized.

@jamesqo

jamesqo Jul 13, 2015
Author Contributor

Thanks for pointing that out; this has been fixed in the next commit.

@@ -5,6 +5,6 @@ namespace System.Reflection.Internal
{
internal static class EmptyArray<T>
{
internal static readonly T[] Instance = new T[0];
internal static readonly T[] Instance = Array.Empty<T>();

This comment has been minimized.

@stephentoub

stephentoub Jul 13, 2015
Member

Does this work? I thought this assembly built against an earlier contract that lacks Empty.

@@ -31,7 +31,7 @@ internal sealed class ImmutableList<T> : IEnumerable<T>
public static ImmutableList<T> Empty { get { return _empty; } }

/// <summary>Initializes the immutable list to be empty.</summary>
private ImmutableList() : this(new T[0]) { }
private ImmutableList() : this(Array.Empty<T>()) { }

This comment has been minimized.

@stephentoub

stephentoub Jul 13, 2015
Member

Ditto; does this compile?

@stephentoub
Copy link
Member

@stephentoub stephentoub commented Jul 13, 2015

@khdang, pelase take a look at the XmlSerializer changes to confirm whether they're acceptable or not.

@bartonjs, please take a look at the crypto changes to confirm whether they're acceptable or whether the behavioral impact (object identity of the returned object) is problematic.

@VSadov, please take a look at the dynamic, LINQ, etc. stuff.

@stephentoub
Copy link
Member

@stephentoub stephentoub commented Jul 13, 2015

Will fix once the compiler tells where it can't recognize Array

I'm curious, does this mean you're not building locally before submitting?

@@ -37,7 +37,7 @@ public partial struct ImmutableArray<T> : IReadOnlyList<T>, IList<T>, IEquatable
/// <summary>
/// An empty (initialized) instance of <see cref="ImmutableArray{T}"/>.
/// </summary>
public static readonly ImmutableArray<T> Empty = new ImmutableArray<T>(new T[0]);
public static readonly ImmutableArray<T> Empty = new ImmutableArray<T>(Array.Empty<T>());

This comment has been minimized.

@stephentoub

stephentoub Jul 13, 2015
Member

Does this compile? I thought we built this library against a contract that lacks Empty. And since it's being cached into a singleton in a static, caching it separately isn't really warranted.

@bartonjs
Copy link
Member

@bartonjs bartonjs commented Jul 13, 2015

I can't imagine anyone needing reference uniqueness from the affected crypto code, so I'm good with the crypto part of the change.

@stephentoub
Copy link
Member

@stephentoub stephentoub commented Jul 13, 2015

Great; thanks, Jeremy.

@khdang
Copy link
Member

@khdang khdang commented Jul 13, 2015

The changes in XmlSerializer look good to me.

@@ -3159,7 +3159,7 @@ internal Buffer(IEnumerable<TElement> source, bool queryInterfaces = true)

internal TElement[] ToArray()
{
if (count == 0) return new TElement[0];
if (count == 0) return Array.Empty<TElement>();

This comment has been minimized.

@VSadov

VSadov Jul 13, 2015
Member

I think we considered changing this, but were not sure if it is enough benefit compared to potential for breaking code relying on distinct array instances.
User might already have some allocations when hitting this path so one less empty array might not make a lot of improvement in typical code. The documentation says "An IEnumerable to create an array from." If taken literally it seems to say that array is always created.
I wonder if there is a way to estimate if there is actual usage of this with expectation that array is created. Perhaps it can be done by scanning some publicly available code or something...

I think we should not change this just yet, but may worth adding a comment that it is being considered.

This comment has been minimized.

@jamesqo

jamesqo Jul 14, 2015
Author Contributor

Great idea; I agree, many of the places where this is used (Iterator.ToArray) seem to imply that they are returning a different array. I'll revert and add a comment on the side.

@JonHanna
Copy link
Collaborator

@JonHanna JonHanna commented Jul 13, 2015

I can see a reverse danger to this; someone could come to expect that the result of some of these operations are all the same object and then be surprised down the line (or perhaps upon deserialisation if they hadn't previously considered this).

I don't see that being a big issue (I can remember people being confused by the fact that ReferenceEquals(string.Empty, "") changed between framework versions, but not stymied by it). But better to consider it and rule it out as a problem than not consider it.

@VSadov
Copy link
Member

@VSadov VSadov commented Jul 13, 2015

LINQ/Dynamic look ok except for ToArray() and where Stephen has already commented about ref comparisons.

The ToArray ones share the same concern that benefits might not be that great while user might expect new instances and therefore we were reluctant to change these. I wonder what would be the right forum to discuss the ToArray issue in general and to conclude one way or another, possibly in the context of the whole framework so it would be consistent.

@jamesqo
Copy link
Contributor Author

@jamesqo jamesqo commented Jul 14, 2015

@hackcraft Hm. I'm assuming you mean scenarios like this:

if (queue.ToArray() == Array.Empty<T>())
{
    // ...

That might happen if someone did stumbled on this by chance and assumed that it would always be true, but in general I think most people would go for checking the length of the array or some variant.

That said, I am undoing the changes I made to Queue for now.

@krwq
Copy link
Member

@krwq krwq commented Jul 14, 2015

Not sure if I remember correctly: I believe we used to have Array.Empty internally some time ago for a while and then we reverted the changes as Array.Empty is not available on some earlier versions of full .NET and we wanted to be compatible with as many versions as possible.

We should find out who did the internal revert to figure out if we should re-do it.

@stephentoub
Copy link
Member

@stephentoub stephentoub commented Jul 14, 2015

@krwq, can you do that? Thanks.

return arr;

return arr; // consider replacing with Array.Empty<T>() to be consistent with non-generic Queue

This comment has been minimized.

@stephentoub

stephentoub Jul 14, 2015
Member

Hmm, it looks like I'm being inconsistent with myself. I'd previously changed Queue.ToArray to return Array.Empty<object>, doing so for the other NonGeneric collections as well, but I explicitly avoided doing so for the generic ones. I don't remember now what my rationale was for thinking it was ok to convert one set but not the other. Seems like (separate from this PR), we should reevaluate that; I'm tempted to roll back those changes to the non-generic collections, as they exist primarily for backwards compatibility, and any code more sensitive to performance should be using the generic ones, especially with value types.

@@ -192,7 +192,7 @@ internal Buffer(IEnumerable<TElement> source)

internal TElement[] ToArray()
{
if (count == 0) return new TElement[0];
if (count == 0) return Array.Empty<TElement>();

This comment has been minimized.

@stephentoub

stephentoub Jul 14, 2015
Member

Please revert this.

@@ -513,7 +513,7 @@ public T[] ToArray()
{
// Short path if the bag is empty
if (_headList == null)
return new T[0];
return new T[0];

This comment has been minimized.

@stephentoub

stephentoub Jul 14, 2015
Member

Nit: unnecessary whitespace changes

@krwq
Copy link
Member

@krwq krwq commented Jul 14, 2015

XML parth looks good to me 👍 Also please disregard my previous comment.

@stephentoub
Copy link
Member

@stephentoub stephentoub commented Jul 15, 2015

Thanks, @James-Ko. Looks good. Can you please squash this?

@jamesqo jamesqo force-pushed the jamesqo:patch-3 branch from 18ca207 to 66e9b98 Jul 15, 2015
@jamesqo
Copy link
Contributor Author

@jamesqo jamesqo commented Jul 15, 2015

I have no idea why, but it seems that the build is suddenly failing after I squashed the commits. According to Jenkins, it seems to be a problem with EtwTests on Linux, which I have not heard of nor touched. I'm going to try a soft reset to the last commit and re-push.

@jamesqo jamesqo force-pushed the jamesqo:patch-3 branch from 66e9b98 to 9b1ed39 Jul 15, 2015
@stephentoub
Copy link
Member

@stephentoub stephentoub commented Jul 15, 2015

@James-Ko, I don't believe this is related to your PR. Others are failing with the same issue. I'm suspecting that there's either something wrong with the infrastructure or that something's changed with the CoreCLR we're pulling in. @mmitche, @ellismg, ideas?

@mmitche
Copy link
Member

@mmitche mmitche commented Jul 16, 2015

This looks like maybe a change got merged in that accidentally enabled etw tests on Linux? It doesn't makes sense to run them there anyway.

@stephentoub
Copy link
Member

@stephentoub stephentoub commented Jul 16, 2015

it doesn't makes sense to run them there anyway.

It does, actually. These have been running on Linux for a long time. EventSource works on Linux, it just isn't hooked up to ETW. EventListeners still receive events, and these tests make sure those events are raised.

@ellismg
Copy link
Contributor

@ellismg ellismg commented Jul 16, 2015

Still trying to dig into this but I have to run for a bit so it won't happen until later tonight.

@stephentoub
Copy link
Member

@stephentoub stephentoub commented Jul 16, 2015

@ellismg, I'm looking as well. I have a hunch. @mmitche, you recently brought new build tools into CoreCLR, right? Is it possible that in doing so we switched something related to the $(OS) / $(OSGroup) properties and what defines get set as a result? It looks like some Windows-specific code that's protected by FEATURE_MANAGED_ETW is getting output in the Linux mscorlib now.

@stephentoub
Copy link
Member

@stephentoub stephentoub commented Jul 16, 2015

I think this fixes it: dotnet/coreclr#1246

@stephentoub
Copy link
Member

@stephentoub stephentoub commented Jul 16, 2015

LGTM

@Maxwe11
Copy link
Contributor

@Maxwe11 Maxwe11 commented Jul 16, 2015

@dotnet-bot test this please

stephentoub added a commit that referenced this pull request Jul 16, 2015
Replace all non-test uses of new T[0] with Array.Empty<T>
@stephentoub stephentoub merged commit bfe5c6a into dotnet:master Jul 16, 2015
1 check passed
1 check passed
default Build finished. No test results found.
Details
@jamesqo jamesqo deleted the jamesqo:patch-3 branch Jul 16, 2015
@guiomie

This comment has been minimized.

Copy link

@guiomie guiomie commented on 9b1ed39 Jul 28, 2015

Hi, I was just wondering why should it be Array.Empty as opposed to new T[0] ?

This comment has been minimized.

Copy link
Member

@akoeplinger akoeplinger replied Jul 28, 2015

@guiomie it saves an extra object allocation since the instance returned by Array.Empty is cached.

This comment has been minimized.

Copy link

@guiomie guiomie replied Jul 28, 2015

Ahh... Thanks for answer!

@karelz karelz modified the milestone: 1.0.0-rtm Dec 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.