Remove delegate allocation #2312

dVakulen · 2016-10-17T16:49:51Z

On each Catalog local lookup the delegate was allocated.

dVakulen · 2016-10-17T16:55:15Z

src/OrleansRuntime/GrainDirectory/LocalGrainDirectory.cs

@@ -471,12 +479,17 @@ public SiloAddress CalculateTargetSilo(GrainId grainId, bool excludeThisSiloIfSt
                    return excludeThisSiloIfStopping && !Running ? null : MyAddress;
                }

-                // excludeMySelf from being a TargetSilo if we're not running and the excludeThisSIloIfStopping flag is true. see the comment in the Stop method.
-                bool excludeMySelf = !Running && excludeThisSiloIfStopping; 
-
                // need to implement a binary search, but for now simply traverse the list of silos sorted by their hashes


The binary search will provide benefit only on relatively large deployments (with silos count > 75). Does it makes sense to implement this?

jdom · 2016-10-17T19:19:57Z

src/OrleansRuntime/GrainDirectory/LocalGrainDirectory.cs

        public LocalGrainDirectory(Silo silo)
        {
            log = LogManager.GetLogger("Orleans.GrainDirectory.LocalGrainDirectory");

            MyAddress = silo.LocalMessageCenter.MyAddress;
+            isSiloNextInTheRing = (siloAddr, hash, excludeMySelf) => (siloAddr.GetConsistentHashCode() <= hash) &&
+                                                                     (!excludeMySelf || !siloAddr.Equals(MyAddress));


Since this is not using any closure, what is the purpose of having a delegate field instead of a (more readable) proper method?

It's still capturing MyAddress.

I'm confused. Is it really capturing the address? I would expect this code to capture this and then accessing the MyAddress field, which is no different to calling a proper instance method.

You're right, @jdom.

~~I would suggest creating a private readonly Predicate<SiloAddress> field which captures this once and passing that to the call here. That way you have no per-call allocations.~~

EDIT: We cannot simply capture the logic in a field since it requires excludeThisSiloIfStopping which is a parameter to the calling function.

Inlining the logic of List<T>.FindLast isn't so bad IMO

There's 2 things here. Since this is inlining FindLast, then there is not even a need to remove the delegate allocation, as there is no longer a closure being passed to another higher order function.
But even if we decided to continue using FindLast and remove the inlining, what I don't understand is the need for a function field that captures this, since that is exactly what an instance method is. So just define the function as a method body instead of in a field that is later assigned.

@jdom I agree with the first point.
For the second, when you pass an instance method as a delegate, like

membershipRingList.FindLast(this.SomePredicateMethod)

you are allocating a new Predicate<T> every time.

To double check, here's what I see in LINQPad:

void Main() { var list = new List<int>(); list.FindLast(this.GreaterThan2); } bool GreaterThan2(int value) => value > 2;

produce this IL:

IL_0000: nop IL_0001: newobj System.Collections.Generic.List<System.Int32>..ctor IL_0006: stloc.0 // list IL_0007: ldloc.0 // list IL_0008: ldarg.0 IL_0009: ldftn UserQuery.GreaterThan2 IL_000F: newobj System.Predicate<System.Int32>..ctor IL_0014: callvirt System.Collections.Generic.List<System.Int32>.FindLast IL_0019: pop IL_001A: ret GreaterThan2: IL_0000: ldarg.1 IL_0001: ldc.i4.2 IL_0002: cgt IL_0004: ret

Indeed; fixed.

jason-bragg · 2016-10-18T22:50:31Z

src/OrleansRuntime/GrainDirectory/LocalGrainDirectory.cs

@@ -1088,6 +1098,11 @@ internal IRemoteGrainDirectory GetDirectoryReference(SiloAddress silo)
            return InsideRuntimeClient.Current.InternalGrainFactory.GetSystemTarget<IRemoteGrainDirectory>(Constants.DirectoryServiceId, silo);
        }

+        private bool IsSiloNextInTheRing(SiloAddress siloAddr, int hash, bool excludeMySelf)


