-
Notifications
You must be signed in to change notification settings - Fork 133
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
Question: why is there exception handling in visitTag? (it slows down compile times considerably) #204
Comments
On YouTrack (see the link in the issue) we read the following comment from Dmitry Petrov:
This refers to: https://github.com/Kotlin/kotlinx.html/blob/f55bf940745e6c6166bd39d4e0c213bb44035c3b/src/commonMain/kotlin/visit.kt I wonder what would happen without allowing tags to override |
So I made a small change to inline fun <T : Tag> T.visitTag(block: T.() -> Unit) {
consumer.onTagStart(this)
this.block()
// removed the exception handling, hoping to speed up compilation of our HTML DSL code
consumer.onTagEnd(this)
} And the from-clean compile time of our entire project (containing a lot of non-Kotlin-related tasks) went down from 5min3sec to 2min46sec. I tried to run the some HTML DSL templating code with a |
I change the title, but I still believe that in it's current shape this library should warn for slow compile times (somewhere in the README). It is slow to a point that I would not recommend someone to use this library for a project with lots of templates (like the project we have with 150+ templates (pages) at a combined 30kLOC). Otherwise I much prefer this eDSL over what I get with template engines! |
Another issue on YouTrack that is related to this: https://youtrack.jetbrains.com/issue/KT-51445/Nested-inlining-of-kotlinx.html.visitTag-produces-unnecessarily-complex-bytecode |
We're considering to fork the project for the time being, having only one difference (the hack described above), only publishing one package (the EDIT: We did, see the very minimal change here: Stager-Software@a719b02 The next comment links to the benchmark results of this change among others. |
I've used the minimal example from @spand to create a little benchmark tool in order to get to the bottom of what slows down/ speeds up kotlinx-html compilation. Find it here: https://github.com/cies/SlowKotlinxHtml The results are interesting, showing various improvements tweaking different parameters. We've also published a hacked version of kotlinx-html-jvm with the exception handling removed. As seen in the benchmarks this improves compile speeds by about 38%. Reducing the nesting of the eDSL to max 4 levels deep improved it by about 6%. Using K2 and adding some compiler flags improved it by 20%. Obviously this is on an unrealistically simple example code base, so this may not translate to your project. |
Thats pretty nice @cies . Hopefully we can get someone to look at the issue and get a faster 0.9 out of the door |
Thanks for chiming in @spand , and thanks for your benchmarks code I thankfully made use of. I think a large part of the compilation slowness (that's left after tuning the parameters and hacking out the exception handling) comes from the fact that we use "deeply nested Kotlin eDSL syntax", and the eDSL syntax being slow to compile even when not deeply nested. But that's a hunch at this point. This would mean the problem is less with kotlinx.html and more with the Kotlin compiler. |
We've migrated a significant number of HTML+Groovy templates over the Kotlinx.html.
The DSL in fantastic! We're super happy developers using it... until we compile.
As mentioned in this issue the DSL compiles very, VERY, slow. We have no way of knowing exactly how much time is spend by the compiler on each file (if anyone knows please let me know), but every time we ported a bunch of templates to Kotlinx.html, we saw a stark increase in compile times.
If I would have know this up front, I'd have chosen a different solution. And hence I believe this project's README should warn (potential) users for this caveat.
The text was updated successfully, but these errors were encountered: