Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pager widget - slow table update when there are a large number of page number links #711

Merged
merged 4 commits into from
Sep 16, 2014

Conversation

camallen
Copy link
Contributor

I am using the pager widget with server side filtering. When paged results where being returned the pager's 'goto' page links get updated. For large page sets the browser slows noticeable (sometimes even causing unresponsive script errors) while inserting / replacing a large list of 'option' goto page links.

You can simulate the issue by replacing pg = Math.min( p.totalPages, p.filteredPages ); with a large value, my page set was something like 25K pg = 25000; and then watch your browser slow down on render

I couldn't get a faster DOM insert / update mechanism working properly, see:

Instead I've created a representative sample into the set of "page options links", selecting a page based on a 'max_option_size' value, e.g. select_every_x_records = 'total number of pages' / max_option_size.

This might not be a great solution for everyone but it works for me. Perhaps this PR will save someone a few hours and possibly start a discussion re large page links sets slowing down the browser.

@Mottie
Copy link
Owner

Mottie commented Aug 25, 2014

Hi @camallen!

It's interesting that this isn't an issue at all in webkit... I changed the code to add 25000 options and I didn't notice any difference. The problem does occur in Firefox and IE.

I believe the fastest way to concatenate is using strings... I tried this test with the more modern browsers and oddly Firefox totally blew away the other browsers making the graphs hard to read.

I did notice a significant delay when using jQuery's .val() instead of plain .value in Firefox but not in Chrome (see this test).

So, I do like your idea of limiting the option steps; but I won't accept your pull request as-is because it seems that the building, then joining of an array is still slower than string concatenation.

Either way, some more testing needs to be done and to determine why Firefox isn't behaving properly.

Also, I will change that one line to read like this:

// p.$goto.html(t).val( p.page + 1 );
p.$goto[0].innerHTML = t;
p.$goto[0].value = p.page + 1;

@Mottie
Copy link
Owner

Mottie commented Aug 25, 2014

Oh, I forgot to mention... as an alternative to using a dropdown select, the output option now includes a method to add an input in place of the current page number by using {page:input}:

output: '{page:input} / {totalPages}'

You can see this working on in the pager demo

@camallen
Copy link
Contributor Author

Hey @Mottie, your totally correct re string concatenation vs array buffer. I've specifically tested for long strings as well - http://jsperf.com/stupid-long-string-concatenation. As such I've removed the offending commit and reworked the PR.

I think the var max_option_size = 20 should probably be moved to a config option on the pager widget with sensible / null defaults, do you want me to add this in?

It's a pretty neat solution changing the page option selector to an input. The user would have to calculate the page but it avoids this issue completely. Unfortunately I can't use this just now as I am using an older version of the library. I need to invest some time upgrading my code to work with the newer version.

@Mottie
Copy link
Owner

Mottie commented Aug 26, 2014

Hi @camallen! Yes, please add an option! :)

So if I understand the logic... basically, if the number of pages is greater than the max option size, the dropdown will skip every max option size number? I think it might be nice to have that number reduced around the current page. For example, if max_option_size were set to 10 and we are on page 50/100, the drop down should show something like this:

10 20 30 40 45 46 47 48 49 [50] 51 52 53 54 55 60 70 80 90 100

What do you think?

@camallen
Copy link
Contributor Author

The logic is slightly different, if the number of pages (pg = Math.min( p.totalPages, p.filteredPages );) is greater than max_option_size then figure out how many records per max options page set i.e. (skip_set_size = Math.floor(pg / max_option_size)).

I see what you want to do have greater page visibility around the current page, I'll have a look at it.

@camallen
Copy link
Contributor Author

I've added a focus set around the current page number, it could probably do with a refactor to a function as well. I've also extracted to the maxOptionSize to pager widget default.

I haven't touched the documentation not tested if the configuration option being overridden.

Let me know what you think.

@Mottie
Copy link
Owner

Mottie commented Sep 16, 2014

Hey @camallen!

Sorry, I totally forgot about this pull request. I'll try to work it into the next update.

Any chance you could update the pull request to make it easy to merge?

@camallen camallen force-pushed the slow_page_number_links branch from c63e6cd to 74163cf Compare September 16, 2014 07:40
@camallen
Copy link
Contributor Author

@Mottie - just rebased of the latest master.

Mottie added a commit that referenced this pull request Sep 16, 2014
Pager widget - slow table update when there are a large number of page number links
@Mottie Mottie merged commit 43d80b6 into Mottie:master Sep 16, 2014
Mottie added a commit that referenced this pull request Sep 16, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants