2-3000% performance enhancement in IE7 #253
Conversation
Use an array and not a string to store chunks internally. Increases performance by 2-3000% in IE7. No measurable changes in performance in other browsers.
|
vow! |
|
This can impact custom helpers if they manipulate the data object. I have played around with a custom helper that does that. I agree with the change and the benefits are great. Just wanted to point out a possible incompatibility with custom helpers that muck with a semi-internal data structure (e.g. such uses are probably at their own risk) |
|
I see what you mean... We tried an approach where chunk.data was joined into a string before each helper invocation and then converted back to a one element array when helper returned the chunk. Didn't work. A bit faster in IE7 but it was way slower in other browsers. Maybe the faster implementation could be provided through some configuration or an alternate render call or something? Anyway! We made some more testing with an isolated standalone web page with a 7 column table. A template with some helpers and template logic. We tested with 100, 1000, 2000, 5000 and 10000 rows. You can see the results in the attached Excel screenshot. Still good results but not the 2-3-10000% we saw when we tested within our huge SPI application (still measuring the same way though). Our "array chunk" implementation is still 20-200 times faster than the original implementation in IE7 depending on number of rows in table. /johan PS: Unfortunately we have to support IE7, 15% of our clients customers still use IE7... (and that's nearly 150 000 users!) |
|
Don't take my compatibility comment too seriously. I've only ever written one helper that depended on data being a string (and changing it would be easy). I just checked the original akdubya github page and data is not externalized in the documentation at all. The only way to know about it is to dig into the code so I consider it an internal data structure rather than an interface. This makes it fair game for changing. |
|
LGTM! @jlkonsultab thanks for ur help. @rragan I agree with your the "data" object is only known by the people who dug into the code anyway we should advice this to the users. Maybe we could release a new dust version after merging this change. What do you think? |
|
A new version with this would be good. I'd love to see the nested path issue get included though with the option defaulted to off for backward compatibility. The PR needs to have some conflict resolution as it is currently not auto mergeable. |
| @@ -229,7 +229,7 @@ Stub.prototype.flush = function() { | |||
|
|
|||
| while (chunk) { | |||
| if (chunk.flushable) { | |||
| this.out += chunk.data; | |||
| this.out += chunk.data.join(""); //ie7 perf | |||
There was a problem hiding this comment.
Would there be a significant improvement by changing this.out to an array instead of a string?
|
Looks good to me. |
|
@smfoote feel free to merge it in.
|
2-3000% performance enhancement in IE7
|
yay! |
Changed lib/dust.js to use an array for storing the chunk. Makes dust.js usable in IE7.
Our tests: 500 rows in a dust generated HTML table
(original implementation ~12000% slower)
Other tests with less data show differences around 2-3000% .
No measurable changes in performance in other browsers like IE8-10, Chrome, Firefox etc
best regards
Johan Linderoth (jlkonsultab@github)
Nicklas Sigurdh (nicklassigurdh@github)
Kent Pettersson (kent78@github)