Skip to content

Remove strong reference of XNamespace from XName #116

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.

Sign up for GitHub

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

Closed
buyaa-n wants to merge 6 commits into from
Closed

Remove strong reference of XNamespace from XName #116

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3b1d6e4
Remove strong reference of XNamespace in XName
buyaa-n Nov 18, 2019
2e534e0
Removing not used using statement
buyaa-n Nov 19, 2019
bce52ec
Addressing feedback
buyaa-n Nov 19, 2019
769732f
Applying feedback
buyaa-n Nov 20, 2019
c1042f8
Reverting back strong reference, changed _names table from XHastable<…
buyaa-n Nov 22, 2019
9078dbc
Removing not used import
buyaa-n Nov 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 48 additions & 36 deletions src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/XNamespace.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,14 @@ public sealed class XNamespace
internal const string xmlPrefixNamespace = "http://www.w3.org/XML/1998/namespace";
internal const string xmlnsPrefixNamespace = "http://www.w3.org/2000/xmlns/";

private static XHashtable<WeakReference> s_namespaces;
private static WeakReference s_refNone;
private static WeakReference s_refXml;
private static WeakReference s_refXmlns;
private static XHashtable<WeakReference<XNamespace>> s_namespaces;
private static WeakReference<XNamespace> s_refNone;
private static WeakReference<XNamespace> s_refXml;
private static WeakReference<XNamespace> s_refXmlns;

private readonly string _namespaceName;
private readonly int _hashCode;
private readonly XHashtable<XName> _names;
private readonly XHashtable<WeakReference<XName>> _names;
Copy link
Member

@stephentoub stephentoub Nov 23, 2019

Choose a reason for hiding this comment

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

What's the reason for this change (and all of the follow-on changes from it)?

Presumably it's to avoid keeping a live an XName object? But this is still going to keep-alive a WeakReference<XName> object even if it loses its reference to the XName, so is there something deeper it's aiming to do? e.g. is the XName transitively keeping alive a large graph and that's the problem?

I'm also concerned about potential perf impact. Have you measured? The comments on XHashtable suggest this code is very perf sensitive, yet we're introducing additional allocations and indirections here to access the data in the WeakReference<T>.

Copy link
Contributor Author

@buyaa-n buyaa-n Nov 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

What's the reason for this change (and all of the follow-on changes from it)?

Namespce keeps its related XNames into this table which is growing endlessly and causing memory leak when adding elements continuously, simple repro:

while (true)
{
    XName _ = $"x{Guid.NewGuid()}";
}

At first I have removed strong ref from XName into XNamespace, which fixed above problem by allowing XNamespace GC-ed, but that eliminates Atomicity of XName and XNamespace, no duplicate NameSpace should exist and no duplicate XName within a NS. If we are keeping a XName instance its Namespace should also kept. So I was need to restore the strong reference back.

Presumably it's to avoid keeping a live an XName object?

Yes, making XNames weak within hashtable enables non referenced XNames GC-ed.

But this is still going to keep-alive a WeakReference object even if it loses its reference to the XName, so is there something deeper it's aiming to do? e.g. is the XName transitively keeping alive a large graph and that's the problem?

The XHashtable used here built to be used with WeakRefence, it cleans up the key which value is released due to WeakReference and nulls the corresponding WeakReference. So XHashtable will not grow tremendously unless all XNames within it kept referenced

I'm also concerned about potential perf impact. Have you measured? The comments on XHashtable suggest this code is very perf sensitive, yet we're introducing additional allocations and indirections here to access the data in the WeakReference.

Yes I have measured performance, the results not that stable on my machine, below result could show average of all perf run results:

dotnet run -f netcoreapp2.1 --base "D:\dotnet\results2\before" --diff "D:\dotnet\results2\after3" --threshold 2%
summary:
better: 2, geomean: 1.073
worse: 5, geomean: 1.110
total diff: 7
Slower diff/base Base Median (ns) Diff Median (ns) Modality
System.Xml.Linq.Perf_XElement.CreateWithElements 1.15 80.00 92.07 several?
System.Xml.Linq.Perf_XElement.GetAttribute 1.12 26.86 30.14 several?
System.Xml.Linq.Perf_XElement.GetElement 1.10 37.22 41.03 several?
System.Xml.Linq.Perf_XName.GetLocalNameFromExpandedName 1.09 28.65 31.26
System.Xml.Linq.Perf_XName.GetLocalName 1.08 25.88 28.05
Faster base/diff Base Median (ns) Diff Median (ns) Modality
System.Xml.Linq.Perf_XName.NonEmptyNameSpaceToString 1.09 28.11 25.84
System.Xml.Linq.Perf_XElementList.Enumerator 1.06 239.92 226.58 several?

