Simple devirtualization #9230

AndyAyersMS · 2017-01-31T22:28:58Z

Devirtualize calls where the this object type at a call site
is a subtype of the type described at the call site. Currently learns
types from local and arg references and a subset of other operators.

Will devirtualize if either the class or method is final (sealed in C#),
or if the type is known exactly (eg from a newobj).

Devirtualization is run twice, once during importation, and again in a
limited way after inlinining. Calls devirtualized during importation are
are subsequently eligible for inlining. Calls devirtualized during inlining
currently cannot be inlined.

AndyAyersMS · 2017-01-31T22:35:24Z

Marking no merge for now since this is work in progress.

This impacts ~267 methods in System.Private.CoreLib,. 453 sites are devirtualized. No hits in other assemblys in jit-diff, presumably because of R2R restrictions, though I have yet to dig in deeper. Simple jitted test cases show expected results.

Particular areas for review: implementation of the jit interface call and the fragile way that call related info is updated.

@jkotas, @JosephTremoulet PTAL
cc @dotnet/jit-contrib

Related issues: #1166, #8772

jkotas · 2017-02-01T01:56:23Z

src/jit/importer.cpp

+
+            // If we have a virtual call, is the class of 'this' a subtype of
+            // the class of the method? If so perhaps we can devirtualize.
+            if ((call->gtFlags & GTF_CALL_VIRT_KIND_MASK) == GTF_CALL_VIRT_VTABLE)


You should also do this for CORINFO_VIRTUALCALL_STUB. It is used for interface calls under JIT and fragile NGen, and for all virtual calls under R2R. Thus, you should see some hits for R2R once you do that.

Have this sort of working, and yes I'm seeing it kick in for R2R code.

jkotas · 2017-02-01T01:58:40Z

src/jit/importer.cpp

+    }
+
+    // If the objClass is sealed (final), then we may be able to devirtualize.
+    const DWORD objClassAttribs = info.compCompHnd->getClassAttribs(objClass);


It should be cheaper (less VM/EE calls) and cleaner to let the VM do all these checks that are pre-reqs for devirtualization.

Agreed. I'll refactor to consolidate stuff on the VM side after I get a bit more functionality in place.

Are you still planning to do this?

Thinking of leaving things as they are now -- a few checks (eg the one for __Canon) were moved to the VM side, but a number remain on the jit side -- notably the attribute checks for final/sealed methods and classes.

Seemed logical to me to keep the policy decisions on the jit side and have the VM deal with correctness/servicing, which more or less dictates the current setup. Otherwise the jit has to pass in a bit more state (for instance, is the type known exactly) and get back more state (whether the target is known exactly).

Some of the jit-side checks will be removed in future work, eg interface call devirtualization.

jkotas · 2017-02-01T01:59:22Z

src/vm/jitinterface.cpp

