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

Move handling mojom::BatAdsClient from UI thread #41632

Closed
atuchin-m opened this issue Oct 15, 2024 · 5 comments · Fixed by brave/brave-core#26866
Closed

Move handling mojom::BatAdsClient from UI thread #41632

atuchin-m opened this issue Oct 15, 2024 · 5 comments · Fixed by brave/brave-core#26866
Assignees
Labels
feature/ads OS/Desktop perf priority/P2 A bad problem. We might uplift this to the next planned release. QA/Yes release-notes/exclude

Comments

@atuchin-m
Copy link
Contributor

As investigated in #41549 decoding DBTransactionInfo mojo messages happens on UI thread.
In some real-world scenarios it could takes 30ms+ even on high-perf m1 machines resulting in hanging UI thread.
That code need to be move from UI or replace to a one without mojom-encoding/decoding (as @aseren suggested in the thread).
Image

slack thread: https://bravesoftware.slack.com/archives/C3T9S9WKD/p1728567272430889

@tmancey tmancey added this to Ads Oct 15, 2024
@github-project-automation github-project-automation bot moved this to New issues in Ads Oct 15, 2024
@tmancey tmancey removed their assignment Oct 15, 2024
@tmancey tmancey added priority/P3 The next thing for us to work on. It'll ride the trains. QA/Yes release-notes/exclude dev-concern labels Oct 15, 2024
@tmancey tmancey moved this from New issues to Backlog in Ads Oct 15, 2024
@iefremov
Copy link
Contributor

@tmancey perhaps this could be P2 - the user impact is pretty visible (so i assume this not merely dev concern, but a product bug, when reproduced)

@bridiver
Copy link
Contributor

Why are we returning such a large amount of data from the DB in the first place vs paginating the results or doing aggregations in SQL?

@tmancey tmancey moved this from Dev Concern ❤️ to Backlog 🔜 in Ads Oct 24, 2024
@tmancey
Copy link
Contributor

tmancey commented Oct 24, 2024

@iefremov because this to my knowledge only occurred after a recent migration path, we added as a P3 which will be fixed before we bump the database version again. If this has changed we can move to a P2.

@tmancey
Copy link
Contributor

tmancey commented Oct 24, 2024

I will run some perf tests locally using chrome://tracing. Thanks

@tmancey tmancey added priority/P2 A bad problem. We might uplift this to the next planned release. and removed priority/P3 The next thing for us to work on. It'll ride the trains. labels Oct 25, 2024
@tmancey tmancey moved this from Backlog 🔜 to To Do in Ads Oct 25, 2024
@atuchin-m
Copy link
Contributor Author

The trace from the perf pre test when we have this long UI task.
tmplc3ac0yr.pb.gz

Image

@tmancey tmancey moved this from To Do to In progress in Ads Oct 25, 2024
@tmancey tmancey self-assigned this Oct 25, 2024
@aseren aseren self-assigned this Dec 2, 2024
@aseren aseren moved this from In progress to Review in Ads Dec 10, 2024
@github-project-automation github-project-automation bot moved this from Review to Done in Ads Dec 10, 2024
@brave-builds brave-builds added this to the 1.75.x - Nightly milestone Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/ads OS/Desktop perf priority/P2 A bad problem. We might uplift this to the next planned release. QA/Yes release-notes/exclude
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

6 participants