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 double rebuilding of default ruleset #33718

Closed
atuchin-m opened this issue Oct 18, 2023 · 7 comments · Fixed by brave/brave-core#20585
Closed

[perf] Fix double rebuilding of default ruleset #33718

atuchin-m opened this issue Oct 18, 2023 · 7 comments · Fixed by brave/brave-core#20585

Comments

@atuchin-m
Copy link
Contributor

Description

As investigated in a thread we have double rebuilding the default adblock ruleset during the startup when one of the extra list is enabled. Sadly we have some extra lists enabled by default (Fanboy's Mobile Notifications, EasyList Cookie).
That significantly increase startup CPU usage.

Steps to Reproduce

There is no user-visible steps, it's about startup performance. Could be verified by perf tests.

Reproduces how often:

Always when at least one extra adblock list is enabled.

Version/Channel Information:

All actual versions are affected (at least from 1.58.x), both Desktop & Android.

Other Additional Information:

  • Does the issue resolve itself when disabling Brave Shields?
    NO (because the engine are still loaded during the startup)
  • Does the issue resolve itself when disabling Brave Rewards?
    NO
  • Is the issue reproducible on the latest version of Chrome?
    NO

Miscellaneous Information:

QA: still need to verify that adblock engine works as expected.

@atuchin-m
Copy link
Contributor Author

The PR is merged to master. Perf metrics were improved as expected.

The dashboard shows about -300ms CPU usage on startup. Adblock CPU startup usage has been reduced by half in the default scenario.
2044ms (before) => 1757ms (after). Just to reference: 1368ms (before, without adblock).

Also, memory usage of the browser process was improved: private_footprint_size: 144MB => 135MB. reported_by_chrome:allocated_objects_size hasn’t been changed. I suppose the PR reduced the peak memory usage during startup. Because some of that memory wasn’t returned to the OS (probably saved in the allocator cache for future usage), the only first metric was improved.
image

@kjozwiak
Copy link
Member

@brave/qa-team re: the above, we'll basically need to ensure that adblocking is still working as expected. So basically run through a quick spot check to ensure that everything is working as expected. For example, here's a quick spot check that I ran through via brave/brave-core#20585 (comment) before uplifting into 1.60.x & 1.59.x.

@kjozwiak
Copy link
Member

The above can also be verified alongside #32799. Basically ensuring that adblocking is working as expected.

@GeetaSarvadnya
Copy link

GeetaSarvadnya commented Oct 25, 2023

Verification PASSED on

Brave | 1.59.124 Chromium: 118.0.5993.117 (Official Build) (64-bit)
-- | --
Revision | 63d57e32ce0ff0a1e380461bc394351e97d1416a
OS | Windows 10 Version 22H2 (Build 19045.3570)

Verification notes can be found under #32799 (comment)


Verification PASSED on Vivo X70 Pro version 12 running Bravemonoarm64.apk_1.59.124

Verification notes can be found under #32799 (comment)

@hffvld hffvld added the QA/In-Progress Indicates that QA is currently in progress for that particular issue label Oct 25, 2023
@hffvld
Copy link
Contributor

hffvld commented Oct 25, 2023

Verified on Galaxy Tab S8 using version(s):

Device/OS: Galaxy Tab S8 [gts8wifixx-user 13 TP1A.220624.014 release-keys]
Brave build: 1.59.124
Chromium: 118.0.5993.117 (Official Build) (64-bit)
Revision: 63d57e32ce0ff0a1e380461bc394351e97d1416a

STEPS:

  1. Followed the steps from [iOS] Use correct components for Adblock services #32799 (comment)

ACTUAL RESULTS:

  • Verified that ads loaded within the page when visited https://www.zerohedge.com/ and disabled Shields. Re-enabled Shields and verified that ads were being blocked without issues.
  • Verified that first-party ads were appearing when visited https://www.google.com/ and searched for Under Armour. Switched to Aggressively block tracker & ads and verified those ads were being removed/not being displayed anymore
  • Verified that enabling Block scripts blocked the carousel when visiting https://www.blizzard.com.
  • Verified that there was no cookie consent banner being displayed on the page when visiting https://digikey.com if Block cookie consent notices toggle is ON.
  • Verified that the page appears grey when visiting https://mixed-script.badssl.com

1 2 3 4
1 2 3 4
1 2 3 4
1 2 3 4

@hffvld hffvld added QA Pass - Android Tab and removed QA/In-Progress Indicates that QA is currently in progress for that particular issue labels Oct 25, 2023
@btlechowski
Copy link

Verified with

Brave 1.59.124 Chromium: 118.0.5993.117 (Official Build) (64-bit)
Revision 63d57e32ce0ff0a1e380461bc394351e97d1416a
OS Linux

Verified in #32799 (comment)

@kjozwiak
Copy link
Member

Verification PASSED on macOS 14.0 x64 Sonoma using the following build(s):

Brave | 1.59.124 Chromium: 118.0.5993.117 (Official Build) (x86_64)
--- | ---
Revision | 63d57e32ce0ff0a1e380461bc394351e97d1416a
OS | macOS Version 14.0 (Build 23A344)

Using the STR/information outlined via #32799 (comment), ensured that ad-blocking was generally working as per the following:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment