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

feat(cat-voices): implement web worker for compression logic #1020

Merged
merged 22 commits into from
Oct 18, 2024

Conversation

apskhem
Copy link
Collaborator

@apskhem apskhem commented Oct 17, 2024

Description

Moved the compression functions in JS side to run in web workers to prevent blocking (freezing) on the main thread that might impact on user experiece during using the app.

Related Issue(s)

Closes #1014

Description of Changes

  • Added web worker support to compression functions.
  • Changed all the functions that rely on the interop functions to run with async manner.

Breaking Changes

TD;LR: Compression functions in JS return Promise<string> instead of string.

The compression functions in JS was changed to run in web workers. Hence, all the functions return Promise<string instead of the old version of returning solely string. The interop functions in the Dart side (catalyst_compression_web) also need to change accordingly to support those JS functions. So the packages or functions that rely on this compression package need to convert themselves to run asynchronously.

Please confirm the following checks

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream module

@apskhem apskhem self-assigned this Oct 17, 2024
@apskhem apskhem linked an issue Oct 17, 2024 that may be closed by this pull request
@apskhem apskhem marked this pull request as draft October 17, 2024 09:58
@apskhem apskhem added this to the M4: Voting & Delegation milestone Oct 17, 2024
@apskhem apskhem added enhancement New feature or request review me PR is ready for review do not merge yet PR is not ready to be merged yet labels Oct 17, 2024
Copy link
Contributor

github-actions bot commented Oct 17, 2024

Test Report | ${\color{lightgreen}Pass: 294/294}$ | ${\color{red}Fail: 0/294}$ |

@apskhem
Copy link
Collaborator Author

apskhem commented Oct 17, 2024

@minikin @dtscalac This change affacts several packages inside catalyst_voices_packages. Should we also update the version of the packages?
And also in catalyst_compresion_web, there's still a fixed path to load web worker from here. It fits only for catalyst_voices at the moment. Should we parameterize the path?

Edit: the fixed path problem is indentified by loading from its relative path.

@apskhem apskhem marked this pull request as ready for review October 17, 2024 10:26
@apskhem apskhem marked this pull request as draft October 17, 2024 10:27
@dtscalac
Copy link
Contributor

@minikin Objectively speaking this PR reduces the lag however it still happens. I've debugged the issue and the biggest offender once the compression was optimized is the Ed25519 signing which takes about 180ms per signing, we do it twice per registration.

@apskhem can you please report another task for this? Here's the part of code that is slow:

Screenshot 2024-10-17 at 14 26 43

@apskhem
Copy link
Collaborator Author

apskhem commented Oct 17, 2024

I created a new ticket related to @dtscalac inspecting on the issue (#1025)

@apskhem apskhem requested a review from dtscalac October 17, 2024 12:56
@apskhem apskhem marked this pull request as ready for review October 17, 2024 17:27
@apskhem apskhem removed the do not merge yet PR is not ready to be merged yet label Oct 17, 2024
@apskhem apskhem requested a review from dtscalac October 18, 2024 08:58
Copy link
Contributor

@dtscalac dtscalac left a comment

Choose a reason for hiding this comment

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

LGTM

@dtscalac dtscalac merged commit 54ec09d into main Oct 18, 2024
36 checks passed
@dtscalac dtscalac deleted the feat/webworker-compression branch October 18, 2024 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request review me PR is ready for review
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

🛠️ [TASK] : Move catalyst_compression to perform in web workers
2 participants