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

perf: improve ProteusClientCoreCrypto performance [WPB-10000] 🍒 #2985

Merged

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Sep 2, 2024

SpikeWPB-10000 [Android] Investigate the text last read event handling bottleneck and spickes

This PR was automatically cherry-picked based on the following PR:

Original PR description:



PR Submission Checklist for internal contributors

  • The PR Title

    • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
    • contains a reference JIRA issue number like SQPIT-764
    • answers the question: If merged, this PR will: ... ³
  • The PR Description

    • is free of optional paragraphs and you have filled the relevant parts to the best of your ability

What's new in this PR?

Issues

We're a bit slow to process events.

Causes

In part, it's nature of E2EE. In part, we are not really being optimal.

In the generic sense, we can improve Proteus/CoreCrypto performance by not saving session (which is unnecessary, as CoreCrypto already does it during decryption), and also avoid checking if the session exists over and over again if we already know it exists.

Solutions

  1. Remove the "saveSession" bit of ProteusClientCoreCrypto.
  2. Cache known sessions in-memory and check that instead of relying only on CoreCrypto, which uses IO in order to do that

Document my findings in this notebook.


PR Post Merge Checklist for internal contributors

  • If any soft of configuration variable was introduced by this PR, it has been added to the relevant documents and the CI jobs have been updated.

References
  1. https://sparkbox.com/foundry/semantic_commit_messages
  2. https://github.com/wireapp/.github#usage
  3. E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

* perf: improve ProteusClientCoreCrypto performance

* docs: fix typos

* chore: move notebooks to docs/notebooks
@github-actions github-actions bot added cherry-pick PR is cherry-picking changes from another banch echoes: technical-roadmap Work contributing to the Technical Roadmap, to improve our velocity or reduce the technical debt. 👕 size: XXL labels Sep 2, 2024
Copy link

sonarqubecloud bot commented Sep 5, 2024

Copy link
Contributor Author

github-actions bot commented Sep 5, 2024

Test Results

3 197 tests  ±0   3 092 ✔️ ±0   3m 35s ⏱️ -5s
   550 suites ±0      105 💤 ±0 
   550 files   ±0          0 ±0 

Results for commit 40edd9e. ± Comparison against base commit 59ddcf2.

♻️ This comment has been updated with latest results.

Copy link
Contributor Author

github-actions bot commented Sep 5, 2024

🐰Bencher

ReportThu, September 5, 2024 at 10:43:20 UTC
Projectkalium
Branchperf/improve-event-processing-speed-cherry-pick
Testbedubuntu-latest

⚠️ WARNING: The following Measure does not have a Threshold. Without a Threshold, no Alerts will ever be generated!

  • Latency (latency)

Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the --ci-only-thresholds CLI flag.

Click to view all benchmark results
BenchmarkLatencyLatency Results
nanoseconds (ns)
com.wire.kalium.benchmarks.logic.CoreLogicBenchmark.createObjectInFiles➖ (view plot)690,606.52
com.wire.kalium.benchmarks.logic.CoreLogicBenchmark.createObjectInMemory➖ (view plot)608,948,235.79
com.wire.kalium.benchmarks.persistence.MessagesNoPragmaTuneBenchmark.messageInsertionBenchmark➖ (view plot)923,110,947.89
com.wire.kalium.benchmarks.persistence.MessagesNoPragmaTuneBenchmark.queryMessagesBenchmark➖ (view plot)21,192,415.36

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

@vitorhugods vitorhugods added this pull request to the merge queue Sep 5, 2024
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Project coverage is 52.60%. Comparing base (59ddcf2) to head (40edd9e).

Files with missing lines Patch % Lines
...kalium/cryptography/ProteusClientCoreCryptoImpl.kt 0.00% 4 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #2985   +/-   ##
========================================
  Coverage    52.60%   52.60%           
========================================
  Files         1296     1296           
  Lines        49823    49831    +8     
  Branches      4648     4650    +2     
========================================
+ Hits         26207    26213    +6     
  Misses       21740    21740           
- Partials      1876     1878    +2     
Files with missing lines Coverage Δ
...kalium/cryptography/ProteusClientCoreCryptoImpl.kt 48.38% <0.00%> (+2.50%) ⬆️

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 59ddcf2...40edd9e. Read the comment docs.

@datadog-wireapp
Copy link

Datadog Report

Branch report: perf/improve-event-processing-speed-cherry-pick
Commit report: 46b508a
Test service: kalium-jvm

✅ 0 Failed, 3092 Passed, 105 Skipped, 10.39s Total Time

Merged via the queue into develop with commit 2f11cb1 Sep 5, 2024
22 checks passed
@vitorhugods vitorhugods deleted the perf/improve-event-processing-speed-cherry-pick branch September 5, 2024 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick PR is cherry-picking changes from another banch echoes: technical-roadmap Work contributing to the Technical Roadmap, to improve our velocity or reduce the technical debt. 👕 size: XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants