Introduce proxy id to simplify identification #379

zvirja · 2018-03-13T14:08:59Z

Closes #44

In this PR I've implemented the proxy id feature. which allows to recognize the particular instance of the proxy. The ID looks like following:

Substitute.PrimaryProxyType#42

Where PrimaryProxyType - the short type name of the primary proxy type, #42 - auto-incremented field (global counter, not per type).

To achieve the desired behavior I've changed the following:

Always make a substitute for the class. If interfaces are specified only, we substitute for the System.Object. That is required to intercept the ToString() method, otherwise Castle doesn't hook it.
Add the ProxyIdInterceptor to return proxy id when ToString() is called. Given that our interceptor participates earlier than forwarding one, calls to the ToString() are transparent for the NSubstutite core.
We do not intercept ToString() if type has its own implementation.

I've tested the code and it works fine in VS debugger - proxy ID is shown for the debugger view:

Due to some reasons Rider doesn't want to evaluate ToString(), however we can still see the proxy ID:

Hope one day they will improve their evaluator and it will invoke the complex ToString() method as well.

Of course, everything works fine with assertions, as they invoke ToString() method:

        [Test]
        public void CollectionAssertion()
        {
            var array = new[] {Substitute.For<IInterface>(), Substitute.For<IInterface>()};
            var item = Substitute.For<IInterface>();
            
            Assert.That(array, Contains.Item(item));
        }

and error:

  Expected: some item equal to <NSubstitute.Proxy.IInterface#3>
  But was:  < <NSubstitute.Proxy.IInterface#1>, <NSubstitute.Proxy.IInterface#2> >

I don't like a bit that now we are always forced to create proxy for the class. However, that should not affect the performance too much and the benefit seems to worth it.

@alexandrnikitin @dtchepak Please take a look and let me know your opinion. Thanks 🌹

zvirja · 2018-03-14T20:59:09Z

UPDATE. I've changed ID to make it shorter. Now it looks like following:

Substitute.PrimaryProxyType#42

It should allow to save space in the different error messages and looks nicer.

zvirja · 2018-03-21T09:23:28Z

@dtchepak Previously you asked to ping you explicitly if you don't reply for a while. Have you noticed this PR (and a few other ones nearby)? 😊

dtchepak · 2018-03-21T23:02:28Z

Arghh I didn't get an email/notification! Sorry! Thanks for pinging!

dtchepak

Looks good! Seems like a very neat solution to this. Have you done any quick measures on perf differences with this extra interception? Might be worth running a quick comparison and making sure it doesn't add much overhead on construction or interception.

I wonder if we could simplify this but still meet the immediate need by switching to Java-style Object.toString which just shoves the hashCode on the end? So for Substitute.For<IFoo>() we'd get Substitute.IFoo@{hashCode} (or hashcode in hex). This may help avoid people getting confused trying to assign meaning to the sequence number generated (as per the IFoo#1/IBar#2 comment).

I was initially expecting this to be implemented via the CallRouter mixed-in via ProxyGenerationOptions. If this logic could go in CallRouter it might give us a good hook to support explicitly tagging substitutes later on? (like this example, where we might want to say sub1.Name("good"); sub2.Name("bad") or similar.)

Sorry for the rambling comment, I just wanted to jot down some notes during my initial look at the PR. I'd appreciate your thoughts on any of this stuff.

dtchepak · 2018-03-21T23:57:07Z

src/NSubstitute/Proxies/CastleDynamicProxy/CastleDynamicProxyFactory.cs

+                var id = Interlocked.Increment(ref _idCounter);
+                var shortTypeName = primaryProxyType.GetNonMangledTypeName();
+
+                return string.Format(CultureInfo.InvariantCulture, "Substitute.{0}#{1}", shortTypeName, id);


I wonder if this format is slightly misleading as it looks like the number refers to the specific type?
e.g. Substitute.For<IFoo>(), Substitute.For<IBar> gives IFoo#1 and IBar#2.

