-
Notifications
You must be signed in to change notification settings - Fork 30
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
PDF: Performance improvement #140
base: master
Are you sure you want to change the base?
Conversation
3a5839c
to
42b57c7
Compare
Is the main idea to skip rendering pages that aren't visible? There's actually a simpler way to retrieve the currently visible pages directly from pdf.js. I noticed that when I scroll, annotations on some pages aren't rendered. Inter-page highlights aren't updated properly re-rendering after modification as well. Was the rendering slow enough that it prompted you to implement this fix? Please tell more about your system and what PDFs caused issues i.e. OCRed. |
Here is my system and document info:
Yes, the primary goal of my pull request is to avoid rendering operations that do not impact the user experience. By reducing unnecessary _render() calls, I aim to enhance the overall performance of the PDF reader in Zotero. I've experienced that Zotero 7 can sometimes feel laggy, especially when running on a 4K display and zooming into PDFs extensively. In contrast, the native pdf.js implementation in Firefox performs exceptionally well. This discrepancy suggests there is significant room for optimizing Zotero's PDF rendering capabilities. Using Firefox's performance tools, I gathered benchmark data indicating that the render() function consumes a considerable amount of time. Upon reviewing Zotero's codebase, I observed that _render(), being a CPU-intensive function, is invoked frequently—many times unnecessarily. This excessive calling contributes to the laggy experience. Even with the changes introduced in this PR, performance can still be suboptimal under extreme conditions, such as deep zooming and rapid scrolling. Here is the performance result I obtained: For reasons that are not entirely clear to me, the reader seems to be heavily utilizing the CPU for data copying operations, which affects overall responsiveness. I would greatly appreciate any guidance or suggestions you might have to further optimize this. Firefox 128 – Linux – 10_12_2024, 5_17_58 AM UTC – Firefox Profiler.zip |
After some reflection and reviewing the codebase, I believe it would be beneficial to separate the rendering content into three distinct levels:
With this structure:
This separation aims to minimize unnecessary rendering operations, thereby improving overall performance. By rendering each layer based on its specific update frequency, we can reduce the computational load and enhance the responsiveness of the PDF reader in Zotero. I would like to develop a proof of concept to validate this approach. What are your thoughts on this idea? |
It's great that you've started investigating Zotero Reader performance, and your suggestion for rendering separation could make sense. However, before implementing anything, we should first compare the performance with pdf.js on Firefox 115 ESR, since that's what Zotero is currently using. And we need to test if there's a performance difference when viewing a PDF file without annotations. Based on those results, we can think about the next steps. The PDF file you provided has no performance issues for me. How many annotations are you testing it with? |
Hi @mrtcode, I am back with some performance data as per your sugguestions. These are the results I got from Firefox Performance tool when I scrolled up and down, zoomed in and zoomed out with the demo.pdf: The first one is from reader without modification whilist the second one is from reader not rendering annotations. I added the following change to reader to avoid rendering annotations: diff --git a/pdfjs/pdf.js b/pdfjs/pdf.js
--- a/pdfjs/pdf.js
+++ b/pdfjs/pdf.js
@@ -1 +1 @@
-Subproject commit 80dd98211fbcdc94ec3a4e7f045d19d069041b1c
+Subproject commit 80dd98211fbcdc94ec3a4e7f045d19d069041b1c-dirty
diff --git a/src/pdf/pdf-view.js b/src/pdf/pdf-view.js
index acbedf6..58113f3 100644
--- a/src/pdf/pdf-view.js
+++ b/src/pdf/pdf-view.js
@@ -612,6 +612,7 @@ class PDFView {
_render(pageIndexes) {
+ return;
for (let page of this._pages) {
if (!pageIndexes || pageIndexes.includes(page.pageIndex)) {
page.render(); What we can see from the results is that when rendering annotaions, it takes a lot of time in In addition, I collected some data with Rendering annotions:
Not rendering annotations:
Overally, I think we can do better in the performance of Zotero reader. As a user heavily relying on Zotero, I'd like to have performance as good as possible and I am glad to participate in the improvement. |
Thanks for investigating! I actually can’t reproduce any performance difference with the rendering function commented out or not. However, I agree that the function is relatively demanding and, if called too often, may cause issues on machines with limited graphics acceleration. In that case, you would likely notice lag when drawing ink annotations or moving/resizing other annotations as well. I’ll look into reducing the number of calls to the function. |
Function
_render()
in PDFView is a time-consuming operation and it's a bit overused .This PR tries to eliminate unnecessary invocatins of the function, which would improve the performance significantly. With this PR, Zotero is much more responsive from my test.