+{
+    CORINFO_METHOD_HANDLE result;
+
+    JIT_TO_EE_TRANSITION();


You do not need this if you just forwarding to other JIT/EE interface method.

jkotas · 2017-02-01T02:00:06Z

src/ToolBox/superpmi/superpmi-shared/icorjitinfoimpl.h

+        CORINFO_CLASS_HANDLE implementingClass
+        );
+
+    CORINFO_METHOD_HANDLE resolveVirtualMethodExact(


It would be useful to have some comments what these methods do. (In particular, what's the difference between them.)

Sure. This is the shim layer header for SPMI, so I'll put something cursory here and something more detailed over in corinfo.h

jkotas · 2017-02-01T02:01:04Z

src/vm/jitinterface.cpp

+    // MethodDescs returned to JIT at runtime are always fully loaded. Verify that it is the case.
+    _ASSERTE(pMT->IsRestored() && pMT->IsFullyLoaded());
+
+    MethodDesc* pMD = pMT->GetMethodDescForSlot(slot);


For R2R, this will also need the version bubble check.

Are you saying the current approach (where the jit calls canInline on the resulting method handle) is insufficient?

canInline should work, but it is too pessimistic check and makes the optimization order dependent because of the noinlining hint is cached. I do not think you should be using canInline to gate it.

Ah, right. I have comments about the unfortunate impact of the noinline hint; this would be a good way to get past it.

Seems like I can maybe use getMethodBeingCompiled() for this? Something like:

// Allow devirtialization if jitting, or if prejitting and the // method being jitted and the devirtualized method are in the // same versioning bubble. if (!IsCompilingForNGen() || IsInSameVersionBubble(getMethodBeingCompiled(), pMD)) { result = (CORINFO_METHOD_HANDLE) pMD; }

Otherwise I need to pass in another method handle.

Yes, this should work.

briansull · 2017-02-02T20:43:23Z

src/vm/jitinterface.cpp

+    JIT_TO_EE_TRANSITION();
+
+    result = resolveVirtualMethodExact(methodHnd, derivedClass);
+


Why do we have two methods here?

I was matching jit interface changes from .Net Native. It looks like I may not need both versions.

On UTC the resolveVirtualMethod canonicalize's its result in most cases, but resolveVirtualMethodExact does not. This is used to handle the inlining of generic dictionary cell usage into the outer method. Given that RyuJit has a different model for handling shared generics that can't do the inlining anyways, this is probably ok. I would suggest only implementing the resolveVirtualMethod api, and not implementing resolveVirtualMethodExact. (This is simply done, as the implementation below is effectively the same as resolveVirtualMethod should be returning, but isn't quite the same as what resolveVirtualMethodExact would return in ProjectN.)

AndyAyersMS · 2017-02-03T03:07:51Z

Could not repro release crossgen failure at b334a7f; wonder if I need to rebase to hit it...?

AndyAyersMS · 2017-02-03T20:13:36Z

Failures in 56f702a are missing null checks; looks like at least some of the vcall stubs do implicit null checking, which will now need to be made explicit. Not sure if I should enable this for all the stub devirts or just a subset; for now I'll turn it on for all.

AndyAyersMS · 2017-02-04T01:19:12Z

Same tests failed again in 0d17370; serves me right for jumping to the conclusion that it was stub null checks at fault. Will debug locally this time...

JosephTremoulet · 2017-02-06T20:54:59Z

src/jit/importer.cpp

+        return;
+    }
+
+    // Do we know anything about the type of the 'this'?


Have you looked at what we do with (constrained) callvirt on value types? Not sure what shape those are in when they hit this code, but presumably they'd make the exact type easier to find...

Yes, we will (always?) see an upstream Box and can work out the type from there.

However I will probably leave this as future work since it would also be nice to optimize away the boxing at the same time.

JosephTremoulet · 2017-02-06T20:59:01Z

src/jit/importer.cpp

+    }
+
+    // Fetch attributes and do more vetting of the method. In general,
+    // if we can't inline, we won't devirtualize.


Not clear how the comment relates to the code. Did you mean to check for some inline-blocking attributes here?

This is a stale comment. I used to call canInline but have since moved the relevant checks to the VM side.

JosephTremoulet · 2017-02-06T20:59:21Z

src/jit/importer.cpp

+
+    if (objClassIsFinal || derivedMethodIsFinal)
+    {
+        JITDUMP("!!! Inlining ok, no restrictions, final; can devirtualize\n");


I'm confused again by the reference to inlining.

Also a stale comment.

JosephTremoulet · 2017-02-06T21:11:50Z

src/jit/importer.cpp

+        }
+#endif
+
+        // Need to update call info too. This is fragile


Do you think we could run these checks upstream, at the point where we're calling getCallInfo today? Then the callInfo wouldn't be mutating on us, and importCall could set these other flags on the call node as usual.

In fact, since you're already intending to push a number of these checks over to the VM, I'm wondering if we could end up with an interface where getCallInfo just takes an additional parameter which is the most-derived "thisPtr" type that the jit can determine...

This is one area I'm still experimenting with.

I don't like the idea of patching the call info or the various context handles, and it seems like the patching may get more complex when extending this to support shared generics.

But I am not sure we can anticipate all the info we might need early.

For instance, I'd like to call back into the devirtualization code during inlining or even later on during optimization, as we potentially learn more about types.

Right now you have it coded up as

(if we can) get an updated CORINFO_METHOD_HANDLE (else abort)

update the GenTreeCall node to the new target (returned from 1) and flags (inferred from change to nonvirtual call)

update the CORINFO_CALL_INFO

I think you might be happier to switch the second two:

(if we can) get an updated CORINFO_METHOD_HANDLE (else abort)

update the CORINFO_CALL_INFO

update the GenTreeCall node to the new target (returned from 1) and flags (returned from 2)

because

You can code the changes to the GenTreeCall as a function of the CORINFO_CALL_INFO, so you may be able to share that code with the importer upstream and optimizations downstream

You can separate the concerns of determining what the new info/attributes should be (which would be the part computing the new CORINFO_CALL_INFO) and determining which fields on a GenTreeCall need to change in response to devirtualization (which could be pretty mechanical). Currently you have a mix of those things in your second and third steps.

It would line up with the desire to move some of this to the VM side, since we're used to the VM populating the CORINFO_CALL_INFO

Wanted to get the post-inline case implemented so whatever approach I finally settle on handles both importer and inliner variants smoothly.

AndyAyersMS · 2017-02-07T01:53:17Z

So with latest updates the jit can devirtualize based on actual return types. For example, in the "sealed default" pattern from #9276:

using System;

public class Base
{
    public virtual int Foo() { return 33; }
    static BaseSealed s_Default = new BaseSealed();
    public static Base Default => s_Default;
}

sealed class BaseSealed : Base {}

public class Test
{
    public static int Main()
    {
        Base b = Base.Default;
        int x = b.Foo();
        return (x == 33 ? 100 : -1);
    }
}

the jit will devirtualize the call to Foo in Main.

Inlining Foo is a bit trickier; my current thinking is that we should split trees in the importer at all virtual calls to make some of the logistics simpler later on.

AndyAyersMS · 2017-02-07T02:53:01Z

Thinking those ARM64 failures may be related to #9375.

AndyAyersMS · 2017-02-07T08:07:37Z

Latest change ends up triggering a lot of non-crossgen fatal R2R errors of the form:

Unspecified error (Exception from HRESULT: 0x80004005 (E_FAIL)) while compiling method Microsoft.CodeAnalysis.CSharp.CSharpCompilation.GetBinderFactory

suspect something is off either with the devirt update to R2R calls or the dependence tracking.

(Update: this was from a private change I never pushed to the fork, so disregard the details here. R2R methods are still failing to encode in some cases, see below).

AndyAyersMS · 2017-02-07T22:01:01Z

Still working through this. Was thrown off by the fact that in R2R mode crossgen will throw/catch for "expected" failures.

AndyAyersMS · 2017-02-07T23:29:27Z

With the 68af9f2 changes we lose 68 methods in R2R images, running into the "Method entrypoint cannot be encoded" notimpl in ZapInfo::getFunctionEntryPoint. So presumably after devirt the entry point info is not getting updated properly and the jit incorrectly tries to make a direct call.

I haven't been able to find a way to re-invoke getCallInfo for the new call target without making things worse. Have tried consing up a resolved token using the class/method handles I already have, as well as trying to recreate the information needed to call resolveToken; both just lead to more problems.

@jkotas any ideas?

jkotas · 2017-02-07T23:42:47Z

src/vm/jitinterface.cpp

+        Assembly* pCallerAssembly = pCallerModule->GetAssembly();
+        Assembly* pDevirtAssembly = pDevirtModule->GetAssembly();
+
+        if (pCallerAssembly == pDevirtAssembly)


This should be just (edited):

if (IsReadyToRunCompilation()) { Assembly * pCallerAssembly = m_pMethodBeingCompiled->GetModule()->GetAssembly(); allowDevirt = IsInSameVersionBubble(pCallerAssembly , pDevirtModule->GetModule()->GetAssembly()) && IsInSameVersionBubble(pCallerAssembly , pMT->GetAssembly()); } else { allowDevirt = true; }

Just to make sure I understand: this extra check is for the case where the class the jit initially identifies as the object type is a subclass of the class that introduces the method, right?

If so, I though I had that covered over on the jit side by mapping back from the devirtualized method to the introducing class, and passing the implementing class to resolveVirtual.

this extra check is for the case where the class the jit initially identifies as the object type is a subclass of the class that introduces the method, right?

Right.

I have not realized that it may be handled in the JIT side. It may be good idea to do the version bubble check defensively here anyway.

Agreed, will update.

Also just to be clear -- no version bubble checks for ngen?

Right. Fragile NGen is like JIT.

jkotas · 2017-02-08T00:24:06Z

@jkotas any ideas?

You can remove the TODO+THROW from ZapInfo::getFunctionEntryPoint. It is possible it will just work because of the getFunctionFixedEntryPoint next to it does not have it.

AndyAyersMS · 2017-02-08T01:06:13Z

Removing the throw leads to an AV in ZapMethodEntryPointTable::GetMethodEntryPoint on since m_pImage->m_pMethodEntryPoints is null.

AndyAyersMS · 2017-02-10T01:54:16Z

@jkotas looks like for R2R it is happy if I reinvoke getCallInfo, this triggers a bunch of stuff on the zap side.
I have to fake up a resolved token but that doesn't seem all that tricky, at least for for non-shared cases.

AndyAyersMS · 2017-02-10T17:45:06Z

Snip from the latest jit-diff stats. It would be really nice if jit-diff would also report how many methods remained unchaged; I'm curious what % of methods this change impacts.

The 41 methods that get lost are still a concern. Looks like most of then now bail out in R2R with: Runtime method access checks not supported. Could be a bug in devirt, since overrides should be just as accessible as the base methods. Will dig in deeper...

Total bytes of diff: -30127 (-0.21 % of base)
    diff is an improvement.

Total byte diff includes -18436 bytes from reconciling methods
        Base had   41 unique methods,    18571 unique bytes
        Diff had    1 unique methods,      135 unique bytes
...
2051 total methods with size differences (1695 improved, 356 regressed).

AndyAyersMS · 2017-02-17T18:24:17Z

Rebased and pushed a few updates. Still pretty rough around the edges.

Jit can now spot "exact" types from newobj and so devirtualize some cases w/o final class or method.

Fixed issue with context update on the call for cases where the base method is from a non-generic class and the derived class is a shared generic.

R2R dropout issues still hit in 41 methods, mostly in Roslyn, still unsure as to what exactly is going wrong. In those cases, the zapper getCallInfo fails with an unsupported access check, so suspect something is amiss with the faked-up resolved token. Note we have to re-call getCallInfo for R2R to trigger proper recording. Have been looking for a simpler repro for this but no luck so far.

AndyAyersMS · 2017-02-20T16:17:01Z

@dotnet-bot test Windows_NT_x64 perf

AndyAyersMS · 2017-02-20T16:17:19Z

@dotnet-bot test Windows_NT arm64 Cross Debug Build

AndyAyersMS · 2017-02-24T18:40:43Z

Rebased, squashed commits down again into two: one for runtime side and one for jit side.

Incorporate some feedback: remove the unused interface method, add the check for __Canon over to the runtime side and remove the overly general check on the jit side.

AndyAyersMS · 2017-02-25T00:14:11Z

Think I cleared up the problems that were causing loss of some R2R methods. No losses seen in the current jit-diff framework set.

@dotnet-bot test Windows_NT Checked r2r

AndyAyersMS · 2017-02-28T03:39:01Z

Pushed an update so the jit interface should be in final form for merging. New GUID and all.
Am going to remove the no merge tag and ask for one last round of review.

@JosephTremoulet PTAL
cc @dotnet/jit-contrib

FYI this will require a coordinated update for desktop. I'll get that ready and kick off desktop testing.

AndyAyersMS · 2017-02-28T04:19:32Z

Another forced update to fix formatting, polish the commit text, and remove an unused declaration.

benaadams · 2017-02-28T04:21:34Z

For example, in the "sealed default" pattern from #9276: ... the jit will devirtualize the call to Foo in Main.

@AndyAyersMS this still true at current iteration? i.e. is it worth reactivating that PR?

AndyAyersMS · 2017-02-28T04:42:29Z

Yes, it will devirtualize (but not inline, yet) -- provided you use the property directly when you make calls.

AndyAyersMS · 2017-02-28T05:57:59Z

CentOS failure (log) was in a PAL test; added note to possibly related #8321. Will retry.

@dotnet-bot retest CentOS7.1 x64 Debug Build and Test

JosephTremoulet

Overall looks good, just a few questions/comments.

JosephTremoulet · 2017-02-28T16:14:31Z

src/jit/flowgraph.cpp

+//    If the parent of the GT_RET_EXPR is a virtual call,
+//    devirtualization is attempted. This should only succeed in the
+//    successful inline case, when the inlinee's return value
+//    expression provides a better type then the declared type of the


typo: then -> than

Fixed and reworded some neighboring text....

JosephTremoulet · 2017-02-28T16:31:39Z

src/jit/lclvars.cpp

        lvaInitVarDsc(varDsc, varNum, strip(corInfoType), typeHnd, localsSig, &info.compMethodInfo->locals);

        varDsc->lvPinned  = ((corInfoType & CORINFO_TYPE_MOD_PINNED) != 0);
        varDsc->lvOnFrame = true; // The final home for this local variable might be our local stack frame
+
+        if (strip(corInfoType) == CORINFO_TYPE_CLASS)


Does this get the same CORINFO_CLASS_HANDLE that the call to getArgType just above got and passed to lvaInitVarDsc? Is so, maybe move this logic down into lvaInitVarDsc?

getArgType doesn't set the class handle for ref types. It is mainly used to establish the representation of the type and not the identity of a type.

JosephTremoulet · 2017-02-28T16:32:06Z

src/jit/lclvars.cpp

@@ -550,6 +558,11 @@ void Compiler::lvaInitUserArgs(InitVarDscInfo* varDscInfo)

        lvaInitVarDsc(varDsc, varDscInfo->varNum, strip(corInfoType), typeHnd, argLst, &info.compMethodInfo->args);

+        if (strip(corInfoType) == CORINFO_TYPE_CLASS)


Same as above regarding moving this into lvaInitVarDsc

Same reason as above here.

JosephTremoulet · 2017-02-28T16:33:24Z

src/jit/importer.cpp

+//     Virtual calls in IL will always "invoke" the base class method.
+//
+//     This transformation looks for evidence that the type of 'this'
+//     in the call is a final class or would invoke a final method, and


nit: or is exact

JosephTremoulet · 2017-02-28T16:34:35Z

src/jit/importer.cpp

+//     if that and other safety checks pan out, modifies the call and
+//     the call info to create a direct call.
+//
+//     This transformation is done in the importer and not in some


This paragraph reads like we only call this before inline, but the same method gets called for late devirtualization, right?

Yes, I need to update the comment.

JosephTremoulet · 2017-02-28T16:42:36Z

src/jit/importer.cpp

+    }
+
+    // If the objClass is sealed (final), then we may be able to devirtualize.
+    const DWORD objClassAttribs = info.compCompHnd->getClassAttribs(objClass);


Are you still planning to do this?

JosephTremoulet · 2017-02-28T16:49:47Z

src/jit/importer.cpp

+    // Virtual calls include an implicit null check, which we may
+    // now need to make explicit. Not sure yet if we can restrict
+    // this to just a subset or need to do it for all cases, so
+    // will do it for all unless we know the object is not null.


Supposing we could restrict this to a subset, would that be up in the code that's setting nullCheck, or down here? If it would be down here, maybe better to invert the sense of the bool and name it knownNotNull or something rather than nullCheck.

When I wrote this I was thinking there might be a locally identifiable subset of calls that would not need checks -- one that could be identified by just looking at the call and its properties. But I no longer believe this.

So, suppressing the check requires some sort of analysis of the 'obj' and doing anything nontrivial here seems risky since we are in the middle of importing.

The string case is trivial so we might as well keep it. I suppose we could also rely on newobj not returning null.

Inverting the sense of the bool looks like a good suggestion since the common case is that we will add a null check.

AndyAyersMS · 2017-02-28T18:50:37Z

Updated to address feedback.

Added a 4.6 compat stub (needed for clean desktop builds).

Removed the distribution table showing which operators lead to type loss. The information is stale and arguably should live somewhere else.

AndyAyersMS · 2017-02-28T21:50:18Z