I have been testing perf with other possible solutions i had and compared to them this result looks acceptable, I didn't found any other/better solution, if you guys have any idea please let me know

Performance sensitivity of XHashtable is mostly from thread safety which I already existing. Additional overhead to the table will be removing invalid WeakRefernces which is as we know, for good.

Copy link
Member

@stephentoub stephentoub Nov 25, 2019

Choose a reason for hiding this comment

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

Did you experiment with using a ConditionalWeakTable instead of XHashtable?

this result looks acceptable

The perf tests demonstrate a 6-15% regression depending on the use case. It's not clear to me why that's acceptable. What other approaches did you explore?

Copy link
Member

@krwq krwq Nov 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@buyaa-n do you perhaps have data on how much that 6-15% shows up in the E2E scenarios? (i.e. large XML document + some queries). I'm not feeling strongly either way on 15% on any of the specific APIs but more interested in E2E impact (if it's still 15% on E2E then that is rather large perf hit). From above data System.Xml.Linq.Perf_XElement.GetElement seems the most concerning to me as it's rather frequently used API (at least I personally use it quite a bit).

Copy link
Contributor Author

@buyaa-n buyaa-n Nov 25, 2019

Choose a reason for hiding this comment

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

Did you experiment with using a ConditionalWeakTable instead of XHashtable?

No, will try with that and measure performance

The perf tests demonstrate a 6-15% regression depending on the use case. It's not clear to me why that's acceptable.

As i mentioned before the performance results was not that consistent, i had 3 before, 3 after measurements and checked their combinations. I will post result of those runs in next comment as its long content. Most impacted section should be XName related tests which were not increased more than 10% and some cases even get faster. I might do perf tests again by creating as much stable env as i can.

What other approaches did you explore?

Well most of the other approaches were not really solving the issue, one of them was my initial approach I had here. Here is some link to others buyaa-n/corefx#4, buyaa-n/corefx#5

Copy link
Member

@stephentoub stephentoub Nov 26, 2019

Choose a reason for hiding this comment

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

What are those numbers comparing?

Copy link
Contributor Author

@buyaa-n buyaa-n Nov 26, 2019

Choose a reason for hiding this comment

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

As perf results was stable i had measured before change and after change 3 times each and compared their combinations. Now I think better to setup Linux env and run the tests

Copy link
Contributor Author

@buyaa-n buyaa-n Dec 6, 2019

Choose a reason for hiding this comment

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

Did you experiment with using a ConditionalWeakTable instead of XHashtable?

@stephentoub I tried but that is not really suitable for this issue.


private const int NamesCapacity = 8; // Starting capacity of XName table, which must be power of 2
private const int NamespacesCapacity = 32; // Starting capacity of XNamespace table, which must be power of 2
Expand All @@ -35,7 +35,7 @@ internal XNamespace(string namespaceName)
{
_namespaceName = namespaceName;
_hashCode = namespaceName.GetHashCode();
_names = new XHashtable<XName>(ExtractLocalName, NamesCapacity);
_names = new XHashtable<WeakReference<XName>>(ExtractLocalName, NamesCapacity);
}

/// <summary>
Expand Down Expand Up @@ -216,11 +216,23 @@ internal XName GetName(string localName, int index, int count)

// Attempt to get the local name from the hash table
XName name;
if (_names.TryGetValue(localName, index, count, out name))
return name;

// No local name has yet been added, so add it now
return _names.Add(new XName(this, localName.Substring(index, count)));
if (_names.TryGetValue(localName, index, count, out WeakReference<XName> nameRef))
{
if (!nameRef.TryGetTarget(out name))
{
name = new XName(this, localName.Substring(index, count));
nameRef.SetTarget(name);
}
}
else
{
// No local name has yet been added, so add it now
name = new XName(this, localName.Substring(index, count));
_names.Add(new WeakReference<XName>(name));
}

