Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Reduce allocation in Index/Range.ToString#21755

Merged
stephentoub merged 2 commits into
dotnet:masterfrom
stephentoub:rangeindextostring
Jan 3, 2019
Merged

Reduce allocation in Index/Range.ToString#21755
stephentoub merged 2 commits into
dotnet:masterfrom
stephentoub:rangeindextostring

Conversation

@stephentoub

Copy link
Copy Markdown
Member

Index.ToString first calls ToString on the integer value, and then if it's FromEnd, concatenates with "^", resulting in an extra string allocation in the FromEnd case. Range.ToString then builds on that, not only generating the intermediate Index strings, but boxing those Index structs to call string.Concat(object, object, object).

This change removes the extra allocation, so there's at most one allocation for Index/Range.ToString, the resulting string itself. For Index, there might still be zero allocations in the case where it just delegates to Value.ToString(), and it's able to return a cached string for a single-digit integer value.

private Index _indexCached = new Index(1, false);
private Index _indexFromStart = new Index(42, false);
private Index _indexFromEnd = new Index(42, true);
private Range _rangeFromStart = Range.Create(new Index(42, false), new Index(43, false));
private Range _rangeFromEnd = Range.Create(new Index(43, true), new Index(42, true));

[Benchmark] public string IndexCached() => _indexCached.ToString();
[Benchmark] public string IndexFromStartToString() => _indexFromStart.ToString();
[Benchmark] public string IndexFromEndToString() => _indexFromEnd.ToString();
[Benchmark] public string RangeFromStartToString() => _rangeFromStart.ToString();
[Benchmark] public string RangeFromEndToString() => _rangeFromEnd.ToString();

Before:

                 Method |      Mean |     Error |    StdDev |  Gen 0 | Allocated |
----------------------- |----------:|----------:|----------:|-------:|----------:|
            IndexCached |  14.41 ns | 0.3629 ns | 0.3883 ns |      - |       0 B |
 IndexFromStartToString |  21.62 ns | 0.5399 ns | 1.5750 ns | 0.0076 |      32 B |
   IndexFromEndToString |  35.04 ns | 0.7046 ns | 0.6591 ns | 0.0153 |      64 B |
 RangeFromStartToString |  73.13 ns | 1.7092 ns | 2.2224 ns | 0.0362 |     152 B |
   RangeFromEndToString | 108.04 ns | 2.0858 ns | 1.8490 ns | 0.0516 |     216 B |

After:

                 Method |     Mean |     Error |    StdDev |  Gen 0 | Allocated |
----------------------- |---------:|----------:|----------:|-------:|----------:|
            IndexCached | 14.19 ns | 0.0979 ns | 0.0818 ns |      - |       0 B |
 IndexFromStartToString | 22.70 ns | 0.1632 ns | 0.1363 ns | 0.0076 |      32 B |
   IndexFromEndToString | 34.87 ns | 0.6725 ns | 0.5962 ns | 0.0076 |      32 B |
 RangeFromStartToString | 55.33 ns | 0.3032 ns | 0.2192 ns | 0.0095 |      40 B |
   RangeFromEndToString | 55.92 ns | 0.3618 ns | 0.3021 ns | 0.0095 |      40 B |

cc: @tarekgh, @jkotas

@GrabYourPitchforks GrabYourPitchforks left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: should we have a debug assert after the calls to TryFormat? I could go either way on this.

((uint)Value).TryFormat(span.Slice(1), out int charsWritten);
span[0] = '^';
return new string(span.Slice(0, charsWritten + 1));
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an alternative - modified version of UInt32ToDecStr without CountDigits, etc

private static unsafe string UInt32ToDecStr(uint value)
{
    char* buf = stackalloc char[11];
    int i = 11;
    do
    {
        uint div = value / 10;
        buf[--i] = (char)('0' + value - (div * 10));
        value = div;
    }
    while (value != 0);
    buf[--i] = '^';
    return new string(buf, i, 11 - i);
}

@stephentoub stephentoub Jan 2, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. But:
a) I personally don't think it's worth all that extra duplication.
b) That actually allocates more in what may be a common case of small values like an index for 0 or 1, as it'll bypass that small-number string cache that UInt32ToDecStr uses. (EDIT: Though I see now you're proposing only using this for the FromEnd case, in which case that cache wouldn't apply anyway.)
c) It's not clear to me yet whether these methods really need to be hyper-optimized; I suspect they don't.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stephentoub yeah, surely doesn't make any sense, I was actually curious if your technique with stackalloc[11] could be used in UInt32ToDecStr, but looks like GetDigitsCount+string.FastAllocate is better 🙂

@stephentoub

Copy link
Copy Markdown
Member Author

Nit: should we have a debug assert after the calls to TryFormat? I could go either way on this.

Thanks, Levi. I considered that, too, but was on the fence and ended up removing them to avoid the clutter. Since you also suggested it, I'll add them back.

@stephentoub

Copy link
Copy Markdown
Member Author

@dotnet-bot test Windows_NT arm Cross Debug Innerloop Build please
@dotnet-bot test Windows_NT arm64 Cross Debug Innerloop Build please

@stephentoub stephentoub merged commit 88eb93a into dotnet:master Jan 3, 2019
@stephentoub stephentoub deleted the rangeindextostring branch January 3, 2019 20:08
Dotnet-GitSync-Bot pushed a commit to Dotnet-GitSync-Bot/mono that referenced this pull request Jan 3, 2019
* Reduce allocation in Index/Range.ToString

* Address PR feedback

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Dotnet-GitSync-Bot pushed a commit to Dotnet-GitSync-Bot/corert that referenced this pull request Jan 3, 2019
* Reduce allocation in Index/Range.ToString

* Address PR feedback

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Dotnet-GitSync-Bot pushed a commit to Dotnet-GitSync-Bot/corefx that referenced this pull request Jan 3, 2019
* Reduce allocation in Index/Range.ToString

* Address PR feedback

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
jkotas pushed a commit to dotnet/corert that referenced this pull request Jan 3, 2019
* Reduce allocation in Index/Range.ToString

* Address PR feedback

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
stephentoub added a commit to Dotnet-GitSync-Bot/corefx that referenced this pull request Jan 4, 2019
* Reduce allocation in Index/Range.ToString

* Address PR feedback

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
marek-safar pushed a commit to mono/mono that referenced this pull request Jan 4, 2019
* Reduce allocation in Index/Range.ToString

* Address PR feedback

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
stephentoub added a commit to dotnet/corefx that referenced this pull request Jan 4, 2019
* Reduce allocation in Index/Range.ToString

* Address PR feedback

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
marek-safar pushed a commit to mono/corefx that referenced this pull request Feb 12, 2019
* Reduce allocation in Index/Range.ToString

* Address PR feedback

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
(cherry picked from commit ceee48b)
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
* Reduce allocation in Index/Range.ToString

* Address PR feedback


Commit migrated from dotnet/coreclr@88eb93a
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants