Optimise multiple Append and Prepend calls. by JonHanna · Pull Request #6864 · dotnet/corefx

JonHanna · 2016-03-14T13:05:26Z

Use a more compact form for multiple calls to Append and Prepend, that shares a collection of appended and/or prepended elements between instances.

Considering @stephentoub's comment at #5947 (comment)

I'm imaging at some point we might want to create some optimized paths for when Append, Prepend, and Concat are used repetitively.

This attempts to optimise multiple append and prepends, but not concatenation.

It would need some more tests at the very least before its ready for merging, but I'd appreciate any input on the approach taken prior to that.

svick · 2016-03-14T15:39:03Z

+
+        public bool Append(T item, int number)
+        {
+            if (number != _count | number >= MaxLength)


Is using | instead of || here intentional micro-optimization? If that's the case, it seems to me it's a very unusual pattern in corefx, so maybe it should be explicitly pointed out by a comment?

Its an as-a-rule rather than a measured micro-opt. As in "as a rule" just comparing integers will be cheaper than the cost of branching to avoid doing so, but you have to measure to know for sure.

Ditto here with my other line note; shouldn't it be > MaxLength?

JonHanna · 2016-03-31T08:45:09Z

Test this please

JonHanna · 2016-03-31T12:29:17Z

Now why did dotnet-bot do that? (Was closing and re-opening seen as Spammy perhaps)

JonHanna · 2016-03-31T13:37:05Z

Test Innerloop Windows_NT Release Build and Test

(System.Net.Http.Functional.Tests.HttpClientHandlerTest.GetAsync_UnsupportedSSLVersion_Throws
Timeout caused cancelling? http://dotnet-ci.cloudapp.net/job/dotnet_corefx/job/windows_nt_release_prtest/4306/testReport/junit/System.Net.Http.Functional.Tests/HttpClientHandlerTest/GetAsync_UnsupportedSSLVersion_Throws_name___SSLv3___url___https___www_ssllabs_com_10300___/)

stephentoub · 2016-04-01T12:08:22Z

+            public abstract int GetCount(bool onlyIfCheap);
+        }
+
+        private sealed class Append1Iterator<TSource> : AppendPrependIterator<TSource>


Rather than duplicating the type hierarchy for append vs prepend, my preference would be to carry a bool around and switch on it in the implementation. It's not much more extra space required, the branch should be negligible compared to all of the interface calls being made, and the amount of code required and associated complexity should shrink non-trivially.

So have a single AppendPrepend with two collections?

Ah, right, I hadn't gotten that far. But, yeah, rather than having a single collection field, have two collection fields, one of which will likely be null. We increase the size of the object by a single reference field, but cut back on complexity and duplication. I think I'm suggesting just get rid of your AppendN and PrependN iterators, in favor of just using AppendPrependNIterator.

And also for this Append1 case, I'm suggesting having a single AppendPrepend1Iterator with an extra bool field rather than having two different Append1 and Prepend1 cases, and use that bool to determine whether the single _element is meant to be appended or prepended.

That would cut down the size, but unless it's all cut down to one class we're either left with still having the interface call, or adding to the number of type checks in the query method.

Of course the case with two collections can handle the Append1 and Prepend1 case too, and maybe the extra weight of that is worth the extra simplicity.

I'm not following. Why would it change the number of type checks or interface calls? It would add branches, yes, because you'd need to check whether the bool was false or true to determine whether to append or prepend the element... why is it more than that? I'm suggesting there just be three types, as there is in the concat case: AppendPrependIterator, AppendPrepend1Iterator, and AppendPrependNIterator. The first is the abstract base, the second contains a single element and a bool to say whether it's an append or prepend, and the third contains two fields for two collections, one for each of append/prepend, and they may be null if there's none of that case.

Maybe it's me that's not following. I read you as suggesting an AppendPrependNIterator and an AppendPrepend1Iterator as separate classes, in which case to Append to one of those we need to either keep the abstract model in use here (so we don't lose the virtual call) or check for both of those types rather than just checking for one.

I read you as suggesting an AppendPrependNIterator and an AppendPrepend1Iterator as separate classes

Yes.

in which case to Append to one of those we need to either keep the abstract model in use here (so we don't lose the virtual call)

Yes. I wasn't suggesting that my approach would be cheaper, but it also shouldn't add more virtual calls than what you already have, and it effectively cuts the amount of code in half.

Ah. I see what I misread now.

…negligible compared to all of the interface calls being made

I misread that as suggesting that said calls would be compensatorily reduced.

JonHanna · 2016-04-09T13:40:04Z

Test Innerloop Windows_NT Debug Build and Test
Test Innerloop Windows_NT Release Build and Test
(Hung process, and SSL tests at http://dotnet-ci.cloudapp.net/job/dotnet_corefx/job/windows_nt_release_prtest/4672/ which might be a network fluke, will open an issue if it repeats).

JonHanna · 2016-04-12T08:56:33Z

+            public override List<TSource> ToList()
+            {
+                int count = GetCount(onlyIfCheap: true);
+                List<TSource> list = count == -1 ? new List<TSource>(Math.Max(_appCount + _preCount, 4)) : new List<TSource>(count);


@stephentoub I dropped the new List<TSource>(4) from the other case, but I've left Math.Max(_appCount + _preCount, 4) as it seems to me that "the sum of the sizes of the two parts we know are adding in for sure, unless that's pretty low in which case a wee bit more" is a reasonable heuristic for the starting size, with 4 as a reasonable value for "wee bit more".

hughbe · 2016-04-13T21:08:32Z

+            {
+                switch (_state)
+                {
+                    case 1:


minor question: is it worth having these integers as private named constants for readability?

It's a linear progression through numbered states. I think the most readable labels are simply 1, 2, 3 etc.

jamesqo · 2016-04-17T00:52:43Z

+            {
+
+                T[] newStore = new T[number << 1];
+                for (int i = 0; i != store.Length; ++i)


Does using != as opposed to < here result in a performance improvement?

I very much doubt it. Preference from a long time ago when more often using C++ iterator objects where != might be defined for the type but < might not. (And longer ago again it might perhaps have made for a slight but measurable performance difference on old CPUs).

jamesqo · 2016-04-17T01:12:33Z

+            if (store.Length == number)
+            {
+
+                T[] newStore = new T[number << 1];


Ditto here; leave a comment explaining it, or just put * 2 if the JIT already does this as an optimization.

jamesqo · 2016-04-17T15:24:06Z

+            _count = 1;
+        }
+
+        public SharedCollection(T first, T second)


Nit: Maybe it would be more readable if you had a params T[] items constructor here instead?

Maybe it would be more readable if you had a params T[] items constructor here instead?

That would allocate an array at the call site, and while there's an array being allocated here internally, it wouldn't be clear to this code whether the array being passed in was having ownership transferred, so it would likely need to make a copy to protect itself.

We could assign it straight to _store so it wouldn't add extra cost, but this makes it clear that it's only ever called with 1 or 2 elements.

VSadov · 2016-04-29T00:05:39Z

@dotnet-bot test Innerloop Windows_NT Debug Build and Test please
@dotnet-bot test Innerloop CentOS7.1 Release Build and Test and Test please

VSadov · 2016-04-29T00:47:49Z

I have concerns about attaching appended elements to iterator through which they are not reachable. It seems it can lead to unexpected retention.

// here a,b,c become GC-reachable from x, but not iterator-reachable
x.Append(a).Append(b).Append(c);

I think prepend case could be handled by sharing a single-linked list. Then whatever is reachable via iteration is also what is GC-reachable.

Not sure if similar solution is possible for Append case.

stephentoub · 2016-04-29T12:24:19Z

I have concerns about attaching appended elements to iterator through which they are not reachable

Excellent point. I agree.

JonHanna · 2016-04-29T17:37:50Z

Yes. That's the reason for the limit on the size of the shared collection, but perhaps a linked-list would be better.

stephentoub · 2016-04-29T17:39:09Z

That's the reason for the limit on the size of the shared collection

It's not necessarily the number that matters. One of the appended items could be a giant instance.

JonHanna · 2016-04-29T17:58:36Z

That is true. A linked-list it shall be then.

JonHanna · 2016-04-29T18:09:34Z

@svick there's a similar thing done with Concat. Maybe it will turn out to not be worth it upon trying, but I shall take a look.

svick · 2016-04-29T18:12:40Z

@JonHanna Sorry, I deleted my comment. I forgot that the current implementation means iterating is quadratic, so a linked list would probably still be worth it.

JonHanna · 2016-05-02T03:15:20Z

Not sure if similar solution is possible for Append case.

I wasn't sure either when I first approached this, and so had rejected a linked-list approach, but if we keep track of the size of the list (which is worth doing anyway, for other optimisations) we can store the appended items in an array as they are being iterated, and so gather them up in a single O(n) sweep before an O(n) iteration through them, rather than having the quadratic behaviour.

VSadov · 2016-05-15T21:08:46Z

                _threadId = Environment.CurrentManagedThreadId;
            }

+            protected bool RunningOnCreatingThread => _threadId == Environment.CurrentManagedThreadId;


Is this used?

I don't think it is after the last round of changes, so it can be reverted. I'm not having a lot of time for CoreFX right now, but I'll try to tidy this up ASAP.

VSadov · 2016-05-15T21:16:30Z

Looks like a good compromise. Append case needs an allocation of an array, but I do not see a lot of alternatives. Without that we have the n^2 problem when iterating.

RunningOnCreatingThread seems to be a method not used anywhere.

Otherwise LGTM.

Use a more compact form for multiple calls to Append and Prepend, that shares a collection of appended and/or prepended elements between instances.

JonHanna · 2016-05-26T14:06:49Z

That RunningOnCreatingThread cruft has been removed.

…_append_prepend_calls Optimise multiple Append and Prepend calls. Commit migrated from dotnet/corefx@2b60a4b

dnfclas added the cla-already-signed label Mar 14, 2016

svick reviewed Mar 14, 2016
View reviewed changes

stephentoub assigned VSadov Mar 16, 2016

JonHanna force-pushed the optimise_multiple_append_prepend_calls branch from 83d3208 to a49cdf9 Compare March 30, 2016 16:02

JonHanna changed the title ~~Optimise multiple Append and Prepend calls. [WIP]~~ Optimise multiple Append and Prepend calls. Mar 30, 2016

JonHanna closed this Mar 31, 2016

JonHanna reopened this Mar 31, 2016

dotnet-bot closed this Mar 31, 2016

dnfclas added the cla-already-signed label Mar 31, 2016

stephentoub reopened this Mar 31, 2016

dnfclas added the cla-already-signed label Mar 31, 2016

stephentoub added the 2 - In Progress label Mar 31, 2016

stephentoub reviewed Apr 1, 2016
View reviewed changes

JonHanna force-pushed the optimise_multiple_append_prepend_calls branch from a49cdf9 to cf498a9 Compare April 9, 2016 11:20

JonHanna reviewed Apr 12, 2016
View reviewed changes

stephentoub added the netfx-port-consider label Apr 13, 2016

hughbe reviewed Apr 13, 2016
View reviewed changes

AlexGhiondea added netfx-port-consider and removed netfx-port-consider labels Apr 13, 2016

jamesqo reviewed Apr 17, 2016
View reviewed changes

JonHanna force-pushed the optimise_multiple_append_prepend_calls branch from cf498a9 to c7aff48 Compare April 17, 2016 13:46

jamesqo reviewed Apr 17, 2016
View reviewed changes

JonHanna force-pushed the optimise_multiple_append_prepend_calls branch from c7aff48 to e128f2d Compare May 2, 2016 03:09

VSadov reviewed May 15, 2016
View reviewed changes

Optimise multiple Append and Prepend calls.

9857e37

Use a more compact form for multiple calls to Append and Prepend, that shares a collection of appended and/or prepended elements between instances.

JonHanna force-pushed the optimise_multiple_append_prepend_calls branch from e128f2d to 9857e37 Compare May 26, 2016 14:05

VSadov merged commit 2b60a4b into dotnet:master May 26, 2016

joshfree removed the 2 - In Progress label May 26, 2016

JonHanna deleted the optimise_multiple_append_prepend_calls branch May 27, 2016 00:30

karelz modified the milestone: 1.1.0 Dec 3, 2016

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/corefx#6864 from JonHanna/optimise_multiple…

3278485

…_append_prepend_calls Optimise multiple Append and Prepend calls. Commit migrated from dotnet/corefx@2b60a4b

Conversation

JonHanna commented Mar 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonHanna commented Mar 31, 2016

Uh oh!

JonHanna commented Mar 31, 2016

Uh oh!

JonHanna commented Mar 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonHanna commented Apr 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonHanna Apr 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesqo Apr 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub Apr 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VSadov commented Apr 29, 2016

Uh oh!

VSadov commented Apr 29, 2016

Uh oh!

stephentoub commented Apr 29, 2016

Uh oh!

JonHanna commented Apr 29, 2016

Uh oh!

stephentoub commented Apr 29, 2016

Uh oh!

JonHanna commented Apr 29, 2016

Uh oh!

JonHanna commented Apr 29, 2016

Uh oh!

svick commented Apr 29, 2016

Uh oh!

JonHanna commented May 2, 2016

Uh oh!

Choose a reason for hiding this comment

JonHanna Apr 17, 2016 •

edited

Loading

jamesqo Apr 17, 2016 •

edited

Loading

stephentoub Apr 17, 2016 •

edited

Loading