EndsWith calls in Constraint constructor can cause major perf issues #1555

Closed
jskeet opened this Issue Jun 8, 2016 · 9 comments

Projects

None yet

4 participants

@jskeet
jskeet commented Jun 8, 2016 edited

This is really due to a CoreCLR issue (which I've filed) but the constructor for Constraint performs three EndsWith checks:

DisplayName = this.GetType().Name;
if (DisplayName.EndsWith("`1") || DisplayName.EndsWith("`2"))
    DisplayName = DisplayName.Substring(0, DisplayName.Length - 2);
if (DisplayName.EndsWith("Constraint"))
    DisplayName = DisplayName.Substring(0, DisplayName.Length - 10);

That's nice and quick when the current culture is en-US and the invariant culture, but slow as molasses under other cultures. When pretty much every assertion (as far as I can see) creates a new constraint, that's far from ideal.

Firstly, given that this is going to be a fixed value per constraint type anyway, I wonder whether it would make sense to cache this or let the caller pass it in - but even if the code is broadly kept the same, just changing this to use EndsWith(..., StringComparison.Ordinal) would make it even faster than the en-US codepath.

@rprouse
Member
rprouse commented Jun 8, 2016

Thanks @jskeet I will try to get the fix for this in 3.4.

@CharliePoole I am bumping the priority on this based on the fact that it is slowing tests down on Linux drastically. It is probably also an easy fix and a decent optimization on all platforms to cache.

@rprouse rprouse added this to the 3.4 milestone Jun 8, 2016
@CharliePoole
Member

Whoever takes this should try to look a bit deeper and see how we actually use it. A lot of this low-level stuff dates back to when I was creating the fluent interface and constraints. I had to do a lot of debugging and Constraint.ToString was very important. It may be that much of this has outlived its usefulness.

@jskeet
jskeet commented Jun 9, 2016

Out of interest, is there any reason this couldn't go into 3.3? It looks like such a safe change - which I know is normally a recipe for problems, but I'm struggling to see how it would be a problem this time.

(I'd suggest performing the easy fix now, and either keeping this issue open for the deeper analysis Charlie mentioned, or creating a separate issue for that. But I realize this isn't my project to manage, and I'll shut up now :)

@CharliePoole
Member

With 3.4 (that's our next) still about 3 weeks off, I guess we can give this a little thought. Removing the property, if it's unused, is also pretty risk-free.

@jskeet
jskeet commented Jun 9, 2016

Ah - sorry, my mistake. I saw that 3.2.1 was current and that there was a 3.3.0-rc, and assumed that 3.3.0 would be the next release.

The fact that it's a public property means that removing it would be a breaking change :(

@CharliePoole
Member

Where did you see 3.3 RC? That would be a glitch in our build labelling. Since we follow the odd / even convention used by many other projects, all 3.3 builds are in the unstable development line. They never reach release, so the terms alpha, beta and RC wouldn't make sense.

Technically, it would be a breaking change, so we would have to evaluate that. Just saying, it shouldn't take long for one of us who is familiar with the code to think before coding. :-)

@jskeet
jskeet commented Jun 9, 2016

Well this is embarrassing: it turns out I can't read properly. On https://www.nuget.org/packages/NUnit, I misread "NUnit Version 3 3.0.0-rc-3" as "NUnit Version 3.3.0.0-rc-3". Doh.

@CharliePoole
Member

Happens to the best of us!

@rprouse rprouse added a commit that referenced this issue Jun 10, 2016
@rprouse rprouse Simple fix for #1555 1f1cad0
@rprouse rprouse added the closed:done label Jun 10, 2016
@ltrzesniewski

Since this is currently in the hot code path, perhaps you could evaluate the property lazily instead, something like this:

private string _displayName;

public string DisplayName
{
    get
    {
        if (_displayName == null)
        {
            _displayName = this.GetType().Name;
            if (_displayName.EndsWith("`1", StringComparison.Ordinal) || _displayName.EndsWith("`2", StringComparison.Ordinal))
                _displayName = _displayName.Substring(0, _displayName.Length - 2);
            if (_displayName.EndsWith("Constraint", StringComparison.Ordinal))
                _displayName = _displayName.Substring(0, _displayName.Length - 10);
        }
        return _displayName;
    }
    protected set { _displayName = value; }
}

This would change the behavior of dubious code like DisplayName = null, but who cares?

@rprouse rprouse modified the milestone: 3.4 Jun 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment