rstaib / jquery-bootgrid

Nice, sleek and intuitive. A grid control especially designed for bootstrap.
http://www.jquery-bootgrid.com
MIT License
972 stars 362 forks source link

Drastically faster templating approach #317

Open zspitzer opened 8 years ago

zspitzer commented 8 years ago

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

yooakim commented 7 years ago

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

alvaropag commented 7 years ago

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).

rstaib commented 7 years ago

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.

zspitzer commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

eazuka commented 7 years ago

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

jamespsterling commented 6 years ago

+1