Haven't hit any issues in desktop testing, though have only run a subset because the GUID/interface change makes getting the full test of tests running a challenge.

AndyAyersMS · 2017-03-01T18:25:35Z

Waiting for builds with #9869 to make their way over to CoreFx before merging this. Believe they should be in the beta25101-02 build of CoreCLR.

AndyAyersMS · 2017-03-01T20:04:42Z

Actually, watching dotnet/corefx#16574

AndyAyersMS · 2017-03-02T01:29:23Z

Local CoreFx changes with these changes look good.

briansull · 2017-03-02T02:46:23Z

LGTM

Add new method to jit interface so the jit can determine what derived method might be called for a given base method, derived class pair. Implement support in the VM and in other places (zap, spmi).

Devirtualize calls where the this object type at a call site is a subtype of the type described at the call site. Currently learns types from local and arg references and a subset of other operators. Will devirtualize if either the class or method is final (sealed in C#), or if the type is known exactly (eg from a newobj). Devirtualization is run twice, once during importation, and again in a limited way after inlinining. Calls devirtualized during importation are are subsequently eligible for inlining. Calls devirtualized during inlining currently cannot be inlined.

AndyAyersMS · 2017-03-02T03:54:31Z

Am going to push one more force update that will reorder things so the non-jit changes are back together in one commit and the jit changes in another.

AndyAyersMS added * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) optimization labels Jan 31, 2017

