New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

StringBuilder.Clear() changes Capacity #34257

Open
fanoI opened this Issue Dec 27, 2018 · 13 comments

Comments

Projects
None yet
6 participants
@fanoI
Copy link

fanoI commented Dec 27, 2018

This simple code show the issue:

            StringBuilder sb = new StringBuilder();

            Assert.IsTrue(sb.Capacity == 16, "StringBuilder.Capacity is wrong");

            Assert.IsTrue(sb.MaxCapacity == Int32.MaxValue, "StringBuilder.MaxCapacity is wrong");

            Assert.IsTrue(sb.Length == 0, "StringBuilder.MaxCapacity is wrong");

            sb.Append("This ");
            sb.Append("is ");
            sb.Append("a test");

            /* Now sb.Lenght should be 14 (the leng of 'This is a test') */
            Assert.IsTrue(sb.Length == 14, "After Append StringBuilder.Lenght is wrong");

            Assert.IsTrue(sb.ToString() == "This is a test", "StringBuilder.Append() does not work");

            sb.Append("...again");

            Assert.IsTrue(sb.Capacity == 32, "StringBuilder.Capacity is wrong (not doubled!)");

            Assert.IsTrue(sb.Length == 22, "After Append StringBuilder.Lenght is wrong");

            Assert.IsTrue(sb.ToString() == "This is a test...again", "StringBuilder.Append() again does not work");

            sb.Clear();
            Console.WriteLine($"sb.Capacity {sb.Capacity}");

            // Capacity should not change after Clear
            Assert.IsTrue(sb.Capacity == 32, "StringBuilder.Capacity after Clear is wrong");

this worked on Net Core 2.0 but not anymore in Net Core 2.1 neither 2.2.
It seems now StringBuilder.Clear() is doing more than simply set Lenght to 0 that is totally not expected, the Capacity indeed becomes 26.

The original Net Framework passes this test.

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Dec 27, 2018

This is expected side-effect of dotnet/coreclr#16926

@MarcoRossignoli

This comment has been minimized.

Copy link
Collaborator

MarcoRossignoli commented Dec 27, 2018

And seem expected also by guide https://docs.microsoft.com/en-us/dotnet/api/system.text.stringbuilder.capacity?view=netcore-2.2#remarks

The StringBuilder dynamically allocates more space when required and increases Capacity accordingly. For performance reasons, a StringBuilder might allocate more memory than needed. The amount of memory allocated is implementation-specific.
@fanoI

This comment has been minimized.

Copy link

fanoI commented Dec 27, 2018

Yes it is implementation specif but this should remain true:
https://docs.microsoft.com/en-us/dotnet/api/system.text.stringbuilder.clear?view=netcore-2.2

Clear is a convenience method that is equivalent to setting the Length property of the current instance to 0 (zero).
Calling the Clear method does not modify the current instance's Capacity or MaxCapacity property.

@MarcoRossignoli

This comment has been minimized.

Copy link
Collaborator

MarcoRossignoli commented Dec 27, 2018

@fanoI look like you'are right...I missed that 😕

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Dec 27, 2018

I think we just need to update the docs.

cc @vancem @maryamariyan

@fanoI

This comment has been minimized.

Copy link

fanoI commented Dec 27, 2018

@jkotas so you consider acceptable to break compatibility with Net Framework on this?

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Dec 27, 2018

We maintain compatibility. We do not maintain bug-for-bug compatibility. This falls into the bug-for-bug compatibility for me.

@fanoI

This comment has been minimized.

Copy link

fanoI commented Dec 27, 2018

Probably no existing code relied on the fact that Clear() does not changed the Capacity properties but it was in your documentation in any case.

So I would have to remove that test as in Net Core 2.1 this is not guaranteed anymore?

@danmosemsft

This comment has been minimized.

Copy link
Member

danmosemsft commented Dec 27, 2018

@fanoI if you mean your own test (?) then yes, it will need to be adjusted or deleted.

Could you please open a bug (or offer a PR) in https://github.com/dotnet/dotnet-api-docs to fix the docs?

@fanoI

This comment has been minimized.

Copy link

fanoI commented Dec 27, 2018

Yes I was meaning my tests in Cosmos, probably we could test that after Clear() the Len is 0 or something similar.

I've opened the issue here as requested: dotnet/dotnet-api-docs#1482

@bencyoung

This comment has been minimized.

Copy link

bencyoung commented Dec 31, 2018

Hmm, we make use of the fact that you can have a threadlocal StringBuilder that you can continually clear and then append to again and not allocate if the new line is shorter than the old. Does that mean that guarantee no longer applies? What are the new guarentees?

We certainly did make use of the previous documentation here.

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Dec 31, 2018

Setting a StringBuilder.Length to zero can allocate if the StringBuilder buffers are fragmented. It was always the case in .NET Core and it was the case in .NET Framework for a very long time. This extra allocation will happen once for given capacity. The pattern you are describing works well.

Change dotnet/coreclr#16926 just caused it to allocate a little bit more than strictly necessary to avoid a pathological corner case.

We certainly did make use of the previous documentation here.

The current documentation says nothing about whether Clear allocates or not. It just says the Capacity property does not change.

@bencyoung

This comment has been minimized.

Copy link

bencyoung commented Dec 31, 2018

Fair enough, we'll assume it's asymptotically ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment