Simplify 'hyphenize' #187
Simplify 'hyphenize' #187
Conversation
|
|
||
| map[it]!! | ||
| } | ||
| fun String.hyphenize(): String = |
There was a problem hiding this comment.
@turansky do you mind writing a micro-benchmark comparing the old and the new versions? Yes, the new version is simpler, but I'm wondering if its performance is also OK. See, this function is called really often when constructing CSS, but the inputs are repeating all the time because the number of CSS properties is quite limited. Therefore memoization might be actually useful.
There was a problem hiding this comment.
- Readability looks more important in this case
- Previous code use
synchronized
There was a problem hiding this comment.
Readability looks more important in this case
Why? It's a simple private utility method.
Previous code use synchronized
So?
There was a problem hiding this comment.
Simple JS test
Simple JVM test
7.000.000 calls
My results:
// JS
Before 1885 ms
After 1482 ms
// JVM
Before 269 ms
After 2115 ms
There was a problem hiding this comment.
Simple test
The link opens an empty playground.
There was a problem hiding this comment.
The link opens an empty playground.
It's a bug. Do they have issue tracker? :)
WA - Incognito mode
There was a problem hiding this comment.
OK, so it is significantly slower on the JVM after all, I was not completely wrong :)
Luckily, the regex version is faster in all browsers, so it's fine. Thanks for doing the tests!
It's a bug. Do they have issue tracker? :)
I think it should be reported to the regular YouTrack project, KT.
There was a problem hiding this comment.
First test - border case - 50% enum constants require correction
Popular case - no correction required
// JS
Before 1632 ms
After 303 ms
Playground