jquery-bootgrid icon indicating copy to clipboard operation
jquery-bootgrid copied to clipboard

Drastically faster templating approach

Open zspitzer opened this issue 8 years ago • 11 comments

The current templating approach is really inefficient, as it recursively loops thru every passed available variable for substituion and applies a regex for each one.

It's much faster to initially parse the template and then only process those strings which are actually present in the template.

This revised implementation both caches the parsed templates and then only processes the strings which are present.

The performance improvement is quite drastic, especially with large tables. A large table was previously taking 15s to render in Chrome, now takes less than 1s with this approach

zspitzer avatar Sep 15 '16 10:09 zspitzer

Any chanche this fill be merged with the main branch? Seems nice to speed up rendering of large tables!

yooakim avatar Dec 08 '16 10:12 yooakim

Is this project still active? I'm using the library, it's awesome, but I don't see any new commits (but many new pull requests).

alvaropag avatar Dec 08 '16 12:12 alvaropag

I have recently verified the pull request, but it seems not to work correctly in any situation. The info footer Showing {{ctx.start}} to {{ctx.end}} of {{ctx.total}} entries for example was not rendered. Would be nice to see some tests for it.

rstaib avatar Dec 08 '16 13:12 rstaib

the problem is a nested template string infos: "Showing {{ctx.start}} to {{ctx.end}} of {{ctx.total}} entries", https://github.com/zspitzer/jquery-bootgrid/blob/master/src/public.js#L337

rather than add yet another level of recursion which would be repeated for every templated string, what do you think about refactoring this template string into the common templates definition? https://github.com/zspitzer/jquery-bootgrid/blob/master/src/public.js#L397

zspitzer avatar Dec 09 '16 00:12 zspitzer

with the small dataset in the demo, it's taking 36ms to render with the old approach and 20ms to render using this new approach

zspitzer avatar Dec 09 '16 00:12 zspitzer

I have updated the demo with more append options (10 ,100 and 1000 rows) and logging render time to both the browser console and on screen

using Chrome 55, win10 x64 old approach [appendData: 10 rows] : 52.189ms [appendData: 100 rows] : 526.082ms [appendData: 1000 rows] : 4707.418ms

new approach [appendData: 10 rows] : 14.790ms [appendData: 100 rows] : 29.907ms [appendData: 1000 rows] : 278.368ms

just testing the new approach, appending extra context

IE 11 [appendData: 100 rows] : 180.595ms [appendData: 1000 rows] : 5,827.922ms [appendData: 1000 rows] : 19,008.279ms EDGE [appendData: 100 rows] : 84.63ms [appendData: 1000 rows] : 1,270.115ms [appendData: 1000 rows] : 4,146.51ms Firefox [appendData: 100 rows] : 41.05ms [appendData: 1000 rows] : 168.66ms [appendData: 1000 rows] : 325.76ms Chrome [appendData: 100 rows] : 29.359ms [appendData: 1000 rows] : 273.077ms [appendData: 1000 rows] : 664.405ms

zspitzer avatar Dec 10 '16 02:12 zspitzer

I have added a test which just compares the output from both the new and old approaches. I found a problem will null being returned when a property didn't exist.

The old approach left the missing properties in as the template string, see the {{ctx.style}} below from the demo page

<td class="select-cell" style="{{ctx.style}}"><input name="select" type="checkbox" class="select-box" value="9" /></td>

zspitzer avatar Dec 10 '16 03:12 zspitzer

Refactoring the functions out of the String prototypes into functions saved 400ms in IE and Edge https://developer.mozilla.org/en-US/docs/Web/JavaScript/The_performance_hazards_of__%5B%5BPrototype%5D%5D_mutation

Refactoring out Array didn't make so much of a difference tho, but it's best practise #148

Including #285 speed up Chrome by 50%, Firefox by 15% and Edge by 10%, IE still sucks

zspitzer avatar Dec 10 '16 08:12 zspitzer

The remaining performance problems in Edge are simply browser problem https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/10152783/

zspitzer avatar Dec 11 '16 00:12 zspitzer

What is the problem stopping this from being merged for this long?

eazuka avatar Jul 27 '17 07:07 eazuka

+1

jamespsterling avatar Jun 22 '18 22:06 jamespsterling