From ed3b750ad2e22d2e970a9b571e55ebefbff405e7 Mon Sep 17 00:00:00 2001 From: Yves57 Date: Sun, 25 Sep 2016 16:28:42 +0200 Subject: [PATCH] Less allocations in ViewBuffer when there is only one ViewBufferPage --- .../Internal/ViewBuffer.cs | 132 +++++++++-------- .../Internal/ViewBufferTest.cs | 136 +++++++++++++----- .../Internal/ViewBufferTextWriterTest.cs | 10 +- 3 files changed, 180 insertions(+), 98 deletions(-) diff --git a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ViewBuffer.cs b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ViewBuffer.cs index 6ad1e02cc9..601c8b5771 100644 --- a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ViewBuffer.cs +++ b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ViewBuffer.cs @@ -25,6 +25,8 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal private readonly IViewBufferScope _bufferScope; private readonly string _name; private readonly int _pageSize; + private ViewBufferPage _currentPage; // Limits allocation if the ViewBuffer has only one page (frequent case). + private List _multiplePages; // Allocated only if necessary /// /// Initializes a new instance of . @@ -47,14 +49,45 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal _bufferScope = bufferScope; _name = name; _pageSize = pageSize; - - Pages = new List(); } /// - /// Gets the backing buffer. + /// Get the count. /// - public IList Pages { get; } + public int Count + { + get + { + if (_multiplePages != null) + { + return _multiplePages.Count; + } + if (_currentPage != null) + { + return 1; + } + return 0; + } + } + + /// + /// Gets a . + /// + public ViewBufferPage this[int index] + { + get + { + if (_multiplePages != null) + { + return _multiplePages[index]; + } + if (index == 0 && _currentPage != null) + { + return _currentPage; + } + throw new IndexOutOfRangeException(); + } + } /// public IHtmlContentBuilder Append(string unencoded) @@ -89,7 +122,7 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal { return this; } - + AppendValue(new ViewBufferValue(encoded)); return this; } @@ -102,33 +135,34 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal private ViewBufferPage GetCurrentPage() { - ViewBufferPage page; - if (Pages.Count == 0) + if (_currentPage == null || _currentPage.IsFull) { - page = new ViewBufferPage(_bufferScope.GetPage(_pageSize)); - Pages.Add(page); + AddPage(new ViewBufferPage(_bufferScope.GetPage(_pageSize))); } - else + return _currentPage; + } + + private void AddPage(ViewBufferPage page) + { + if (_multiplePages != null) { - page = Pages[Pages.Count - 1]; - if (page.IsFull) - { - page = new ViewBufferPage(_bufferScope.GetPage(_pageSize)); - Pages.Add(page); - } + _multiplePages.Add(page); + } + else if (_currentPage != null) + { + _multiplePages = new List(2); + _multiplePages.Add(_currentPage); + _multiplePages.Add(page); } - return page; + _currentPage = page; } /// public IHtmlContentBuilder Clear() { - if (Pages != null) - { - Pages.Clear(); - } - + _multiplePages = null; + _currentPage = null; return this; } @@ -145,14 +179,9 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal throw new ArgumentNullException(nameof(encoder)); } - if (Pages == null) + for (var i = 0; i < Count; i++) { - return; - } - - for (var i = 0; i < Pages.Count; i++) - { - var page = Pages[i]; + var page = this[i]; for (var j = 0; j < page.Count; j++) { var value = page.Buffer[j]; @@ -192,14 +221,9 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal throw new ArgumentNullException(nameof(encoder)); } - if (Pages == null) + for (var i = 0; i < Count; i++) { - return; - } - - for (var i = 0; i < Pages.Count; i++) - { - var page = Pages[i]; + var page = this[i]; for (var j = 0; j < page.Count; j++) { var value = page.Buffer[j]; @@ -238,14 +262,9 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal throw new ArgumentNullException(nameof(destination)); } - if (Pages == null) + for (var i = 0; i < Count; i++) { - return; - } - - for (var i = 0; i < Pages.Count; i++) - { - var page = Pages[i]; + var page = this[i]; for (var j = 0; j < page.Count; j++) { var value = page.Buffer[j]; @@ -275,11 +294,6 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal throw new ArgumentNullException(nameof(destination)); } - if (Pages == null) - { - return; - } - // Perf: We have an efficient implementation when the destination is another view buffer, // we can just insert our pages as-is. var other = destination as ViewBuffer; @@ -289,9 +303,9 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal return; } - for (var i = 0; i < Pages.Count; i++) + for (var i = 0; i < Count; i++) { - var page = Pages[i]; + var page = this[i]; for (var j = 0; j < page.Count; j++) { var value = page.Buffer[j]; @@ -313,23 +327,23 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal } } - for (var i = 0; i < Pages.Count; i++) + for (var i = 0; i < Count; i++) { - var page = Pages[i]; + var page = this[i]; Array.Clear(page.Buffer, 0, page.Count); _bufferScope.ReturnSegment(page.Buffer); } - Pages.Clear(); + Clear(); } private void MoveTo(ViewBuffer destination) { - for (var i = 0; i < Pages.Count; i++) + for (var i = 0; i < Count; i++) { - var page = Pages[i]; + var page = this[i]; - var destinationPage = destination.Pages.Count == 0 ? null : destination.Pages[destination.Pages.Count - 1]; + var destinationPage = destination.Count == 0 ? null : destination[destination.Count - 1]; // If the source page is less or equal to than half full, let's copy it's content to the destination // page if possible. @@ -351,17 +365,17 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal // Now we can return the source page, and it can be reused in the scope of this request. Array.Clear(page.Buffer, 0, page.Count); _bufferScope.ReturnSegment(page.Buffer); - + } else { // Otherwise, let's just add the source page to the other buffer. - destination.Pages.Add(page); + destination.AddPage(page); } } - Pages.Clear(); + Clear(); } private class EncodingWrapper : IHtmlContent diff --git a/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/ViewBufferTest.cs b/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/ViewBufferTest.cs index 1d9da0fc0b..eeb189dafc 100644 --- a/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/ViewBufferTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/ViewBufferTest.cs @@ -25,7 +25,8 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal buffer.Append("Hello world"); // Assert - var page = Assert.Single(buffer.Pages); + Assert.Equal(1, buffer.Count); + var page = buffer[0]; Assert.Equal(1, page.Count); Assert.IsAssignableFrom(page.Buffer[0].Value); } @@ -41,7 +42,8 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal buffer.AppendHtml(content); // Assert - var page = Assert.Single(buffer.Pages); + Assert.Equal(1, buffer.Count); + var page = buffer[0]; Assert.Equal(1, page.Count); Assert.Same(content, page.Buffer[0].Value); } @@ -57,13 +59,32 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal buffer.AppendHtml(value); // Assert - var page = Assert.Single(buffer.Pages); + Assert.Equal(1, buffer.Count); + var page = buffer[0]; Assert.Equal(1, page.Count); Assert.Equal("Hello world", Assert.IsType(page.Buffer[0].Value)); } [Fact] - public void Append_CreatesNewPages_WhenCurrentPageIsFull() + public void Append_CreatesOnePage() + { + // Arrange + var buffer = new ViewBuffer(new TestViewBufferScope(), "some-name", pageSize: 32); + var expected = Enumerable.Range(0, 32).Select(i => i.ToString()); + + // Act + foreach (var item in expected) + { + buffer.AppendHtml(item); + } + + // Assert + Assert.Equal(1, buffer.Count); + Assert.Equal(expected, buffer[0].Buffer.Select(v => v.Value)); + } + + [Fact] + public void Append_CreatesTwoPages() { // Arrange var buffer = new ViewBuffer(new TestViewBufferScope(), "some-name", pageSize: 32); @@ -78,8 +99,8 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal buffer.AppendHtml("world"); // Assert - Assert.Equal(2, buffer.Pages.Count); - Assert.Collection(buffer.Pages, + Assert.Equal(2, buffer.Count); + Assert.Collection(new[] { buffer[0], buffer[1] }, page => Assert.Equal(expected, page.Buffer.Select(v => v.Value)), page => { @@ -89,9 +110,43 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal }); } + [Fact] + public void Append_CreatesManyPages() + { + // Arrange + var buffer = new ViewBuffer(new TestViewBufferScope(), "some-name", pageSize: 32); + var expected0 = Enumerable.Range(0, 32).Select(i => i.ToString()); + var expected1 = Enumerable.Range(32, 32).Select(i => i.ToString()); + + // Act + foreach (var item in expected0) + { + buffer.AppendHtml(item); + } + foreach (var item in expected1) + { + buffer.AppendHtml(item); + } + buffer.AppendHtml("Hello"); + buffer.AppendHtml("world"); + + // Assert + Assert.Equal(3, buffer.Count); + Assert.Collection(new[] { buffer[0], buffer[1], buffer[2] }, + page => Assert.Equal(expected0, page.Buffer.Select(v => v.Value)), + page => Assert.Equal(expected1, page.Buffer.Select(v => v.Value)), + page => + { + var array = page.Buffer; + Assert.Equal("Hello", array[0].Value); + Assert.Equal("world", array[1].Value); + }); + } + [Theory] - [InlineData(1)] - [InlineData(35)] + [InlineData(1)] // Create one page before clear + [InlineData(35)] // Create two pages before clear + [InlineData(65)] // Create many pages before clear public void Clear_ResetsBackingBufferAndIndex(int valuesToWrite) { // Arrange @@ -106,7 +161,8 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal buffer.AppendHtml("world"); // Assert - var page = Assert.Single(buffer.Pages); + Assert.Equal(1, buffer.Count); + var page = buffer[0]; Assert.Equal(1, page.Count); Assert.Equal("world", page.Buffer[0].Value); } @@ -211,7 +267,7 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal buffer.CopyTo(destination); // Assert - Assert.Same(nested, buffer.Pages[0].Buffer[0].Value); + Assert.Same(nested, buffer[0].Buffer[0].Value); Assert.Equal("Hello", Assert.IsType(nestedItems[0]).Value); Assert.Equal("Hello", Assert.IsType(destinationItems[0]).Value); } @@ -235,7 +291,7 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal // Assert Assert.Empty(nestedItems); - Assert.Empty(buffer.Pages); + Assert.Equal(0, buffer.Count); Assert.Equal("Hello", Assert.IsType(destinationItems[0]).Value); } @@ -250,14 +306,15 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal other.AppendHtml("Hi"); - var page = other.Pages[0]; + var page = other[0]; // Act other.MoveTo(original); // Assert - Assert.Empty(other.Pages); // Page was taken - Assert.Same(page, Assert.Single(original.Pages)); + Assert.Equal(0, other.Count); // Page was taken + Assert.Equal(1, original.Count); + Assert.Same(page, original[0]); } [Fact] @@ -276,15 +333,15 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal other.AppendHtml("Hi"); - var page = other.Pages[0]; + var page = other[0]; // Act other.MoveTo(original); // Assert - Assert.Empty(other.Pages); // Page was taken - Assert.Equal(2, original.Pages.Count); - Assert.Same(page, original.Pages[1]); + Assert.Equal(0, other.Count); // Page was taken + Assert.Equal(2, original.Count); + Assert.Same(page, original[1]); } [Fact] @@ -308,15 +365,15 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal other.AppendHtml($"other-{i}"); } - var page = other.Pages[0]; + var page = other[0]; // Act other.MoveTo(original); // Assert - Assert.Empty(other.Pages); // Page was taken - Assert.Equal(2, original.Pages.Count); - Assert.Same(page, original.Pages[1]); + Assert.Equal(0, other.Count); // Page was taken + Assert.Equal(2, original.Count); + Assert.Same(page, original[1]); } [Fact] @@ -339,17 +396,18 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal other.AppendHtml($"other-{i}"); } - var page = other.Pages[0]; + var page = other[0]; // Act other.MoveTo(original); // Assert - Assert.Empty(other.Pages); // Other is cleared + Assert.Equal(0, other.Count); // Other is cleared Assert.Contains(page.Buffer, scope.ReturnedBuffers); // Buffer was returned + Assert.Equal(1, original.Count); Assert.Collection( - Assert.Single(original.Pages).Buffer, + original[0].Buffer, item => Assert.Equal("original-0", item.Value), item => Assert.Equal("original-1", item.Value), item => Assert.Equal("other-0", item.Value), @@ -376,24 +434,24 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal other.AppendHtml($"other-{i}"); } - var page = other.Pages[0]; + var page = other[0]; other.MoveTo(original); // Act original.AppendHtml("after-merge"); // Assert - Assert.Empty(other.Pages); // Other is cleared + Assert.Equal(0, other.Count); // Other is cleared - Assert.Equal(2, original.Pages.Count); + Assert.Equal(2, original.Count); Assert.Collection( - original.Pages[0].Buffer, + original[0].Buffer, item => Assert.Equal("original-0", item.Value), item => Assert.Equal("original-1", item.Value), item => Assert.Equal("original-2", item.Value), item => Assert.Null(item.Value)); Assert.Collection( - original.Pages[1].Buffer, + original[1].Buffer, item => Assert.Equal("other-0", item.Value), item => Assert.Equal("other-1", item.Value), item => Assert.Equal("other-2", item.Value), @@ -419,35 +477,39 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal other.AppendHtml($"other-{i}"); } - var pages = new List(other.Pages); + var pages = new List(); + for (var i = 0; i < other.Count; i++) + { + pages.Add(other[i]); + } // Act other.MoveTo(original); // Assert - Assert.Empty(other.Pages); // Other is cleared + Assert.Equal(0, other.Count); // Other is cleared - Assert.Equal(4, original.Pages.Count); + Assert.Equal(4, original.Count); Assert.Collection( - original.Pages[0].Buffer, + original[0].Buffer, item => Assert.Equal("original-0", item.Value), item => Assert.Equal("original-1", item.Value), item => Assert.Null(item.Value), item => Assert.Null(item.Value)); Assert.Collection( - original.Pages[1].Buffer, + original[1].Buffer, item => Assert.Equal("other-0", item.Value), item => Assert.Equal("other-1", item.Value), item => Assert.Equal("other-2", item.Value), item => Assert.Equal("other-3", item.Value)); Assert.Collection( - original.Pages[2].Buffer, + original[2].Buffer, item => Assert.Equal("other-4", item.Value), item => Assert.Equal("other-5", item.Value), item => Assert.Equal("other-6", item.Value), item => Assert.Equal("other-7", item.Value)); Assert.Collection( - original.Pages[3].Buffer, + original[3].Buffer, item => Assert.Equal("other-8", item.Value), item => Assert.Null(item.Value), item => Assert.Null(item.Value), diff --git a/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/ViewBufferTextWriterTest.cs b/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/ViewBufferTextWriterTest.cs index 0e2d2a7df6..d5951053af 100644 --- a/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/ViewBufferTextWriterTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/ViewBufferTextWriterTest.cs @@ -60,7 +60,7 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal writer.Write(2.718m); // Assert - Assert.Empty(buffer.Pages); + Assert.Equal(0, buffer.Count); foreach (var item in expected) { inner.Verify(v => v.Write(item), Times.Once()); @@ -220,7 +220,13 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal private static object[] GetValues(ViewBuffer buffer) { - return buffer.Pages + var pages = new List(); + for (var i = 0; i < buffer.Count; i++) + { + pages.Add(buffer[i]); + } + + return pages .SelectMany(c => c.Buffer) .Select(d => d.Value) .TakeWhile(d => d != null)