Int32.ToString() too slow

Question

I have the following for a position class:

public struct Pos
{
    public int x;
    public int y;
    public float height;

    public Pos (int _x, int _y, float _height) 
    {
        x = _x;
        y = _y;
        height = _height;
    }

    public override string ToString () 
    {
        return x.ToString() + "," + y.ToString();
    }
}

But since I am calling Pos.ToString() thousands of times, this is too slow for me. All I need is an efficient way to get a single unique value based on Pos.x and Pos.y, for use as a dictionary key. Note: I cannot use Pos because I am comparing different instances of Pos on merely x and y.

Don't use `ToString` for dictionary keys. Implement `IEquatable` instead. — Yuval Itzchakov, Sep 03 '15 at 12:32
No need for `ToString()` in concatenation. Just use `x + "," + y` — NASSER, Sep 03 '15 at 12:32
string.Format("{0},{1}", x ,y) should make it slightly better — Yahya, Sep 03 '15 at 12:34
What about using `(long) ((x << 32) | y)` as the dictionary key? It would be much faster, I suppose. — vojta, Sep 03 '15 at 12:37
@Yahya that makes it like 10-100x slower because of all the format string parsing. — usr, Sep 03 '15 at 12:38
@usr still better than `x.ToString() + "," + y.ToString();`, no? — Yahya, Sep 03 '15 at 12:39
@Yahya 100x slower does not mean faster. string.format does the same concatenation work that you could do manually. In addition it has to do format string processing. It does not take away work in any way. It adds. — usr, Sep 03 '15 at 12:40
@usr suggesting string.Format came to mind to reduce time creating new string objects, or is it just a myth? — Yahya, Sep 03 '15 at 12:44
@Yahya there is no more expensive way to create a string. There is no magic in it. It parses the format string, then calls ToString on everything, then concats,. — usr, Sep 03 '15 at 12:45
@usr but string.Format("{0}, {1}", "abc", "def") should perform better than plain concatenation? — Yahya, Sep 03 '15 at 12:47
@usr Sorry for being naive, thought I would clarify rather than assume. Learnt something new today! — Yahya, Sep 03 '15 at 12:51
@Yahya: string format is painfully slow and complex, because the string needs to be parsed and interpreted before being formatted. If you look at how Java implements it, for example, you'll see it involves very complex operations (while plain concatenation is quite simple) — njzk2, Sep 03 '15 at 16:05

Yuval Itzchakov · Accepted Answer · 2015-09-03T12:40:27.443

29

All I need is an efficient way to get a single unique value based on Pos.x and Pos.y, for use as a dictionary key.

Don't use ToString as a way to generate unique dictionary keys, implement IEquatable<Pos> instead. This way, you don't have to allocate any strings at all to measure equality:

public struct Pos : IEquatable<Pos>
{
    public int X { get; private set; }
    public int Y { get; private set; }
    public float Height { get; private set; }

    public Pos(int x, int y, float height)
    {
        X = x;
        Y = y;
        Height = height;
    }

    public bool Equals(Pos other)
    {
        return X == other.X && Y == other.Y;
    }

    public override bool Equals(object obj)
    {
        if (ReferenceEquals(null, obj)) return false;
        return obj is Pos && Equals((Pos) obj);
    }

    public override int GetHashCode()
    {
        unchecked
        {
            return (X*397) ^ Y;
        }
    }

    public static bool operator ==(Pos left, Pos right)
    {
        return left.Equals(right);
    }

    public static bool operator !=(Pos left, Pos right)
    {
        return !left.Equals(right);
    }
}

Note you can remove the private set from the properties declarations if you're using C#-6.

edited Sep 03 '15 at 12:40

answered Sep 03 '15 at 12:35

Yuval Itzchakov

146,575
32
257
321

2

One tiny improvement: In C#6 you can remove the `private set` from the X and Y properties. – Matthew Watson Sep 03 '15 at 12:39
1

I know, but not everyone is using C#6, so I went with the lenghty version. – Yuval Itzchakov Sep 03 '15 at 12:40
1

@vojta You want to use large prime numbers, since you want to try and avoid collisions. See [this post](http://stackoverflow.com/questions/263400/what-is-the-best-algorithm-for-an-overridden-system-object-gethashcode) – Yuval Itzchakov Sep 03 '15 at 12:51
@EhsanSajjad Thank you :) – Yuval Itzchakov Sep 03 '15 at 12:54
A number as small as 397 already counts as a LARGE prime? When hearing the term "large prime number" then I would rather think of a number with 397 or so digits. – Kaiserludi Sep 03 '15 at 15:37
@Kaiseludi Lets call it an intermediate sized prime. The point itself remains the same. It's all about context. :) – Yuval Itzchakov Sep 03 '15 at 15:38
2

const int OPTIMUS_PRIME = 397; // local constant in GetHashCode() – Mystra007 Sep 03 '15 at 17:21
3

Since this class is immutable, you can get a small performance improvement by computing the hash once, in the constructor rather than each time `GetHashCode` is called. This would be at the cost of additional storage for the hash. – Jack A. Sep 03 '15 at 17:37
1

@JackA Or lazily the first name it is called. You're right. – Yuval Itzchakov Sep 03 '15 at 17:38
1

@JackA.since the dictionary will memoise the hash itself and so only call `GetHashCode()` once on storage and once on lookup (on what is very likely a different key object) that cost of the additional storage is almost never worth it. – Jon Hanna Dec 04 '15 at 10:00

Int32.ToString() too slow

1 Answers1