Added tests and improved allocations for HtmlEncoder#313
Added tests and improved allocations for HtmlEncoder#313rexm merged 4 commits intoHandlebars-Net:masterfrom
Conversation
That's strange, it works local :| |
|
Thanks for the additional test. It's not clear to me how the allocation change is a net improvement. |
|
Allocations are taking memory and makes the GC busy. FYI: We've done a lot of those improvements in NLog. |
This is fixed with the UTF-8 BOM on the file |
|
Yeah I get that generally speaking allocations take memory and make GC busy, but in this specific case it is not clear to me what the net improvement is. |
|
less memory usage and lower GC pressure for all renderings? |
|
Great! How much? |
|
That depends on the machine. Could try to measure (but that's more work than the fix+unit tests ;)) |
|
Can always leave the unit tests and remove the change. Generally we don't put in changes that claim to be performance improvements without instrumenting them. |
That's clear ;)
If I don't find time this week, then that's OK. I think it could do the performance instrumention with https://www.jetbrains.com/dotmemory/unit/ |
|
I tested this case: [Fact]
public void LargeEncodeTest()
{
var input = new string('0', 1_000_000_000);
// Arrange
var htmlEncoder = new HtmlEncoder();
// Act
var result = htmlEncoder.Encode(input);
}before:
after:
all run 3 times (results where significant the same), release mode. |
|
from the profiler: before:
after:
|
|
Thanks! New release probably this weekend. |
|
Great! |
roydukkey
left a comment
roydukkey
left a comment
There was a problem hiding this comment.
Please verify and document the addition of five (5) to the text length when creating the StringBuilder.
|
|
||
| private static string ReallyEncode(string text, int i) | ||
| { | ||
| var sb = new StringBuilder(text.Length + 5); |
There was a problem hiding this comment.
@304NotModified Why are you adding 5 to the length? How does this help during allocation?
| { | ||
| return ReallyEncode(text, i); | ||
| } | ||
| else |
There was a problem hiding this comment.
@304NotModified Could this else be omitted? Or, at the very least be use braces {} to match the style of the rest of the project?
| sb.Append(((int)text[i]).ToString(CultureInfo.InvariantCulture)); | ||
| sb.Append(";"); | ||
| } | ||
| else |
There was a problem hiding this comment.
@304NotModified I guess I see the braces existed like this before your edits. So maybe this would be an appropriate time to fix?
3 participants
As the HtmlEncoder is an important thing when rendering, I added some tests for the coverage and improved the allocations, no need to allocate a stringbuilder and new string if no encoding needs to be done :)