return name;
}

/// <summary>
Expand All @@ -236,51 +248,51 @@ internal static XNamespace Get(string namespaceName, int index, int count)

// Use CompareExchange to ensure that exactly one XHashtable<WeakReference> is used to store namespaces
if (s_namespaces == null)
Interlocked.CompareExchange(ref s_namespaces, new XHashtable<WeakReference>(ExtractNamespace, NamespacesCapacity), null);
Interlocked.CompareExchange(ref s_namespaces, new XHashtable<WeakReference<XNamespace>>(ExtractNamespace, NamespacesCapacity), null);

WeakReference refNamespace;
XNamespace ns;

// Keep looping until a non-null namespace has been retrieved
do
Copy link
Member

@stephentoub stephentoub Nov 23, 2019

Choose a reason for hiding this comment

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

Why is this loop no longer necessary?

Copy link
Contributor Author

@buyaa-n buyaa-n Nov 24, 2019

Choose a reason for hiding this comment

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

Before it was using non generic WeakReference and even weakReference of a Namespace found from table its target might be null, so it was need to loop again when target is was null. But now we are using WeakReference<XNamespace> which TryGetTarget(out ns) which returns target atomically, i. e. no need to check for null of out variable ns and loop again in case TryGetTarget(out ns) the method returns true. In case of ref not found or ns was not alive we are just creating new instance, so no need to loop for that case too

// Attempt to get the WeakReference for the namespace from the hash table
if (s_namespaces.TryGetValue(namespaceName, index, count, out WeakReference<XNamespace> refNamespace))
{
// Attempt to get the WeakReference for the namespace from the hash table
if (!s_namespaces.TryGetValue(namespaceName, index, count, out refNamespace))
if (!refNamespace.TryGetTarget(out ns))
{
// If it is not there, first determine whether it's a special namespace
if (count == xmlPrefixNamespace.Length && string.CompareOrdinal(namespaceName, index, xmlPrefixNamespace, 0, count) == 0) return Xml;
if (count == xmlnsPrefixNamespace.Length && string.CompareOrdinal(namespaceName, index, xmlnsPrefixNamespace, 0, count) == 0) return Xmlns;

// Go ahead and create the namespace and add it to the table
refNamespace = s_namespaces.Add(new WeakReference(new XNamespace(namespaceName.Substring(index, count))));
ns = new XNamespace(namespaceName.Substring(index, count));
refNamespace.SetTarget(ns);
}
}
else
{
// determine whether it's a special namespace
if (count == xmlPrefixNamespace.Length && string.CompareOrdinal(namespaceName, index, xmlPrefixNamespace, 0, count) == 0) return Xml;
if (count == xmlnsPrefixNamespace.Length && string.CompareOrdinal(namespaceName, index, xmlnsPrefixNamespace, 0, count) == 0) return Xmlns;

ns = (refNamespace != null) ? (XNamespace)refNamespace.Target : null;
// Go ahead and create the namespace and add it to the table
ns = new XNamespace(namespaceName.Substring(index, count));
s_namespaces.Add(new WeakReference<XNamespace>(ns));
}
while (ns == null);

return ns;
}

/// <summary>
/// This function is used by the <see cref="XHashtable{XName}"/> to extract the local name part from an <see cref="XName"/>. The hash table
/// This function is used by the <see cref="XHashtable{WeakReference}"/> to extract the local name part from an <see cref="XName"/>. The hash table
/// uses the local name as the hash key.
/// </summary>
private static string ExtractLocalName(XName n)
private static string ExtractLocalName(WeakReference<XName> r)
{
Debug.Assert(n != null, "Null name should never exist here");
if (r == null || !r.TryGetTarget(out XName n))
return null;
return n.LocalName;
}

