2
\$\begingroup\$

I've made a very basic wrapper over a String that offers mutability in place (context: mono/XNA, budget is about 11ms per frame, 0B (ZERO, NIL, НОЛЬ, NADA) allocations hard requirement). Before that I've used a wrapper over StringBuilder that essentially would do the same - get raw value of the string it operates on.

public class MutableString
{
    // Setup fields
    private int m_Pos;
    private string m_valueStr;
    private readonly bool dontThrow = false;

    // Publics
    public int Capacity { get { return m_valueStr.Length; } }
    public int Length { get { return m_Pos; } }

    public MutableString(int size) : this(size, '\0', false) { }
    public MutableString(int size, bool ignoreOverflow) : this(size, '\0', ignoreOverflow) { }
    public MutableString(int size, char fillChar) : this(size, fillChar, false) { }

    public MutableString(int size, char fillChar, bool ignoreOverflow)
    {
        if (size < 1)
            throw new ArgumentException("Size cannot be 0 or less");
        m_valueStr = new string(fillChar, size);
        dontThrow = ignoreOverflow;
    }

    public override string ToString()
    {
        if (m_Pos == 0)
            return string.Empty;

        var free = m_valueStr.Length - m_Pos;
        if (free > 0)
            repeatChar('\0', free);

        m_Pos = 0;
        return m_valueStr;
    }


    //
    // LOGIC
    //
    #region CHARS
    public void Append(char[] value, int indx, int count)
    {
        if (value == null)
            return;
        var len = value.Length;
        if (len == 0 || count < 1 || indx < 0 || (count > len - indx))
            return;
        if (len > 1)
            AppendInternal(value, indx, count);
        else
            Append(value[0]);
    }

    public void Append(char[] value)
    {
        if (value == null)
            return;
        var len = value.Length;
        if (len > 1)
            AppendInternal(value, 0, len);
        else
            Append(value[0]);
    }

    public void Append(char value)
    {
        if (m_Pos >= m_valueStr.Length)
        {
            if (dontThrow)
                return;
            else
                throw new ArgumentException("Not enough free space to accomodate element!");
        }   
        singleChar(value);
        m_Pos++;
    }

    private void AppendInternal(char[] value, int indx, int count)
    {
        var free = m_valueStr.Length - m_Pos;
        if (count > free)
        {
            if (dontThrow)
                return;
            else
                throw new ArgumentException(string.Format("Not enough free space to accomodate {0} elements!", count));
        }
        charCopy(value, indx, count);
        m_Pos = m_Pos + count;
    }
    #endregion

    #region STRINGS
    private void AppendInternal(string value, int indx, int count)
    {
        var free = m_valueStr.Length - m_Pos;
        if (count > free)
        {
            if (dontThrow == true)
                return;
            else
                throw new ArgumentOutOfRangeException(string.Format("Not enough free space to accomodate {0} elements!", count));
        }
        stringCopy(value, indx, count);
        m_Pos = m_Pos + count;
    }

    public void Append(string value, int indx, int count)
    {
        if (value == null)
            return;
        var len = value.Length;
        if (count < 1 || indx < 0 || (count > len - indx))
            return;
        if (len > 1)
            AppendInternal(value, indx, count);
        else
            Append(value[0]);
    }

    public void Append(string value)
    {
        if (value == null)
            return;
        var len = value.Length;
        if (len > 1)
            AppendInternal(value, 0, len);
        else
            Append(value[0]);
    }
    #endregion


    // Copy logic

    private unsafe void stringCopy(string value, int indx, int charCount)
    {
        fixed (char* ptrDest = m_valueStr)
        {
            fixed (char* ptrSrc = value)
            {
                wstrcpy(ptrDest + m_Pos, ptrSrc + indx, charCount);
            }
        }
    }

    private unsafe void charCopy(char[] value, int indx, int charCount)
    {
        fixed (char* ptrDest = m_valueStr)
        {
            fixed (char* ptrSrc = value)
            {
                wstrcpy(ptrDest + m_Pos, ptrSrc + indx, charCount);
            }
        }
    }

    private unsafe void singleChar(char value)
    {
        fixed (char* ptrDest = m_valueStr)
        {
            ptrDest[m_Pos] = value;  
        }
    }

    private unsafe void repeatChar(char value, int count)
    {
        var fin = m_Pos + count;
        fixed (char* ptrDest = m_valueStr)
        {
            while (m_Pos < fin)
            {
                ptrDest[m_Pos] = value;
                m_Pos++;
            }
        }
    }

    private unsafe void rawCopy(char* dest, char* src, int charCount)
    {
        for (int i = 0; i < charCount; i++)
            dest[i] = src[i];
    }

    private unsafe static void wstrcpy(char* dmem, char* smem, int charCount)
    {
        if (((int)dmem & 2) != 0)
        {
            *dmem = *smem;
            dmem++;
            smem++;
            charCount--;
        }

        while (charCount >= 8)
        {
            *(uint*)dmem = *(uint*)smem;
            *(uint*)(dmem + 2) = *(uint*)(smem + 2);
            *(uint*)(dmem + 4) = *(uint*)(smem + 4);
            *(uint*)(dmem + 6) = *(uint*)(smem + 6);
            dmem += 8;
            smem += 8;
            charCount -= 8;
        }

        if ((charCount & 4) != 0)
        {
            *(uint*)dmem = *(uint*)smem;
            *(uint*)(dmem + 2) = *(uint*)(smem + 2);
            dmem += 4;
            smem += 4;
        }

        if ((charCount & 2) != 0)
        {
            *(uint*)dmem = *(uint*)smem;
            dmem += 2;
            smem += 2;
        }

        if ((charCount & 1) != 0)
        {
            *dmem = *smem;
        }
    }
}

