Avoid large objects allocations and reuse everthing #532
Conversation
| return string.Format( | ||
| "ReactDOM.hydrate({0}, document.getElementById({1}))", | ||
| base.GetComponentInitialiser(), | ||
| JsonConvert.SerializeObject(ContainerId, _configuration.JsonSerializerSettings) // SerializeObject accepts null settings |
There was a problem hiding this comment.
Huh. This change is surprising to me but looks correct... not sure why SerializeObject was called here before.
There was a problem hiding this comment.
I think it was to ensure that characters are escaped as appropriate (eg. if your container ID has special characters in it). However, it's likely that most users are using the standard auto-generated container IDs, so this change is probably safe.
| component.Setup(x => x.RenderHtml(false, false, null)).Returns("HTML"); | ||
| component.Setup(x => x.RenderJavaScript()).Returns("JS"); | ||
|
|
||
| component.Setup(x => x.RenderHtml(It.IsAny<TextWriter>(), false, false, null)) |
There was a problem hiding this comment.
Thanks for updating the tests. Can you add back a test for RenderHtml that doesn’t use a TextWriter to show that the existing method still works?
Edit: After working on this locally, I discovered that there are already tests that ensure RenderHtml still works as expected. The helper extensions now only use the TextWriter version, so this isn't an issue.
| component.Setup(x => x.RenderHtml(It.IsAny<TextWriter>(), false, false, null)) | ||
| .Callback((TextWriter writer, bool renderContainerOnly, bool renderServerOnly, Action<Exception, string, string> exceptionHandler) => writer.Write("HTML")).Verifiable(); | ||
|
|
||
| component.Setup(x => x.RenderJavaScript(It.IsAny<TextWriter>())).Callback((TextWriter writer) => writer.Write("JS")).Verifiable(); |
There was a problem hiding this comment.
It lets you call component.verify() without arguments and will throw if any of the .Verifiable() mock configurations were not called. It is not needed if explicit verifications are used like they are in this test.
|
Ok! I got around to testing this and it looks good. However, I'd like to get some more concrete benchmarks documented before merging this in. Here the tool I put together, based on BenchmarkDotNet... feel free to suggest a better code sample I can use to show the benefits of this change. I'll do some more testing and see what I can find out. My goal is to add this benchmark tool to this project so we can find other improvements in the render pipeline. |
|
Before this change: BenchmarkDotNet=v0.10.14, OS=Windows 10.0.16299.371 (1709/FallCreatorsUpdate/Redstone3)
Intel Core i5-4570 CPU 3.20GHz (Haswell), 1 CPU, 4 logical and 4 physical cores
Frequency=3117782 Hz, Resolution=320.7408 ns, Timer=TSC
[Host] : .NET Framework 4.7 (CLR 4.0.30319.42000), 64bit RyuJIT-v4.7.2633.0
DefaultJob : .NET Framework 4.7 (CLR 4.0.30319.42000), 64bit RyuJIT-v4.7.2633.0
After: BenchmarkDotNet=v0.10.14, OS=Windows 10.0.16299.371 (1709/FallCreatorsUpdate/Redstone3)
Intel Core i5-4570 CPU 3.20GHz (Haswell), 1 CPU, 4 logical and 4 physical cores
Frequency=3117782 Hz, Resolution=320.7408 ns, Timer=TSC
[Host] : .NET Framework 4.7 (CLR 4.0.30319.42000), 64bit RyuJIT-v4.7.2633.0
DefaultJob : .NET Framework 4.7 (CLR 4.0.30319.42000), 64bit RyuJIT-v4.7.2633.0
|
|
@DaniilSokolyuk this has shipped in 3.4! |
|
@dustinsoftware Thank you! 👍 |
Main part of this PR
I propose to mark the old methods that do not take the TextWriter as obsolete, to avoid errors if it is overridden
@dustinsoftware @Daniel15