-
Notifications
You must be signed in to change notification settings - Fork 410
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.
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
HTML rendering memory allocations optimisation #3800
HTML rendering memory allocations optimisation #3800
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just out of curiosity: have you measured the overall effect of this pr on memory consumption?
elements.forEach { | ||
buildContentNode(it, pageContext, sourceSet) | ||
} | ||
}.stripDiv() | ||
sourceSet to createHTML(prettyPrint = false).prepareForTemplates() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you considered an optimal capacity?
In this example, It always takes more than the default 16
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was thinking about it, but failed to think of a good default... Will it be 128 or 1024? It's hard to calculate which value will be best.
But I agree, that probably it would be nice to initialise it with bigger value. Do you have anything in mind?
I can try to check different values if needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have anything in mind?
I think you can find it out on stdlib.
E.g. for 128 vs 1024 - choose one with the minimal memory allocated in a profiler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried several values (default, 128, 256, 512, 1024), the difference in memory allocations is ±3% from best to worst. With 1024 it started to behave even worse, though the diff is minimal and not always the same.
I've decided to stick with 256.
@@ -344,14 +347,14 @@ public open class HtmlRenderer( | |||
val distinct = | |||
groupDivergentInstancesWithSourceSet(it.value, it.key, pageContext, | |||
beforeTransformer = { instance, _, sourceSet -> | |||
createHTML(prettyPrint = false).prepareForTemplates().div { | |||
createSmallHTML(prettyPrint = false).prepareForTemplates().div { | |||
instance.before?.let { before -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can extract instance.before?.let
from createHTML
to avoid the redundant invocation of createHTML
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will take a look, thanks!
Yes! I've rechecked once more to get numbers. Tested with stdlib 1.5, newer versions will probably have even bigger differences.
I can do the same with some other project of your choice if you want to validate if it affects anything else |
7e8d6b8
to
fe1fa45
Compare
Overall the main idea of the changes is to reduce usage of
createHTML
which by default createStringBuilder
of 32KB size and in most cases it needs much less, specifically in source-set-dependent content and during resource links embedding.Those optimisations were found during investigation of stdlib docs build (internal discussion)