This logic is same as other logic, except with <= rather than >=.
Please consider using one static function with both hashes in arguments, with the hash order changed, rather then two functions with the comparison reversed.

I think that the resulting logic for this one-liner function will be more complex than now. If this is desired - I can change it.

jason-bragg · 2016-10-18T23:00:18Z

In general, I've no objection to this change, but I don't really see much value in it ether. Unless I'm misunderstanding something, this call path is not nearly hot enough to merit this sort of micro-optimization. I could well be wrong though.

dVakulen · 2016-10-19T18:57:03Z

@jason-bragg This allocation happens on almost every request message send. On the other hand GC induced by Orleans doesn't takes much CPU, so I don't expect any effect from one or two removed allocations. But in perspective the number of allocs on hot paths could be reduced to noticeable extent.

jason-bragg · 2016-10-19T20:13:22Z

@dVakulen, Agreed. I don't expect perf gain from this, but removing allocations and link expressions (notoriously slow) from the hot path can't hurt.

LGTM

sergeybykov · 2016-10-20T23:24:32Z

Thank you, @dVakulen for pushing the throughput numbers higher with nearly every PR!

dVakulen · 2016-10-21T23:36:16Z

@sergeybykov Just curious - was there any effect after merge of the #2275?

ReubenBond · 2016-10-22T04:11:51Z

@dVakulen there was an approx 6% increase by my reckoning. It's hard to be sure that it was caused by #2275 specifically, particularly because #2295 went in at about the same time.

* Remove closure allocation * Address feedback * Remove closure allocation in consistent ring provider

Remove closure allocation

b951e62

dnfclas added the cla-already-signed label Oct 17, 2016

dVakulen commented Oct 17, 2016

View reviewed changes

jdom reviewed Oct 17, 2016

View reviewed changes

dVakulen added 2 commits October 18, 2016 20:07

Address feedback

07291d6

Remove closure allocation in consistent ring provider

e789be3

jason-bragg reviewed Oct 18, 2016

View reviewed changes

jason-bragg merged commit 04a3b19 into dotnet:master Oct 20, 2016

dVakulen deleted the allocs-removal-2 branch October 20, 2016 08:44

sergeybykov pushed a commit to sergeybykov/orleans that referenced this pull request Oct 27, 2016

Remove delegate allocation (dotnet#2312)

9994ae7

* Remove closure allocation * Address feedback * Remove closure allocation in consistent ring provider

github-actions bot locked and limited conversation to collaborators Dec 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove delegate allocation #2312

Remove delegate allocation #2312

dVakulen commented Oct 17, 2016

dVakulen Oct 17, 2016

jdom Oct 17, 2016

dVakulen Oct 17, 2016

jdom Oct 17, 2016 •

edited

ReubenBond Oct 18, 2016 •

edited

jdom Oct 18, 2016

ReubenBond Oct 18, 2016

ReubenBond Oct 18, 2016

dVakulen Oct 18, 2016 •

edited

jason-bragg Oct 18, 2016

dVakulen Oct 19, 2016

jason-bragg commented Oct 18, 2016

dVakulen commented Oct 19, 2016

jason-bragg commented Oct 19, 2016

sergeybykov commented Oct 20, 2016

dVakulen commented Oct 21, 2016 •

edited

ReubenBond commented Oct 22, 2016

Remove delegate allocation #2312

Remove delegate allocation #2312

Conversation

dVakulen commented Oct 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdom Oct 17, 2016 • edited

Choose a reason for hiding this comment

ReubenBond Oct 18, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dVakulen Oct 18, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jason-bragg commented Oct 18, 2016

dVakulen commented Oct 19, 2016

jason-bragg commented Oct 19, 2016

sergeybykov commented Oct 20, 2016

dVakulen commented Oct 21, 2016 • edited

ReubenBond commented Oct 22, 2016

jdom Oct 17, 2016 •

edited

ReubenBond Oct 18, 2016 •

edited

dVakulen Oct 18, 2016 •

edited

dVakulen commented Oct 21, 2016 •

edited