Skip to content
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

Restore: improve DependencyGraphSpec.Save(...) performance #3215

Merged
merged 1 commit into from
Feb 20, 2020
Merged

Restore: improve DependencyGraphSpec.Save(...) performance #3215

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
52 changes: 33 additions & 19 deletions src/NuGet.Core/NuGet.Packaging/RuntimeModel/IObjectWriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;

namespace NuGet.RuntimeModel
Expand All @@ -11,6 +12,17 @@ namespace NuGet.RuntimeModel
/// </summary>
public interface IObjectWriter
{
/// <summary>
/// Writes the start of the root object or an object in an array.
///
/// This new object becomes the scope for all other method calls until either WriteObjectStart
/// is called again to start a new nested object or WriteObjectEnd is called.
///
/// Every call to WriteObjectStart must be balanced by a corresponding call to WriteObjectEnd.
/// </summary>
/// <exception cref="ObjectDisposedException">Thrown if this object is disposed.</exception>
void WriteObjectStart();
Copy link
Member

Choose a reason for hiding this comment

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

Last time, someone complained about this breaking change: NuGet/Home#8080.

Since then they have fixed their dependency https://github.com/dotnet/arcade/pull/2664/files, but we need to be aware that people do take dependencies.

We can say, the client tooling comes first though :)

Copy link
Contributor

Choose a reason for hiding this comment

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

We just observed this breaking change in dotnet/runtime#48462 after upgrading the SDK to 6.0. You could argue that our custom packaging code should be replaced by NuGet's pack task, but we aren't there yet.

https://github.com/dotnet/arcade/blob/1ca4a4637bf32494c8ba579c32e4512309cae751/src/Microsoft.DotNet.Build.Tasks.Packaging/src/NuGetUtility.cs#L94

cc @Anipik @ericstj

Copy link
Contributor

Choose a reason for hiding this comment

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

What is interesting about this is, even though the change was merged in Feb 2020, it didn't make it into 5.0.100. Was that intentional? The fix for us in dotnet/arcade is just to reference newer nuget packages to update what we bind against.

Copy link
Member

Choose a reason for hiding this comment

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

This change is in 5.0.100, https://github.com/NuGet/NuGet.Client/blob/release-5.8.x/src/NuGet.Core/NuGet.Packaging/RuntimeModel/IObjectWriter.cs

Or at least should be according to the branch and tags.

Can you help me understand the problem you are seeing and why you think it's not in 5.0.100.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think @nkolev92 was commenting that this is breaking because they added a member to an interface. We aren't broken by that addition. I think the break we're currently seeing is something different.


/// <summary>
/// Writes the start of a nested object.
///
Expand All @@ -19,7 +31,9 @@ public interface IObjectWriter
///
/// Every call to WriteObjectStart must be balanced by a corresponding call to WriteObjectEnd.
/// </summary>
/// <param name="name">The name of the object. Throws if <c>null</c>.</param>
/// <param name="name">The name of the object.</param>
/// <exception cref="ArgumentNullException">Thrown if <paramref name="name" /> is <c>null</c>.</exception>
/// <exception cref="ObjectDisposedException">Thrown if this object is disposed.</exception>
void WriteObjectStart(string name);

/// <summary>
Expand All @@ -30,42 +44,53 @@ public interface IObjectWriter
///
/// Every call to WriteObjectStart must be balanced by a corresponding call to WriteObjectEnd.
/// </summary>
/// <exception cref="ObjectDisposedException">Thrown if this object is disposed.</exception>
void WriteObjectEnd();

/// <summary>
/// Writes a name-value pair.
/// </summary>
/// <param name="name">The name of the datum. Throws if <c>null</c>.</param>
/// <param name="name">The name of the datum.</param>
/// <param name="value">The datum.</param>
/// <exception cref="ArgumentNullException">Thrown if <paramref name="name" /> is <c>null</c>.</exception>
/// <exception cref="ObjectDisposedException">Thrown if this object is disposed.</exception>
void WriteNameValue(string name, int value);

/// <summary>
/// Writes a name-value pair.
/// </summary>
/// <param name="name">The name of the datum. Throws if <c>null</c>.</param>
/// <param name="name">The name of the datum.</param>
/// <param name="value">The datum.</param>
/// <exception cref="ArgumentNullException">Thrown if <paramref name="name" /> is <c>null</c>.</exception>
/// <exception cref="ObjectDisposedException">Thrown if this object is disposed.</exception>
void WriteNameValue(string name, bool value);

