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.

Sign up for GitHub

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

Jump to bottom

Use binary search in getVisibleElements() #5595

Merged
merged 1 commit into from
Mar 10, 2015
Merged

Use binary search in getVisibleElements() #5595

merged 1 commit into from
Mar 10, 2015

Conversation

fkaelberer
Copy link
Contributor

The method getVisibleElements() uses a linear search to find the first page and thumbnail in the visible range. It is called on every scroll event, so it can take a noticeable amount of time for the bottom pages of a very large document.
This PR replaces the linear search with a binary one (which needs O(log n) time).

A good test:
[3] pdf (5000+ pages)

A benchmark:
Scrolling through pages 5500 to 5515 of [3], using the ↓ key, Linux, Firefox Nightly 37.0a1, textLayer=off
Before:
linearsearch2
After:
binarysearch2

(Admittedly, the document has to be very large to notice a difference, but it should be more pronounced on slow devices).

@fkaelberer fkaelberer force-pushed the useBinarySearchToFindVisibleElements branch from cebc851 to 6a72760 Compare December 29, 2014 18:23
@fkaelberer fkaelberer changed the title Update thumbnail style only when visible Use binary search in getVisibleElements() Dec 29, 2014
@yurydelendik
Copy link
Contributor

Really like binary search idea :) I think you can perform binary search only once: to find first div that lays on (or about to touch) top viewport boundary. Then we can scan down to find bottom one (I think using binary search for second part might be slower since it will touch more of clientTop and other similar DOM properties)

On different note: did you look into using https://developer.mozilla.org/en-US/docs/Web/API/element.getBoundingClientRect ?

@fkaelberer
Copy link
Contributor Author

I think you can perform binary search only once:

that might be true, but since the second binary search is very cheap (because it starts where the first search ends), it wouldn't matter much.

(I think using binary search for second part might be slower since it will touch more of clientTop and other similar DOM properties)

I don't think that matters too, because once the layout is done (after the first call of top, clientTop, or similar), further calls to these properties are quite fast.

On different note: did you look into using https://developer.mozilla.org/en-US/docs/Web/API/element.getBoundingClientRect?

I'll have a look.

@fkaelberer
Copy link
Contributor Author

More numbers: getVisibleElements() takes ~12ms to scroll through 5500 pages, 7ms for the thumbnails. Together this is more time than we are allowed to spend within an 60fps animation frame, but it does not matter if we check 5 or 55 pages, because I wouldn't care if it takes 12µs or 120µs.

@CodingFabian
Copy link
Contributor

to be fair, using the key down is not even realistic. this method was hot before i added the rAF debouncing. its still hot on mac which likes to fire millions of scroll events when using the touchpad. everything helps here. even 12µs.

@fkaelberer
Copy link
Contributor Author

its still hot on mac which likes to fire millions of scroll events when using the touchpad.

Since we have rAF now, the method is not be called too often. The method still seems to be very heavy as shown in the profiler, but it isn't: Most of the time is spent in the first line var top = scrollEl.scrollTop, which invokes the layout of the DOM tree (which you can see in the Chrome browser's timeline). It is the layout that is really slow, not the getVisibleElements() method.

@fkaelberer fkaelberer force-pushed the useBinarySearchToFindVisibleElements branch 2 times, most recently from 0ba69b0 to 351909a Compare December 31, 2014 15:04
@fkaelberer
Copy link
Contributor Author

I think using binary search for second part might be slower...

Ok, after thinking about it I agree that linear search might even be faster for the second part, because binary search doesn't profit from the fact that the item to find is likely very close to the start of the search.
I reverted the stop condition of the visibility search to a break statement as it was before, which also makes things a little simpler.

@fkaelberer
Copy link
Contributor Author

On different note: did you look into using https://developer.mozilla.org/en-US/docs/Web/API/element.getBoundingClientRect ?

@yurydelendik I had a look, but honestly I don't know how this could be helpful.

@fkaelberer fkaelberer force-pushed the useBinarySearchToFindVisibleElements branch from 351909a to 347cdde Compare December 31, 2014 15:24
yurydelendik
yurydelendik reviewed Feb 27, 2015
View reviewed changes
web/ui_utils.js
if ((currentHeight + viewHeight) < top) {
continue;
}
elem = view.el;
Copy link
Contributor

Choose a reason for hiding this comment

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

We removed el from the page/thumb. Could you replace this with div?

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: elem -> element

@fkaelberer fkaelberer force-pushed the useBinarySearchToFindVisibleElements branch 3 times, most recently from 8fe8820 to f47dcb1 Compare March 1, 2015 16:37
@fkaelberer fkaelberer force-pushed the useBinarySearchToFindVisibleElements branch from f47dcb1 to a78bb6b Compare March 1, 2015 16:41
@fkaelberer
Copy link
Contributor Author

@yurydelendik I addressed your comments.

@timvandermeij
Copy link
Contributor

/botio-linux preview

@pdfjsbot
Copy link

pdfjsbot commented Mar 1, 2015

From: Bot.io (Linux)

Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/271bf1a4bf2caf0/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Mar 1, 2015

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/271bf1a4bf2caf0/output.txt

Total script time: 1.00 mins

Published

@yurydelendik
Copy link
Contributor

/botio unittest

@pdfjsbot
Copy link

From: Bot.io (Linux)

Received

Command cmd_unittest from @yurydelendik received. Current queue size: 0

Live output at: http://107.21.233.14:8877/e384c9cbfd80aa1/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/e384c9cbfd80aa1/output.txt

Total script time: 0.71 mins

  • Unit Tests: Passed

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

Command cmd_unittest from @yurydelendik received. Current queue size: 0

Live output at: http://107.22.172.223:8877/f3c665227f34dc8/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/f3c665227f34dc8/output.txt

Total script time: 0.70 mins

  • Unit Tests: Passed

yurydelendik added a commit that referenced this pull request Mar 10, 2015
@yurydelendik
Merge pull request #5595 from fkaelberer/useBinarySearchToFindVisible…
f4dbd69 
…Elements

Use binary search in getVisibleElements()
@yurydelendik yurydelendik merged commit f4dbd69 into mozilla:master Mar 10, 2015
@yurydelendik
Copy link
Contributor

Really good. Thank you for the patch.

@fkaelberer fkaelberer mentioned this pull request Mar 15, 2015
Closed
@fkaelberer fkaelberer deleted the useBinarySearchToFindVisibleElements branch March 15, 2015 15:45
@timvandermeij timvandermeij mentioned this pull request Mar 15, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@yurydelendik yurydelendik

Labels
performance viewer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@fkaelberer @yurydelendik @CodingFabian @timvandermeij @pdfjsbot