dnfclas added the cla-already-signed label Jan 31, 2017

jkotas reviewed Feb 1, 2017

View reviewed changes

benaadams mentioned this pull request Feb 2, 2017

Support Encoding devirtualization #9276

Merged

briansull reviewed Feb 2, 2017

View reviewed changes

JosephTremoulet reviewed Feb 6, 2017

View reviewed changes

jkotas reviewed Feb 7, 2017

View reviewed changes

JosephTremoulet mentioned this pull request Feb 13, 2017

Display total method count in addition to changed method count dotnet/jitutils#83

Closed

AndyAyersMS force-pushed the SimpleDevirtualization branch from 16705f1 to 5dde62b Compare February 17, 2017 18:14

AndyAyersMS force-pushed the SimpleDevirtualization branch from 25112a4 to 398bc6e Compare February 24, 2017 18:38

AndyAyersMS mentioned this pull request Feb 27, 2017

Some tests for devirtualization #9834

Merged

AndyAyersMS force-pushed the SimpleDevirtualization branch from ac18d9a to 59136f8 Compare February 28, 2017 03:34

AndyAyersMS removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 28, 2017

AndyAyersMS force-pushed the SimpleDevirtualization branch from 2328df3 to 40b1841 Compare February 28, 2017 04:18

JosephTremoulet reviewed Feb 28, 2017

View reviewed changes

JosephTremoulet approved these changes Mar 1, 2017

View reviewed changes

AndyAyersMS added 2 commits March 1, 2017 19:50

Jit interface support for devirtualization

70cf442

Add new method to jit interface so the jit can determine what derived method might be called for a given base method, derived class pair. Implement support in the VM and in other places (zap, spmi).

AndyAyersMS force-pushed the SimpleDevirtualization branch from dd298a8 to 393ee92 Compare March 2, 2017 04:14

AndyAyersMS merged commit e0637ea into dotnet:master Mar 2, 2017

benaadams mentioned this pull request Mar 4, 2017

Seal safehandles to devirtualize aspnet/KestrelHttpServer#1443

Closed

NinoFloris mentioned this pull request May 10, 2017

Seal FSharpFunc / FSharpTypeFunc implementers to benefit from devirt 🎉 dotnet/fsharp#3020

Closed

karelz modified the milestone: 2.0.0 Aug 28, 2017

AndyAyersMS mentioned this pull request Jan 31, 2020

JIT: devirtualization next steps dotnet/runtime#7541

Open

26 tasks

		JIT_TO_EE_TRANSITION();

		result = resolveVirtualMethodExact(methodHnd, derivedClass);

		@@ -550,6 +558,11 @@ void Compiler::lvaInitUserArgs(InitVarDscInfo* varDscInfo)

		lvaInitVarDsc(varDsc, varDscInfo->varNum, strip(corInfoType), typeHnd, argLst, &info.compMethodInfo->args);

		if (strip(corInfoType) == CORINFO_TYPE_CLASS)

Simple devirtualization #9230

Simple devirtualization #9230

Conversation

AndyAyersMS commented Jan 31, 2017 • edited Loading

AndyAyersMS commented Jan 31, 2017

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndyAyersMS commented Feb 3, 2017

AndyAyersMS commented Feb 3, 2017

AndyAyersMS commented Feb 4, 2017

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndyAyersMS commented Feb 7, 2017

AndyAyersMS commented Feb 7, 2017

AndyAyersMS commented Feb 7, 2017 • edited Loading

AndyAyersMS commented Feb 7, 2017

AndyAyersMS commented Feb 7, 2017

jkotas Feb 7, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas commented Feb 8, 2017 • edited Loading

AndyAyersMS commented Feb 8, 2017

AndyAyersMS commented Feb 10, 2017

AndyAyersMS commented Feb 10, 2017

AndyAyersMS commented Feb 17, 2017

AndyAyersMS commented Feb 20, 2017

AndyAyersMS commented Feb 20, 2017

AndyAyersMS commented Feb 24, 2017

AndyAyersMS commented Feb 25, 2017

AndyAyersMS commented Feb 28, 2017

AndyAyersMS commented Feb 28, 2017

benaadams commented Feb 28, 2017

AndyAyersMS commented Feb 28, 2017

AndyAyersMS commented Feb 28, 2017

JosephTremoulet left a comment

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndyAyersMS commented Feb 28, 2017

AndyAyersMS commented Feb 28, 2017

AndyAyersMS commented Jan 31, 2017 •

edited

Loading

AndyAyersMS commented Feb 7, 2017 •

edited

Loading

jkotas Feb 7, 2017 •

edited

Loading

jkotas commented Feb 8, 2017 •

edited

Loading