/// <summary>
/// Writes a name-value pair.
/// </summary>
/// <param name="name">The name of the datum. Throws if <c>null</c>.</param>
/// <param name="name">The name of the datum.</param>
/// <param name="value">The datum.</param>
/// <exception cref="ArgumentNullException">Thrown if <paramref name="name" /> is <c>null</c>.</exception>
/// <exception cref="ObjectDisposedException">Thrown if this object is disposed.</exception>
void WriteNameValue(string name, string value);

/// <summary>
/// Writes a name-collection pair.
/// </summary>
/// <param name="name">The name of the data. Throws if <c>null</c>.</param>
/// <param name="name">The name of the data.</param>
/// <param name="values">The data.</param>
/// <exception cref="ArgumentNullException">Thrown if <paramref name="name" /> is <c>null</c>.</exception>
/// <exception cref="ObjectDisposedException">Thrown if this object is disposed.</exception>
void WriteNameArray(string name, IEnumerable<string> values);

/// <summary>
/// Writes the start of an array.
/// The new object becomes the scope of all other methods until WriteObjectInArrayStart is called to start a new object in the array, or WriteArrayEnd is called.
/// The new object becomes the scope of all other methods until WriteArrayStart is called to start a new object in the array, or WriteArrayEnd is called.
/// Every call to WriteArrayStart needs to be balanced with a corresponding call to WriteArrayEnd and not WriteObjectEnd.
/// </summary>
/// <param name="name">The array name</param>
/// <exception cref="ArgumentNullException">Thrown if <paramref name="name" /> is <c>null</c>.</exception>
/// <exception cref="ObjectDisposedException">Thrown if this object is disposed.</exception>
void WriteArrayStart(string name);

/// <summary>
Expand All @@ -76,18 +101,7 @@ public interface IObjectWriter
///
/// Every call to WriteArrayStart needs to be balanced with a corresponding call to WriteArrayEnd and not WriteObjectEnd.
/// </summary>
/// <exception cref="ObjectDisposedException">Thrown if this object is disposed.</exception>
void WriteArrayEnd();

/// <summary>
/// Writes the start of a nested object in array.
///
/// This new object becomes the scope for all other method calls until either WriteObjectInArrayStart
/// is called again to start a new nested object or WriteObjectEnd is called.
///
/// Every call to WriteObjectInArrayStart must be balanced by a corresponding call to WriteObjectEnd.
/// </summary>
void WriteObjectInArrayStart();


}
}
}
160 changes: 55 additions & 105 deletions src/NuGet.Core/NuGet.Packaging/RuntimeModel/JsonObjectWriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
using System;
using System.Collections.Generic;
using Newtonsoft.Json;
using Newtonsoft.Json.Linq;

namespace NuGet.RuntimeModel
{
Expand All @@ -13,48 +12,49 @@ namespace NuGet.RuntimeModel
///
/// This is non-private only to facilitate unit testing.
/// </summary>
public sealed class JsonObjectWriter : IObjectWriter
public sealed class JsonObjectWriter : IObjectWriter, IDisposable
{
private readonly Stack<JContainer> _containers;
private JContainer _currentContainer;
private bool _isReadOnly;
private readonly JObject _root;
private readonly JsonWriter _writer;
private bool _isDisposed;

public JsonObjectWriter()
public JsonObjectWriter(JsonWriter writer)
{
_containers = new Stack<JContainer>();
_root = new JObject();
if (writer == null)
{
throw new ArgumentNullException(nameof(writer));
}

_currentContainer = _root;
_writer = writer;
}

public void WriteObjectStart(string name)
public void Dispose()
{
if (name == null)
if (!_isDisposed)
{
throw new ArgumentNullException(nameof(name));
}

ThrowIfReadOnly();
((IDisposable)_writer).Dispose();

_containers.Push(_currentContainer);
_isDisposed = true;
}
}

var newContainer = new JObject();
public void WriteObjectStart()
{
ThrowIfDisposed();

_currentContainer[name] = newContainer;
_currentContainer = newContainer;
_writer.WriteStartObject();
}

public void WriteObjectInArrayStart()
public void WriteObjectStart(string name)
{
ThrowIfReadOnly();

_containers.Push(_currentContainer);
if (name == null)
{
throw new ArgumentNullException(nameof(name));
}

var newContainer = new JObject();
ThrowIfDisposed();

_currentContainer.Add(newContainer);
_currentContainer = newContainer;
_writer.WritePropertyName(name);
_writer.WriteStartObject();
}

public void WriteArrayStart(string name)
Expand All @@ -64,38 +64,24 @@ public void WriteArrayStart(string name)
throw new ArgumentNullException(nameof(name));
}

ThrowIfReadOnly();

_containers.Push(_currentContainer);
ThrowIfDisposed();

var newContainer = new JArray();

_currentContainer[name] = newContainer;
_currentContainer = newContainer;
_writer.WritePropertyName(name);
_writer.WriteStartArray();
}

public void WriteObjectEnd()
{
ThrowIfReadOnly();

if (_currentContainer == _root)
{
throw new InvalidOperationException();
}
ThrowIfDisposed();

_currentContainer = GetPreviousContainer();
_writer.WriteEndObject();
}

public void WriteArrayEnd()
{
ThrowIfReadOnly();

if (_currentContainer == _root)
{
throw new InvalidOperationException();
}
ThrowIfDisposed();

_currentContainer = GetPreviousContainer();
_writer.WriteEndArray();
}

public void WriteNameValue(string name, int value)
Expand All @@ -105,9 +91,10 @@ public void WriteNameValue(string name, int value)
throw new ArgumentNullException(nameof(name));
}

ThrowIfReadOnly();
ThrowIfDisposed();

_currentContainer[name] = value;
_writer.WritePropertyName(name);
_writer.WriteValue(value);
}

public void WriteNameValue(string name, bool value)
Expand All @@ -117,9 +104,10 @@ public void WriteNameValue(string name, bool value)
throw new ArgumentNullException(nameof(name));
}

ThrowIfReadOnly();
ThrowIfDisposed();

_currentContainer[name] = value;
_writer.WritePropertyName(name);
_writer.WriteValue(value);
}

public void WriteNameValue(string name, string value)
Expand All @@ -129,9 +117,10 @@ public void WriteNameValue(string name, string value)
throw new ArgumentNullException(nameof(name));
}

ThrowIfReadOnly();
ThrowIfDisposed();

_currentContainer[name] = value;
_writer.WritePropertyName(name);
_writer.WriteValue(value);
}

public void WriteNameArray(string name, IEnumerable<string> values)
Expand All @@ -141,69 +130,30 @@ public void WriteNameArray(string name, IEnumerable<string> values)
throw new ArgumentNullException(nameof(name));
}

ThrowIfReadOnly();

_currentContainer[name] = new JArray(values);
}

/// <summary>
/// Gets the JSON for the object.
///
/// Once <see cref="GetJson"/> is called, no further writing is allowed.
/// </summary>
public string GetJson()
{
_isReadOnly = true;

return _root.ToString();
}

/// <summary>
/// Gets the JObject (in-memory JSON model) for the object.
///
/// Once <see cref="GetJObject"/> is called, no further writing is allowed.
/// </summary>
/// <returns></returns>
public JObject GetJObject()
{
_isReadOnly = true;

return _root;
}

/// <summary>
/// Writes the result to a <c>JsonTextWriter</c>.
///
/// Once WriteTo is called, no further writing is allowed.
/// </summary>
public void WriteTo(JsonTextWriter writer)
{
if (writer == null)
if (values == null)
{
throw new ArgumentNullException(nameof(writer));
throw new ArgumentNullException(nameof(values));
}

_isReadOnly = true;
ThrowIfDisposed();

_root.WriteTo(writer);
}
_writer.WritePropertyName(name);
_writer.WriteStartArray();

private JContainer GetPreviousContainer()
{
if (_containers.Count == 0)
foreach (string value in values)
{
return null;
_writer.WriteValue(value);
}

return _containers.Pop();
_writer.WriteEndArray();
}

private void ThrowIfReadOnly()
private void ThrowIfDisposed()
{
if (_isReadOnly)
if (_isDisposed)
{
throw new InvalidOperationException();
throw new ObjectDisposedException(nameof(JsonObjectWriter));
}
}
}
}
}
Loading