Skip to content

Remove some StringBuilderCache usage from Corelib - #64522

Merged
stephentoub merged 2 commits into
dotnet:mainfrom
stephentoub:corelibsbcache
Jan 31, 2022
Merged

Remove some StringBuilderCache usage from Corelib#64522
stephentoub merged 2 commits into
dotnet:mainfrom
stephentoub:corelibsbcache

Conversation

@stephentoub

@stephentoubstephentoub commented Jan 31, 2022

Copy link
Copy Markdown
Member

No description provided.

@ghost

ghost commented Jan 31, 2022

Copy link
Copy Markdown

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost

ghost commented Jan 31, 2022

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-meta
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author:stephentoub
Assignees:stephentoub
Labels:

area-Meta

Milestone:-

danmoseley
danmoseley reviewed Jan 31, 2022
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Globalization/TimeSpanParse.cs Outdated
case '\"':
StringBuilder enquotedString = StringBuilderCache.Acquire();
if (!DateTimeParse.TryParseQuoteString(format, i, enquotedString, out tokenLen))
var enquotedString = new ValueStringBuilder(32);

@danmoseleydanmoseleyJan 31, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

would it make sense to be slightly less conservative? 64?

@stephentoubstephentoubJan 31, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

sure

@danmoseley

danmoseley commented Jan 31, 2022

Copy link
Copy Markdown
Contributor

BTW, I noticed the comment higher up StreamWriter about #8890

The docs I've seen say that it's not guaranteed that stackallocs are zero inited - although the issue above implies we always do. Should we document that we do without SkipLocalsInit, and don't with SkipLocalsInit? Otherwise it's hard to reason about code.

@stephentoub

stephentoub commented Jan 31, 2022

Copy link
Copy Markdown
MemberAuthor

BTW, I noticed the comment higher up StreamWriter about #8890

I don't believe SkipLocalsInit is relevant to that comment. It's talking about ref temporaries on the stack, which need to be zeroed even with SkipLocalsInit.

@stephentoub
stephentoub merged commit 9058470 into dotnet:mainJan 31, 2022
@stephentoub
stephentoub deleted the corelibsbcache branch January 31, 2022 16:53
This was referenced Feb 8, 2022
Closed
Closed
@kunalspathakkunalspathak mentioned this pull request Feb 24, 2022
Closed
@ghostghost locked as resolved and limited conversation to collaborators Mar 2, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@danmoseleydanmoseleydanmoseley approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@stephentoubstephentoub

Labels

area-Meta

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@stephentoub@danmoseley