From 43071319aa0c347a1d2dfcc8f14042807ddebe51 Mon Sep 17 00:00:00 2001 From: Pranav K Date: Fri, 2 Sep 2016 16:33:46 -0700 Subject: [PATCH] Buffer rendered CacheTagHelper content strings to a custom TextWriter Fixes #4893 --- .../CacheTagHelper.cs | 57 ++++- .../Internal/ArrayPoolBufferSource.cs | 21 ++ .../Internal/CharArrayBufferSource.cs | 17 ++ .../Internal/ICharBufferSource.cs | 12 + .../Internal/PagedBufferedTextWriter.cs | 150 ++----------- .../Internal/PagedCharBuffer.cs | 159 +++++++++++++ .../Internal/PagedBufferedTextWriterTest.cs | 2 +- .../Internal/PagedCharBufferTest.cs | 210 ++++++++++++++++++ 8 files changed, 483 insertions(+), 145 deletions(-) create mode 100644 src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ArrayPoolBufferSource.cs create mode 100644 src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/CharArrayBufferSource.cs create mode 100644 src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ICharBufferSource.cs create mode 100644 src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/PagedCharBuffer.cs create mode 100644 test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/PagedCharBufferTest.cs diff --git a/src/Microsoft.AspNetCore.Mvc.TagHelpers/CacheTagHelper.cs b/src/Microsoft.AspNetCore.Mvc.TagHelpers/CacheTagHelper.cs index 3f23690c25..6d484b495e 100644 --- a/src/Microsoft.AspNetCore.Mvc.TagHelpers/CacheTagHelper.cs +++ b/src/Microsoft.AspNetCore.Mvc.TagHelpers/CacheTagHelper.cs @@ -2,6 +2,7 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; +using System.Collections.Generic; using System.IO; using System.Text; using System.Text.Encodings.Web; @@ -9,6 +10,7 @@ using System.Threading; using System.Threading.Tasks; using Microsoft.AspNetCore.Html; using Microsoft.AspNetCore.Mvc.TagHelpers.Cache; +using Microsoft.AspNetCore.Mvc.ViewFeatures.Internal; using Microsoft.AspNetCore.Razor.TagHelpers; using Microsoft.Extensions.Caching.Memory; using Microsoft.Extensions.Primitives; @@ -174,29 +176,62 @@ namespace Microsoft.AspNetCore.Mvc.TagHelpers { var content = await output.GetChildContentAsync(); - var stringBuilder = new StringBuilder(); - using (var writer = new StringWriter(stringBuilder)) + using (var writer = new CharBufferTextWriter()) { content.WriteTo(writer, HtmlEncoder); + return new CharBufferHtmlContent(writer.Buffer); } - - return new StringBuilderHtmlContent(stringBuilder); } - private class StringBuilderHtmlContent : IHtmlContent + private class CharBufferTextWriter : TextWriter { - private readonly StringBuilder _builder; - - public StringBuilderHtmlContent(StringBuilder builder) + public CharBufferTextWriter() { - _builder = builder; + Buffer = new PagedCharBuffer(CharArrayBufferSource.Instance); + } + + public override Encoding Encoding => Null.Encoding; + + public PagedCharBuffer Buffer { get; } + + public override void Write(char value) + { + Buffer.Append(value); + } + + public override void Write(char[] buffer, int index, int count) + { + Buffer.Append(buffer, index, count); + } + + public override void Write(string value) + { + Buffer.Append(value); + } + } + + private class CharBufferHtmlContent : IHtmlContent + { + private readonly PagedCharBuffer _buffer; + + public CharBufferHtmlContent(PagedCharBuffer buffer) + { + _buffer = buffer; } public void WriteTo(TextWriter writer, HtmlEncoder encoder) { - for (var i = 0; i < _builder.Length; i++) + var length = _buffer.Length; + if (length == 0) { - writer.Write(_builder[i]); + return; + } + + for (var i = 0; i < _buffer.Pages.Count; i++) + { + var pageLength = Math.Min(length, PagedCharBuffer.PageSize); + writer.Write(_buffer.Pages[i], 0, pageLength); + length -= pageLength; } } } diff --git a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ArrayPoolBufferSource.cs b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ArrayPoolBufferSource.cs new file mode 100644 index 0000000000..b21e645cbc --- /dev/null +++ b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ArrayPoolBufferSource.cs @@ -0,0 +1,21 @@ +// 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.Buffers; + +namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal +{ + public class ArrayPoolBufferSource : ICharBufferSource + { + private readonly ArrayPool _pool; + + public ArrayPoolBufferSource(ArrayPool pool) + { + _pool = pool; + } + + public char[] Rent(int bufferSize) => _pool.Rent(bufferSize); + + public void Return(char[] buffer) => _pool.Return(buffer); + } +} diff --git a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/CharArrayBufferSource.cs b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/CharArrayBufferSource.cs new file mode 100644 index 0000000000..50d526172d --- /dev/null +++ b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/CharArrayBufferSource.cs @@ -0,0 +1,17 @@ +// 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. + +namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal +{ + public class CharArrayBufferSource : ICharBufferSource + { + public static readonly CharArrayBufferSource Instance = new CharArrayBufferSource(); + + public char[] Rent(int bufferSize) => new char[bufferSize]; + + public void Return(char[] buffer) + { + // Do nothing. + } + } +} diff --git a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ICharBufferSource.cs b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ICharBufferSource.cs new file mode 100644 index 0000000000..99e23c1997 --- /dev/null +++ b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ICharBufferSource.cs @@ -0,0 +1,12 @@ +// 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. + +namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal +{ + public interface ICharBufferSource + { + char[] Rent(int bufferSize); + + void Return(char[] buffer); + } +} diff --git a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/PagedBufferedTextWriter.cs b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/PagedBufferedTextWriter.cs index c0eaac468a..b4f2f575c2 100644 --- a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/PagedBufferedTextWriter.cs +++ b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/PagedBufferedTextWriter.cs @@ -3,8 +3,6 @@ using System; using System.Buffers; -using System.Collections.Generic; -using System.Diagnostics; using System.IO; using System.Text; using System.Threading.Tasks; @@ -13,20 +11,13 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal { public class PagedBufferedTextWriter : TextWriter { - public const int PageSize = 1024; - private readonly TextWriter _inner; - private readonly List _pages; - private readonly ArrayPool _pool; - - private int _currentPage; - private int _currentIndex; // The next 'free' character + private readonly PagedCharBuffer _charBuffer; public PagedBufferedTextWriter(ArrayPool pool, TextWriter inner) { - _pool = pool; + _charBuffer = new PagedCharBuffer(new ArrayPoolBufferSource(pool)); _inner = inner; - _pages = new List(); } public override Encoding Encoding => _inner.Encoding; @@ -38,47 +29,31 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal public override async Task FlushAsync() { - if (_pages.Count == 0) + var length = _charBuffer.Length; + if (length == 0) { return; } - for (var i = 0; i <= _currentPage; i++) + var pages = _charBuffer.Pages; + for (var i = 0; i < pages.Count; i++) { - var page = _pages[i]; + var page = pages[i]; - var count = i == _currentPage ? _currentIndex : page.Length; - if (count > 0) + var pageLength = Math.Min(length, PagedCharBuffer.PageSize); + if (pageLength != 0) { - await _inner.WriteAsync(page, 0, count); + await _inner.WriteAsync(page, 0, pageLength); } + length -= pageLength; } - // Return all but one of the pages. This way if someone writes a large chunk of - // content, we can return those buffers and avoid holding them for the whole - // page's lifetime. - for (var i = _pages.Count - 1; i > 0; i--) - { - var page = _pages[i]; - - try - { - _pages.RemoveAt(i); - } - finally - { - _pool.Return(page); - } - } - - _currentPage = 0; - _currentIndex = 0; + _charBuffer.Clear(); } public override void Write(char value) { - var page = GetCurrentPage(); - page[_currentIndex++] = value; + _charBuffer.Append(value); } public override void Write(char[] buffer) @@ -88,7 +63,7 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal return; } - Write(buffer, 0, buffer.Length); + _charBuffer.Append(buffer, 0, buffer.Length); } public override void Write(char[] buffer, int index, int count) @@ -98,24 +73,7 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal throw new ArgumentNullException(nameof(buffer)); } - while (count > 0) - { - var page = GetCurrentPage(); - var copyLength = Math.Min(count, page.Length - _currentIndex); - Debug.Assert(copyLength > 0); - - Array.Copy( - buffer, - index, - page, - _currentIndex, - copyLength); - - _currentIndex += copyLength; - index += copyLength; - - count -= copyLength; - } + _charBuffer.Append(buffer, index, count); } public override void Write(string value) @@ -125,26 +83,7 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal return; } - var index = 0; - var count = value.Length; - - while (count > 0) - { - var page = GetCurrentPage(); - var copyLength = Math.Min(count, page.Length - _currentIndex); - Debug.Assert(copyLength > 0); - - value.CopyTo( - index, - page, - _currentIndex, - copyLength); - - _currentIndex += copyLength; - index += copyLength; - - count -= copyLength; - } + _charBuffer.Append(value); } public override Task WriteAsync(char value) @@ -162,65 +101,10 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal return _inner.WriteAsync(value); } - private char[] GetCurrentPage() - { - char[] page = null; - if (_pages.Count == 0) - { - Debug.Assert(_currentPage == 0); - Debug.Assert(_currentIndex == 0); - - try - { - page = _pool.Rent(PageSize); - _pages.Add(page); - } - catch when (page != null) - { - _pool.Return(page); - throw; - } - - return page; - } - - Debug.Assert(_pages.Count > _currentPage); - page = _pages[_currentPage]; - - if (_currentIndex == page.Length) - { - // Current page is full. - _currentPage++; - _currentIndex = 0; - - if (_pages.Count == _currentPage) - { - try - { - page = _pool.Rent(PageSize); - _pages.Add(page); - } - catch when (page != null) - { - _pool.Return(page); - throw; - } - } - } - - return page; - } - protected override void Dispose(bool disposing) { base.Dispose(disposing); - - for (var i = 0; i < _pages.Count; i++) - { - _pool.Return(_pages[i]); - } - - _pages.Clear(); + _charBuffer.Dispose(); } } } diff --git a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/PagedCharBuffer.cs b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/PagedCharBuffer.cs new file mode 100644 index 0000000000..bb6509790b --- /dev/null +++ b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/PagedCharBuffer.cs @@ -0,0 +1,159 @@ +// 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; +using System.Diagnostics; + +namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal +{ + public class PagedCharBuffer : IDisposable + { + public const int PageSize = 1024; + private int _charIndex; + + public PagedCharBuffer(ICharBufferSource bufferSource) + { + BufferSource = bufferSource; + } + + public ICharBufferSource BufferSource { get; } + + public IList Pages { get; } = new List(); + + public int Length + { + get + { + var length = _charIndex; + if (Pages.Count > 1) + { + length += PageSize * (Pages.Count - 1); + } + + return length; + } + } + + private char[] CurrentPage { get; set; } + + public void Append(char value) + { + var page = GetCurrentPage(); + page[_charIndex++] = value; + } + + public void Append(string value) + { + if (value == null) + { + return; + } + + var index = 0; + var count = value.Length; + + while (count > 0) + { + var page = GetCurrentPage(); + var copyLength = Math.Min(count, page.Length - _charIndex); + Debug.Assert(copyLength > 0); + + value.CopyTo( + index, + page, + _charIndex, + copyLength); + + _charIndex += copyLength; + index += copyLength; + + count -= copyLength; + } + } + + public void Append(char[] buffer, int index, int count) + { + while (count > 0) + { + var page = GetCurrentPage(); + var copyLength = Math.Min(count, page.Length - _charIndex); + Debug.Assert(copyLength > 0); + + Array.Copy( + buffer, + index, + page, + _charIndex, + copyLength); + + _charIndex += copyLength; + index += copyLength; + count -= copyLength; + } + } + + /// + /// Return all but one of the pages to the . + /// This way if someone writes a large chunk of content, we can return those buffers and avoid holding them + /// for extended durations. + /// + public void Clear() + { + for (var i = Pages.Count - 1; i > 0; i--) + { + var page = Pages[i]; + + try + { + Pages.RemoveAt(i); + } + finally + { + BufferSource.Return(page); + } + } + + _charIndex = 0; + } + + private char[] GetCurrentPage() + { + if (CurrentPage == null || + _charIndex == CurrentPage.Length) + { + CurrentPage = NewPage(); + _charIndex = 0; + } + + return CurrentPage; + } + + private char[] NewPage() + { + char[] page = null; + try + { + page = BufferSource.Rent(PageSize); + Pages.Add(page); + } + catch when (page != null) + { + BufferSource.Return(page); + throw; + } + + return page; + } + + public void Dispose() + { + for (var i = 0; i < Pages.Count; i++) + { + BufferSource.Return(Pages[i]); + } + + Pages.Clear(); + } + } +} diff --git a/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/PagedBufferedTextWriterTest.cs b/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/PagedBufferedTextWriterTest.cs index f3681d7714..91954fb81b 100644 --- a/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/PagedBufferedTextWriterTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/PagedBufferedTextWriterTest.cs @@ -16,7 +16,7 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal static PagedBufferedTextWriterTest() { - Content = new char[4 * PagedBufferedTextWriter.PageSize]; + Content = new char[4 * PagedCharBuffer.PageSize]; for (var i = 0; i < Content.Length; i++) { Content[i] = (char)((i % 26) + 'A'); diff --git a/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/PagedCharBufferTest.cs b/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/PagedCharBufferTest.cs new file mode 100644 index 0000000000..f8b9f7c7cc --- /dev/null +++ b/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/PagedCharBufferTest.cs @@ -0,0 +1,210 @@ +// 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.Linq; +using Moq; +using Xunit; + +namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal +{ + public class PagedCharBufferTest + { + [Fact] + public void AppendWithChar_AddsCharacterToPage() + { + // Arrange + var charToAppend = 't'; + var buffer = new PagedCharBuffer(new CharArrayBufferSource()); + + // Act + buffer.Append(charToAppend); + + // Assert + var page = Assert.Single(buffer.Pages); + Assert.Equal(1, buffer.Length); + Assert.Equal(charToAppend, page[buffer.Length - 1]); + } + + [Fact] + public void AppendWithChar_AddsCharacterToTheLastPage() + { + // Arrange + var stringToAppend = new string('a', PagedCharBuffer.PageSize); + var charToAppend = 't'; + var buffer = new PagedCharBuffer(new CharArrayBufferSource()); + + // Act + buffer.Append(stringToAppend); + buffer.Append(charToAppend); + + // Assert + Assert.Collection(buffer.Pages, + page => Assert.Equal(stringToAppend.ToCharArray(), page), + page => Assert.Equal(charToAppend, page[0])); + Assert.Equal(1 + PagedCharBuffer.PageSize, buffer.Length); + } + + [Fact] + public void AppendWithChar_AppendsToTheCurrentPageIfItIsNotFull() + { + // Arrange + var stringToAppend = "abc"; + var charToAppend = 't'; + var buffer = new PagedCharBuffer(new CharArrayBufferSource()); + + // Act + buffer.Append(stringToAppend); + buffer.Append(charToAppend); + + // Assert + var page = Assert.Single(buffer.Pages); + Assert.Equal(new[] { 'a', 'b', 'c', 't' }, page.Take(4)); + Assert.Equal(4, buffer.Length); + } + + [Fact] + public void AppendWithString_AppendsToPage() + { + // Arrange + var stringToAppend = "abc"; + var buffer = new PagedCharBuffer(new CharArrayBufferSource()); + + // Act + buffer.Append(stringToAppend); + + // Assert + Assert.Equal(3, buffer.Length); + var page = Assert.Single(buffer.Pages); + Assert.Equal(new[] { 'a', 'b', 'c' }, page.Take(buffer.Length)); + } + + [Fact] + public void AppendWithString_AcrossMultiplePages() + { + // Arrange + var length = 2 * PagedCharBuffer.PageSize + 1; + var expected = Enumerable.Repeat('d', PagedCharBuffer.PageSize); + var stringToAppend = new string('d', length); + var buffer = new PagedCharBuffer(new CharArrayBufferSource()); + + // Act + buffer.Append(stringToAppend); + + // Assert + Assert.Equal(length, buffer.Length); + Assert.Collection( + buffer.Pages, + page => Assert.Equal(expected, page), + page => Assert.Equal(expected, page), + page => Assert.Equal('d', page[0])); + } + + [Fact] + public void AppendWithString_AppendsToTheCurrentPageIfItIsNotEmpty() + { + // Arrange + var string1 = "abc"; + var string2 = "def"; + var buffer = new PagedCharBuffer(new CharArrayBufferSource()); + + // Act + buffer.Append(string1); + buffer.Append(string2); + + // Assert + Assert.Equal(6, buffer.Length); + var page = Assert.Single(buffer.Pages); + Assert.Equal(new[] { 'a', 'b', 'c', 'd', 'e', 'f' }, page.Take(buffer.Length)); + } + + [Fact] + public void AppendWithCharArray_AppendsToPage() + { + // Arrange + var charsToAppend = new[] { 'a', 'b', 'c', 'd' }; + var buffer = new PagedCharBuffer(new CharArrayBufferSource()); + + // Act + buffer.Append(charsToAppend, 1, 3); + + // Assert + Assert.Equal(3, buffer.Length); + var page = Assert.Single(buffer.Pages); + Assert.Equal(new[] { 'b', 'c', 'd' }, page.Take(buffer.Length)); + } + + [Fact] + public void AppendWithCharArray_AppendsToMultiplePages() + { + // Arrange + var ch = 'e'; + var length = PagedCharBuffer.PageSize * 2 + 3; + var charsToAppend = Enumerable.Repeat(ch, 2 * length).ToArray(); + var expected = Enumerable.Repeat(ch, PagedCharBuffer.PageSize); + var buffer = new PagedCharBuffer(new CharArrayBufferSource()); + + // Act + buffer.Append(charsToAppend, 0, length); + + // Assert + Assert.Equal(length, buffer.Length); + Assert.Collection(buffer.Pages, + page => Assert.Equal(expected, page), + page => Assert.Equal(expected, page), + page => + { + Assert.Equal(ch, page[0]); + Assert.Equal(ch, page[1]); + Assert.Equal(ch, page[2]); + }); + } + + [Fact] + public void AppendWithCharArray_AppendsToCurrentPage() + { + // Arrange + var arrayToAppend = new[] { 'c', 'd', 'e' }; + var buffer = new PagedCharBuffer(new CharArrayBufferSource()); + + // Act + buffer.Append("Ab"); + buffer.Append(arrayToAppend, 0, arrayToAppend.Length); + + // Assert + Assert.Equal(5, buffer.Length); + var page = Assert.Single(buffer.Pages); + Assert.Equal(new[] { 'A', 'b', 'c', 'd', 'e' }, page.Take(buffer.Length)); + } + + [Fact] + public void Clear_WorksIfBufferHasNotBeenWrittenTo() + { + // Arrange + var buffer = new PagedCharBuffer(new CharArrayBufferSource()); + + // Act + buffer.Clear(); + + // Assert + Assert.Equal(0, buffer.Length); + } + + [Fact] + public void Clear_ReturnsPagesToBufferSource() + { + // Arrange + var bufferSource = new Mock(); + bufferSource.Setup(s => s.Rent(PagedCharBuffer.PageSize)) + .Returns(new char[PagedCharBuffer.PageSize]); + var buffer = new PagedCharBuffer(bufferSource.Object); + + // Act + buffer.Append(new string('a', PagedCharBuffer.PageSize * 3 + 4)); + buffer.Clear(); + + // Assert + Assert.Equal(0, buffer.Length); + bufferSource.Verify(s => s.Return(It.IsAny()), Times.Exactly(3)); + } + } +}