Skip to content
This repository has been archived by the owner on Feb 3, 2022. It is now read-only.

Performance Improvement #128

Closed
wants to merge 2 commits into from
Closed

Performance Improvement #128

wants to merge 2 commits into from

Conversation

sboisvert
Copy link

Add deferRender = true to improve the performance in older browsers by only creating the DOM nodes for the items that are currently being displayed on page 1.

See https://datatables.net/reference/option/deferRender

maxneuvians and others added 2 commits April 8, 2019 11:19
* Modification for deploying on k8s

* Small fix on dockerfile

* Added CI workflow file

* Ignore pip pinning in CI
add deferRender = true to improve the performance in older browsers by only creating the DOM nodes for the items that are currently being displayed on page 1.

See https://datatables.net/manual/ajax
Copy link
Member

@timarney timarney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great. For this PR thinking we should only include the patch for the single file change in tables.js

Copy link
Contributor

@obrien-j obrien-j left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rebase to get rid of already merged commit plz

Can you also profile this on a datastore of say 2000 records, to get an idea of improvement in load time plz.

@timarney
Copy link
Member

timarney commented Apr 8, 2019

Tested with a sandbox database 3314 records.

Using BrowserStack + Sandbox DB to localhost so not super accurate but seems like for IE 11 this speeds things up by guessing around 8secs.

The big thing in this case is it's not creating a bunch of DOM nodes that it doesn't need off the top.

As an example to help illustrate this, if you load a data set with 10,000 rows, but a paging display length of only 10 records, rather than create all 10,000 rows, when deferred rendering is enabled, DataTables will create only 10.

@obrien-j
Copy link
Contributor

obrien-j commented Apr 8, 2019

@ptd-tbs @sayaHub We might want to cherry pick this into existing production deployment.

@timarney
Copy link
Member

timarney commented Apr 8, 2019

@sboisvert

The K8s branch was merged #127 so you can rebase to master then make you one line update.

if (!options.deferRender) options.deferRender = true;

@timarney timarney mentioned this pull request Apr 9, 2019
@timarney
Copy link
Member

timarney commented Apr 9, 2019

Closing in favour of #129

-> same fix

@timarney timarney closed this Apr 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants