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

Performance improvements #38

Merged
merged 47 commits into from
Jun 27, 2024
Merged

Performance improvements #38

merged 47 commits into from
Jun 27, 2024

Conversation

boardend
Copy link
Collaborator

@boardend boardend commented May 22, 2024

Rather large pull request consisting of various performance improvements:

Ideas and progress tracked in docs/performance.md

@boardend boardend marked this pull request as ready for review May 22, 2024 15:06
@boardend boardend requested a review from wonder-sk May 22, 2024 15:06
Copy link
Member

@wonder-sk wonder-sk left a comment

Choose a reason for hiding this comment

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

👏 Nice set of dependency & code & performance updates!

Please feel free to merge...

Few notes:

  • it would be useful to have a small howto for the performance testing tool - how to launch it, what does it do
  • would be great to mark in performance.md what was done / not done
  • I have realized my Firefox is constantly using 1 full CPU when I have the qgis-js demo open, even if the computer is completely idle otherwise (does that happen to you as well?)
  • was there a reason why not stick to emscripten 3.1.37 that is preferred for qt 6.6?
  • 1928c39 - I got a bit lost how this is meant to work - I assume the idea is to have the progressive rendering like in QGIS map canvas, right? In my testing, it behaves a bit strangely, but I am not able to explain well why it's that 🙂
  • minor typo: Srouce -> Source
  • dff9618 - the API was not built to allow starting of jobs in a worker thread - in theory strange things could happen 😄

@boardend
Copy link
Collaborator Author

boardend commented Jun 2, 2024

Hey @wonder-sk, huge thanks for your review 🙏

Feedback to your points:

  • it would be useful to have a small howto for the performance testing tool - how to launch it, what does it do
  • Sure, will do that
  • would be great to mark in performance.md what was done / not done
  • I have realized my Firefox is constantly using 1 full CPU when I have the qgis-js demo open, even if the computer is completely idle otherwise (does that happen to you as well?)
  • Not that I've noticed. But I will do some testing
  • was there a reason why not stick to emscripten 3.1.37 that is preferred for qt 6.6?
  • Just tried to find my source for "by anecdotal evidence, binaries compiled with the latest version are faster (latest LLVM)" but couldn't 😅
    • But I would like to stick to the latest Emscripten version in order to have the latest features and optimizations available (like for example the removal of the separate .worker.js file)
    • Note that Qt states: "You can build Qt from source if you require more flexibility when selecting the Emscripten version. In this case the versions above are minimum versions. Later versions are expected to work but may introduce behavior changes which require making changes to Qt."
      • So I assume this is rather a safe thing to do..?
  • 1928c39 - I got a bit lost how this is meant to work - I assume the idea is to have the progressive rendering like in QGIS map canvas, right? In my testing, it behaves a bit strangely, but I am not able to explain well why it's that 🙂
  • Could you create a video? 😄
  • Note that this is already deployed to the GitHub Pages, see the "Preview" Tab 👀
  • minor typo: Srouce -> Source
  • dff9618 - the API was not built to allow starting of jobs in a worker thread - in theory strange things could happen 😄
  • Added a small comment with 412ba3f
    • I would like to stick to that, because without there is a noticeable freeze of the main thread

@boardend
Copy link
Collaborator Author

boardend commented Jun 7, 2024

@wonder-sk I observe the same behavior in FF 😟 This needs to be fixed before merging this PR

@boardend
Copy link
Collaborator Author

@wonder-sk Did some more testing; This is likely something the FF does internally when loading such a large .wasm module:

  • The high CPU load goes towards zero after some time (~4min on my machine). The website is still working/responsive after that and stays idle when doing more render calls
  • The same thing happens with the current 0.0.3 version, see https://boardend.github.io/qgis-js-baseline/
  • The same thing happens if I just return 0 in the main function
  • This behavior was observed in FF on Ubuntu Wayland and X11, also on FF on Windows 11
  • FF profiler only shows that the PollWrapper is busy polling
  • Hard to judge, but it seems that other larger Qt projects show similar behavior, although not as extreme as in qgis-js:

Unsure what to do with this 🤔

(cc @andreasneumann)

@boardend
Copy link
Collaborator Author

boardend commented Jun 27, 2024

Will merge this one in and add the Firefox high CPU load as a separate issue: #45

@boardend boardend merged commit 73110a3 into main Jun 27, 2024
@boardend boardend deleted the feature/performance-preview branch July 1, 2024 11:00
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