I don't think I can come up with anything better (Substitute#{1} [{0}]?), but if you've got any ideas around this I'd love to hear them.

I wonder if this format is slightly misleading as it looks like the number refers to the specific type?

Fixed by using the pipe - it brings much better semantics and it's clear that values are not related.

I don't think I can come up with anything better (Substitute#{1} [{0}]?)

My goal was to make a substitute id as concise as possible. Some test frameworks have limits on the number of characters they show in e.g. error messages. Therefore the shorter the ID is, the more information will fit into the screen/message. Additionally, it's better to not have spaces in the ID - we never know how they will be formatted).

Personally, I feel that Substitute.type#id is the best tradeoff between readability and length.

I wonder if we could simplify this but still meet the immediate need by switching to Java-style Object.toString which just shoves the hashCode on the end? So for Substitute.For() we'd get Substitute.IFoo@{hashCode} (or hashcode in hex). This may help avoid people getting confused trying to assign meaning to the sequence number generated (as per the IFoo#1/IBar#2 comment).

Just realized that you are completely right. This counter might look strange to people, as it's global across all the tests and they might expect that counter is local to the proxy type. If we stick to the hash code, we'll likely always have large number, so it will be clear to user that numbers are non-predictable. I've changed the code to fix that.

Now about the format. I see a few options:

Substitute.IInterface#007877b1 - padded hex

Substitute.IInterface#13b9a - non padded hex

Substitute.IInterface#59207139 - non-padded decimal

I suggest us to stick to hex, as hash code might be negative and will look ugly in decimal: Substitute.IInterface#-7894961. Also the perfectionist inside me votes for padding 😅

As for the delimiter, I see the following options:

Substitute.IInterface#007877b1

Substitute.IInterface:007877b1

Substitute.IInterface|007877b1

