-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Fix NamespaceCache
usage in System.Private.Xml.Linq
#101158
Conversation
Tagging subscribers to this area: @dotnet/area-system-xml |
private readonly NamespaceCache _eCache; | ||
private readonly NamespaceCache _aCache; | ||
private NamespaceCache _eCache; | ||
private NamespaceCache _aCache; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this occurred as part of moving these from being locals to being fields as part of d88ba5b#diff-b2b608017bdbacf5e4484368d6e1fcb03a9ea3b68f4aece41da253f72826acf9R908. Would they be better again as locals? I'm not sure why they were moved to be fields, but they're increasing the size of the ContentReader object by being here.
cc: @krwq
@hamarb123 please see @stephentoub's comments |
So, we want to move them to locals and pass them by reference to the methods? |
Except that won't work for the async methods... since it's a ref |
So, it seems like previously all the stuff was done in Please confirm the preferred approach @krwq @stephentoub |
/ping @krwq @stephentoub ^ |
Ok, let's keep them as fields for now but revisit subsequently. |
See https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/XLinq.cs#L181-L193 - it's intended to be mutated on
Get
so that it can cache theXNamespace
object - the fields being markedreadonly
completely block all caching.