/// <summary>
/// This function is used by the <see cref="XHashtable{WeakReference}"/> to extract the XNamespace that the WeakReference is
/// referencing. In cases where the XNamespace has been cleaned up, this function returns null.
/// </summary>
private static string ExtractNamespace(WeakReference r)
private static string ExtractNamespace(WeakReference<XNamespace> r)
{
XNamespace ns;

if (r == null || (ns = (XNamespace)r.Target) == null)
if (r == null || !r.TryGetTarget(out XNamespace ns))
return null;

return ns.NamespaceName;
Expand All @@ -292,9 +304,9 @@ private static string ExtractNamespace(WeakReference r)
/// since other threads can be concurrently calling this method, and the target of a WeakReference can be cleaned up
/// at any time by the GC.
/// </summary>
private static XNamespace EnsureNamespace(ref WeakReference refNmsp, string namespaceName)
private static XNamespace EnsureNamespace(ref WeakReference<XNamespace> refNmsp, string namespaceName)
{
WeakReference refOld;
WeakReference<XNamespace> refOld;

// Keep looping until a non-null namespace has been retrieved
while (true)
Expand All @@ -305,13 +317,13 @@ private static XNamespace EnsureNamespace(ref WeakReference refNmsp, string name
if (refOld != null)
{
// If the target of the WeakReference is non-null, then we're done--just return the value
XNamespace ns = (XNamespace)refOld.Target;
if (ns != null) return ns;
if (refOld.TryGetTarget(out XNamespace ns))
return ns;
}

// Either refNmsp is null, or its target is null, so update it
// Make sure to do this atomically, so that we can guarantee atomicity of XNamespace objects
Interlocked.CompareExchange(ref refNmsp, new WeakReference(new XNamespace(namespaceName)), refOld);
Interlocked.CompareExchange(ref refNmsp, new WeakReference<XNamespace>(new XNamespace(namespaceName)), refOld);
}
}
}
Expand Down
61 changes: 61 additions & 0 deletions src/libraries/System.Private.Xml.Linq/tests/misc/XHashtable.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,5 +170,66 @@ public void DifferentNamespaceAndNameSameHashcodeElements()
Assert.NotSame(a.Name.Namespace, b.Name.Namespace);
Assert.NotSame(a.Name, b.Name);
}

[Fact]
public void DifferentNamespaceAndSameNameProduceDifferentXNameInstance()
{
XName a = "{Namespace1}Name1", b = "{Namespace2}Name1";
Assert.NotSame(a.Namespace, b.Namespace);
Assert.NotSame(a, b);
Assert.NotSame(a.ToString(), b.ToString());
Assert.Equal(a.LocalName, b.LocalName);
}

[Fact]
public void SameDefaultNamespaceSameNameProduceSameXNameInstance()
{
XName a = "Name2";
var b = new XElement($"Name2");
Assert.Same(a.Namespace, b.Name.Namespace);
Assert.Same(a, b.Name);
Assert.Same(XNamespace.None, a.Namespace);
Assert.Same(a.ToString(), b.Name.ToString());
}

[Fact]
public void SameNamespaceSameStringDifferentInstanceProduceSameXNameInstance()
{
string instance1 = "Name3";
string instance2 = $"Name3{null}"; // different string instance populated
XName a = instance1;
var b = new XElement(instance2);
Assert.NotSame(instance1, instance2);
Assert.Same(a.Namespace, b.Name.Namespace);
Assert.Same(a, b.Name);
Assert.Same(XNamespace.None, a.Namespace);
Assert.Same(a.ToString(), b.Name.ToString());
}

[Fact]
public void SameNamespaceDifferentNameProduceDifferentXNameInstance()
{
XName a = "{Namespace2}Name4";
var b = new XElement("{Namespace2}Name5");
Assert.Same(a.Namespace, b.Name.Namespace);
Assert.NotSame(a, b.Name);
Assert.Same(a.Namespace.ToString(), b.Name.Namespace.ToString());
}

[Fact]
public void SameNamespaceSameNameProduceSameXNameInstanceAfterGC()
{
XName a = "Name6";

GC.Collect();
GC.WaitForPendingFinalizers();
GC.Collect();

var b = new XElement($"Name6");
Assert.Same(a.Namespace, b.Name.Namespace);
Assert.Same(a, b.Name);
Assert.Same(XNamespace.None, a.Namespace);
Assert.Same(a.ToString(), b.Name.ToString());
}
}
}