Substitute.IInterface`007877b1

Substitute.IInterface@007877b1

Substitute.IInterface/007877b1

Substitute.IInterface\007877b1

Substitute.IInterface_007877b1

Substitute.IInterface-007877b1

Substitute.IInterface.007877b1

Substitute.IInterface$007877b1

Substitute.IInterface!007877b1

Substitute.IInterface%007877b1

Substitute.IInterface^007877b1

Substitute.IInterface*007877b1

Substitute.IInterface:#007877b1

Substitute.IInterface[007877b1]

Personally, I like the first 4 options, but decided to proceed with pipe as it clearly divide parts. Here how the message looks with pipes:

Expected: some item equal to <Substitute.IAdviseSink|03dc30f2> But was: < <Substitute.IAdviseSink|03624f36>, <Substitute.IAdviseSink|03ac360b> >

Notice, we cannot use proxy.GetType().Name to get ID, because it will always return ObjectProxy no matter what we mock. It's less useful than the primary type name.

dtchepak · 2018-03-21T23:58:58Z

src/NSubstitute/Proxies/CastleDynamicProxy/CastleDynamicProxyFactory.cs

            }
        }

-        private void VerifyNoConstructorArgumentsGivenForInterface(object[] constructorArguments)
+        private class ProxyIdGenerator


Not sure it's worthwhile, but we could conceivably reuse SequenceNumberGenerator (with a new ctor that lets the count start from 0) here and inline the method so we lose the additional class. WDYT?

Removed the counter at all in favor of GetHashCode().

In order to simplify proxy identification we override the ToString() method to return an unique proxy id. We don't override the ToString() method if proxied type provides its own implementation.

zvirja · 2018-03-22T17:37:16Z

@dtchepak See my replies in the comments as well.

I was initially expecting this to be implemented via the CallRouter mixed-in via ProxyGenerationOptions. If this logic could go in CallRouter it might give us a good hook to support explicitly tagging substitutes later on? (like this example, where we might want to say sub1.Name("good"); sub2.Name("bad") or similar.)

I thought that, but later realized that it's a bad idea. If we decide to move call through the CallRouter, it will modify the last active router on dispatch. It might have very bad consequences for the whole NSubsitute functionality, because this method is sometimes called implicitly by e.g.:

string.Format()
string concatentation
debugger, when you hover your mouse.

I want ToString() invocation to be:

completely transparent for all our business logic, so we don't worry about those invocations at all.
very fast, so there are less chances that debugger will refuse to evaluate it.

The first point might be achieved by adding the extra conditions inside the CallRouter.Route() and CastleInvocationMapper. But that will force us to always remember about this special case and one day we'll overlook something 😉

The feature with explicit naming will be still possible even with current architecture, as it's possible to pass ICallRouter instance to the ProxyIdInterceptor. So it could check e.g. router.SubstituteName property and return it if it's not null. It's only one of the possible ways - believe we have the better ones.

Given that I prefer to have a separate interceptor as the best guarantee of isolation. I believe that overhead is negligible.

Have you done any quick measures on perf differences with this extra interception? Might be worth running a quick comparison and making sure it doesn't add much overhead on construction or interception.

I'll follow up on that a bit later, when I have more free time.

dtchepak · 2018-03-22T22:54:29Z

Thanks @zvirja, yet another awesome PR :)

zvirja · 2018-03-23T16:29:24Z

@dtchepak Here are the performance testing results:

Code

public class NSubstituteBenchmark
{
    private ClassWithIntMethod _classProxy;
    private IInterfaceWithIntMethod _interfaceProxy;

    public NSubstituteBenchmark()
    {
        this._classProxy = Substitute.For<ClassWithIntMethod>();
        this._interfaceProxy = Substitute.For<IInterfaceWithIntMethod>();
    }

    [Benchmark]
    public ClassWithIntMethod CreateClassProxy() => Substitute.For<ClassWithIntMethod>();

    [Benchmark]
    public IInterfaceWithIntMethod CreateInterfaceProxy() => Substitute.For<IInterfaceWithIntMethod>();

    [Benchmark]
    public string InvokeToStringInterfaceProxy() => _interfaceProxy.ToString();

    [Benchmark]
    public string InvokeToStringClassProxy() => _classProxy.ToString();

    [Benchmark]
    public int InvokeRegularMethodInterfaceProxy() => _interfaceProxy.GetInt();

    [Benchmark]
    public int InvokeRegularMethodClassProxy() => _classProxy.GetInt();

    public interface IInterfaceWithIntMethod
    {
        int GetInt();
    }

    public abstract class ClassWithIntMethod
    {
        public abstract int GetInt();
    }
}

Environment:

BenchmarkDotNet=v0.10.13, OS=Windows 10 Redstone 2 [1703, Creators Update] (10.0.15063.786)
Intel Core i7-4770 CPU 3.40GHz (Haswell), 1 CPU, 8 logical cores and 4 physical cores
Frequency=3320311 Hz, Resolution=301.1766 ns, Timer=TSC
  [Host]     : .NET Framework 4.6.2 (CLR 4.0.30319.42000), 64bit RyuJIT-v4.7.2115.0
  DefaultJob : .NET Framework 4.6.2 (CLR 4.0.30319.42000), 64bit RyuJIT-v4.7.2115.0

Before:

                            Method |        Mean |      Error |      StdDev |
---------------------------------- |------------:|-----------:|------------:|
                  CreateClassProxy | 5,011.48 ns | 95.3044 ns | 105.9305 ns |
              CreateInterfaceProxy | 5,124.30 ns | 26.9081 ns |  23.8533 ns |
      InvokeToStringInterfaceProxy |    10.95 ns |  0.0948 ns |   0.0887 ns |
          InvokeToStringClassProxy |    10.77 ns |  0.0822 ns |   0.0642 ns |
 InvokeRegularMethodInterfaceProxy | 4,636.77 ns | 77.1161 ns |  64.3955 ns |
     InvokeRegularMethodClassProxy | 6,981.80 ns | 63.3611 ns |  52.9094 ns |

After:

                            Method |        Mean |      Error |     StdDev |
---------------------------------- |------------:|-----------:|-----------:|
                  CreateClassProxy | 5,134.43 ns | 58.4588 ns | 54.6824 ns |
              CreateInterfaceProxy | 5,539.99 ns | 71.3262 ns | 63.2288 ns |
      InvokeToStringInterfaceProxy |    48.91 ns |  1.1124 ns |  1.8890 ns |
          InvokeToStringClassProxy |    47.88 ns |  0.1797 ns |  0.1593 ns |
 InvokeRegularMethodInterfaceProxy | 4,728.07 ns | 59.1856 ns | 49.4227 ns |
     InvokeRegularMethodClassProxy | 6,997.70 ns | 30.7895 ns | 22.2628 ns |

As you can see, there is almost no difference in performance and overhead from extra interceptor is negligible.

The only sensible difference is in the CreateInterfaceProxy - that's because now we create proxy for the Object, rather than IInterface only. However, there is no way to avoid that - otherwise ToString() is not hooked.

Also you can see how quick the InvokeToStringClassProxy is compared to InvokeRegularMethodClassProxy - one more value from the separate interceptor 😄

zvirja force-pushed the add-proxy-id branch from ee99c0c to 59882d5 Compare March 14, 2018 20:57

zvirja force-pushed the add-proxy-id branch from 59882d5 to 24534a9 Compare March 21, 2018 09:21

dtchepak reviewed Mar 22, 2018

View reviewed changes

Introduce proxy id to simplify identification

51f14f7

In order to simplify proxy identification we override the ToString() method to return an unique proxy id. We don't override the ToString() method if proxied type provides its own implementation.

zvirja force-pushed the add-proxy-id branch from 24534a9 to 51f14f7 Compare March 22, 2018 17:10

dtchepak merged commit 6a5414e into nsubstitute:master Mar 22, 2018

dtchepak mentioned this pull request Mar 22, 2018

Identifying substitutes #44

Open

zvirja deleted the add-proxy-id branch March 23, 2018 10:29

zvirja mentioned this pull request May 4, 2018

Improve interface proxy generation performance #394

Closed

dtchepak mentioned this pull request Apr 5, 2020

Is it possible to create many objects from a specific interface, but different type names? #617

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce proxy id to simplify identification #379

Introduce proxy id to simplify identification #379

zvirja commented Mar 13, 2018 •

edited

zvirja commented Mar 14, 2018

zvirja commented Mar 21, 2018

dtchepak commented Mar 21, 2018

dtchepak left a comment

dtchepak Mar 21, 2018

zvirja Mar 22, 2018 •

edited

dtchepak Mar 21, 2018

zvirja Mar 22, 2018

zvirja commented Mar 22, 2018 •

edited

dtchepak commented Mar 22, 2018

zvirja commented Mar 23, 2018 •

edited

Introduce proxy id to simplify identification #379

Introduce proxy id to simplify identification #379

Conversation

zvirja commented Mar 13, 2018 • edited

zvirja commented Mar 14, 2018

zvirja commented Mar 21, 2018

dtchepak commented Mar 21, 2018

dtchepak left a comment

Choose a reason for hiding this comment

dtchepak Mar 21, 2018

Choose a reason for hiding this comment

zvirja Mar 22, 2018 • edited

Choose a reason for hiding this comment

dtchepak Mar 21, 2018

Choose a reason for hiding this comment

zvirja Mar 22, 2018

Choose a reason for hiding this comment

zvirja commented Mar 22, 2018 • edited

dtchepak commented Mar 22, 2018

zvirja commented Mar 23, 2018 • edited

zvirja commented Mar 13, 2018 •

edited

zvirja Mar 22, 2018 •

edited

zvirja commented Mar 22, 2018 •

edited

zvirja commented Mar 23, 2018 •

edited