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] Fix the rest UI thread heavy tasks in Brave News on UI thread #35890

Closed
Tracked by #35960
atuchin-m opened this issue Feb 8, 2024 · 3 comments · Fixed by brave/brave-core#24522
Closed
Tracked by #35960
Assignees
Labels
feature/brave-news formerly brave-today OS/Android Fixes related to Android browser functionality OS/Desktop perf priority/P2 A bad problem. We might uplift this to the next planned release. QA Pass - Android ARM QA Pass-Win64 QA/Yes release-notes/exclude

Comments

@atuchin-m
Copy link
Contributor

atuchin-m commented Feb 8, 2024

A issue to fix the rest of #35695
The #perf thread: https://bravesoftware.slack.com/archives/C3T9S9WKD/p1706889037498549?thread_ts=1706626988.579219&cid=C3T9S9WKD

  1. large data are still stored/iterated on UI thread: [Brave News]: Build feed off the main thread brave-core#21833 (comment)
  2. other similar places in brave_news that process base::Value and builds something after (e.g. getting the list of sources)
  3. private_cdn_request_helper callbacks: https://github.com/brave/brave-core/blob/master/components/brave_news/browser/brave_news_controller.cc#L429-L453
@atuchin-m atuchin-m added perf OS/Android Fixes related to Android browser functionality OS/Desktop feature/brave-news formerly brave-today labels Feb 8, 2024
@atuchin-m atuchin-m changed the title [perf] Fix the rest UI thread heavy task in Brave News on UI thread [perf] Fix the rest UI thread heavy tasks in Brave News on UI thread Feb 8, 2024
@rebron rebron added the priority/P2 A bad problem. We might uplift this to the next planned release. label Feb 9, 2024
@atuchin-m
Copy link
Contributor Author

atuchin-m commented Feb 14, 2024

Some extra details: this is ParseCombinedPublisherList work (+the following callbacks) on the UI thread (19ms on a high end workstation)
image
image

@MadhaviSeelam
Copy link

MadhaviSeelam commented Aug 29, 2024

Verification PASSED using

Brave | 1.70.92 Chromium: 128.0.6613.85 (Official Build) beta (64-bit)
-- | --
Revision | 711f50838f629ac2be4d1d9592f72adecfd3c928
OS | Windows 11 Version 23H2 (Build 22631.4112)
  1. Installed 1.70.94
  2. launched Brave
  3. completed onboarding
  4. clicked News >> Turn on Brave News
  5. confirmed Brave News feed loaded expected
  6. clicked on a news card (eg. Serious Eats)
  7. verified news article opened in a new tab (https://www.seriouseats.com/how-to-store-raspberries-8703108)
  8. confirmed Manage subscriptions button is shown in the toolbar in the article
  9. clicked Manage subscriptions in the toolbar
  10. clicked Follow button
  11. returned to Brave news feed tab
  12. confirmed the publication Serious Eats is shown in Publishers list
  13. confirmed New content available. Reload? button shown
  14. clicked the button and news feed refreshed
  15. clicked Following in the left side panel
  16. confirmed Serious Eats cards are shown in the Following
  17. return to the news article tab
  18. clicked again Manage subscriptions in the toolbar
  19. clicked Unfollow button
  20. return to news feed
  21. confirmed Serious Eats source is removed from Publishers list
  22. clicked New content available, Reload?
  23. confirmed page news feed refreshed and Serious Eats source was removed from Following as well
  24. clicked + sign next to Channels in the Panel left of the feed
  25. added a channel (eg.Cars) in the Brave News dashboard
  26. closed the dashboard
  27. confirmed New content available, Reload?message shown
  28. confirmed Cars is shown in the Channels
  29. clicked Cars
  30. confirmed Channel feed for Cars is loaded
step 5-6 step 7-10 step 12-13 step 15-16 step 17-19 step 21 step 23 step 25 step 27 step 30
image image image image image image image image image image

@Uni-verse Uni-verse added the QA/In-Progress Indicates that QA is currently in progress for that particular issue label Sep 10, 2024
@Uni-verse
Copy link
Contributor

Verified on Samsung Galaxy S21 using version:

Brave	1.70.101 Chromium: 128.0.6613.120 (Official Build) beta (64-bit) 
Revision	dc28ab38d07e9fa668801884c0c55f669ce538eb
OS	Android 13; Build/TP1A.220624.014; 33; REL
  • Ensured that Brave News feed is shown after opting in using the Brave News onboarding card.
  • Ensured subscribing/following sources (Channels, Popular, Suggestions) will be shown in the news feed.
  • Ensured that Brave News toolbar is shown when exploring the news feed. Tapping on the toolbar will open the Brave News settings page.
  • Ensured that updating sources will show Load new content button on NTP.
  • Ensured that subscriptions are retained after turning Brave News off and then on again.
  • Ensured custom RSS feeds are added to Following after subscribing.
  • Ensured following/unfollowing sources is working and updates the feed.
  • Ensured scrolling through feed is smooth/consistent and does not cause performance issues.
Example Example Example Example
Screenshot 2024-09-10 at 3 47 10 PM Screenshot 2024-09-10 at 3 47 14 PM Screenshot 2024-09-10 at 3 47 35 PM Screenshot 2024-09-10 at 3 47 46 PM
Screenshot 2024-09-10 at 3 53 24 PM Screenshot 2024-09-10 at 3 53 39 PM Screenshot 2024-09-10 at 3 53 48 PM Screenshot 2024-09-10 at 3 55 00 PM

@Uni-verse Uni-verse added QA Pass - Android ARM and removed QA/In-Progress Indicates that QA is currently in progress for that particular issue labels Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/brave-news formerly brave-today OS/Android Fixes related to Android browser functionality OS/Desktop perf priority/P2 A bad problem. We might uplift this to the next planned release. QA Pass - Android ARM QA Pass-Win64 QA/Yes release-notes/exclude
Projects
Status: Completed
Status: Done
Development

Successfully merging a pull request may close this issue.

6 participants