The issue is that my performance over SB wrapper is way slower for some reason. on my Core i7-3770K I get like +0.600-0.700ms performance drop for 10 000 operations run. And it makes zero sense code wise, I have less call overhead and less branching, code-wise.

Is there anything I'm missing or can do? Also, unrelated general feedback is welcomed.

UPD: forgot to say aiming .net3.5 for max compatibility with mono UPD2: got it, had to swap while to for in repeatChar() looks like JIT wasnt able to work out some magic with while.

\$\endgroup\$
8
  • \$\begingroup\$ So why not the StringBuilder? Are you intentionally trying to reinvent the wheel? We have a tag for that... \$\endgroup\$ – slepic Sep 23 '20 at 4:26
  • 1
    \$\begingroup\$ Do we also have tag for people who dont know what they are talking about but want to share their condescending opinion? \$\endgroup\$ – Izukai Sep 23 '20 at 4:29
  • 3
    \$\begingroup\$ It's a legitimate question. If you have reasons to not use StringBuilder, just speak them out, it may not be obvious to others. Arrogance will get you nowhere... \$\endgroup\$ – slepic Sep 23 '20 at 4:39
  • 1
    \$\begingroup\$ Your fallacy has nothing to do with my question because my question isnt "Should I use StringBuilder or use my custom solution", I was way past that last week after extensive profiling and research. My question is very specific and you are now being agitated after being pointed out your commentary is unneeded and oftopic. \$\endgroup\$ – Izukai Sep 23 '20 at 4:49
  • 1
    \$\begingroup\$ @Izukai Is Span<char> or Memory<char> out of scope? With them you could reduce the direct usage of unsafe code. From performance perspective it seems a valid option. You could also take advantage of ArrayPool<char> to reduce memory allocation if it matters for you from performance perspective. \$\endgroup\$ – Peter Csala Sep 23 '20 at 6:16
1
\$\begingroup\$

Just two observations:

  • your code does not state prominently what it, what every public member is there for:
    don't write, never present undocumented.

  • While I expect early out with C# and len == 0 needs no preceding computation, I'd use (count < 1 || index < 0 || (len < index + count))

\$\endgroup\$
1
\$\begingroup\$

There are a couple of cosmetic things that might improve your code. E.g. you can declare multiple pointers in in each fixed statement:

fixed (char* ptrDest = m_valueStr)
{
    fixed (char* ptrSrc = value)
    {
        wstrcpy(ptrDest + m_Pos, ptrSrc + indx, charCount);
    }
}

Becomes:

fixed (char* ptrDest = m_valueStr, ptrSrc = value)
{
    wstrcpy(ptrDest + m_Pos, ptrSrc + indx, charCount);
}

Note: it's normal in C# for all methods to be PascalCase.

I'd also suggest that your public Append methods return this to allow chaining like a normal StringBuilder does:

var a = new MutableString(10);
a.Append("A string")
   .Append("!");

Now, my only functional comment is that your ToString method is interesting. I'd go as far as saying it's broken.

var a = new MutableString(1);
a.Append("a");
var areEqual = a.ToString() == a.ToString();

I think anyone used to C# would say that, clearly, areEqual is true. Alas, no. You reset m_pos so the second call returns an empty string. Very odd.

Without seeing the benchmark code and alternate StringBuilder implementation, I'll shy away from the performance differences.

I had a bit of a look and saw a good speedup from using Buffer.MemoryCopy. Although, I only ran the benchmark on .Net Core so you'll want to double check results. My bad.

private unsafe static void wstrcpy(char* dmem, char* smem, int charCount)
{
    // C# char is always 2 bytes.
    Buffer.MemoryCopy(smem, dmem, charCount * 2, charCount * 2);
}
\$\endgroup\$
3
  • \$\begingroup\$ Thanks for the cosmetics, these are great. Regarding the .ToString() - it is not broken it is very specific. As you can see I mutate the string and I fill the remaining of it with null-term chars - it is not intended to be used in proper comparison, the approach not compatible with either that or multithreading. Idea is that it will be called every 11ms to concat a lot of string data and I need no allocations at all, including the return result. And yeah, wish I could use Buffer, aiming .Net3.5 for max compat. \$\endgroup\$ – Izukai Sep 23 '20 at 11:32
  • \$\begingroup\$ @Izukai - Take a look at the reference source and pull out the MemoryCopy into your own code ;) In that case, I think you'd be better off having a method called CreateAndReset (or similar). ToString() doesn't usually mutate an instance - it's best to keep to those conventions. \$\endgroup\$ – RobH Sep 23 '20 at 11:42
  • \$\begingroup\$ I think it is a bit more complex than just copying like I did with widestrcopy (I even preserved their name): referencesource.microsoft.com#mscorlib/system/… In fact not even sure it will play along with old .net or mono 2.x \$\endgroup\$ – Izukai Sep 23 '20 at 11:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.