Closed Bug 1527505 Opened 7 years ago Closed 7 years ago

Speed up testing for permissions

Categories

(Core :: Permission Manager, enhancement, P1)

Product:
Core
Component:
Permission Manager
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla67
Tracking Flags:
Tracking Status
firefox67 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

Attachments

(11 files)

Bug 1527505 - Part 1: Add a platform-bench test for measuring the performance of TestPermissionFromPrincipal() in the common case that the anti-tracking backend uses it;
47 bytes, text/x-phabricator-request
Details | Review
Bug 1527505 - Part 2: Inline the IsSystemPrincipal check;
47 bytes, text/x-phabricator-request
Details | Review
Bug 1527505 - Part 3: Compute the origin hash once when storing permissions;
47 bytes, text/x-phabricator-request
Details | Review
Bug 1527505 - Part 4: Compute the type index in the recursive loop only once;
47 bytes, text/x-phabricator-request
Details | Review
Bug 1527505 - Part 5: Inline nsPermissionManager::GetTypeIndex();
47 bytes, text/x-phabricator-request
Details | Review
Bug 1527505 - Part 6: Use inline storage for storing the type array in order to speed up searching though it in the common case;
47 bytes, text/x-phabricator-request
Details | Review
Bug 1527505 - Part 7: Compute the default permission in the recursive loop only once;
47 bytes, text/x-phabricator-request
Details | Review
Bug 1527505 - Part 8: Lift the handling of expanded principals out of CommonTestPermissionInternal() into CommonPrepareToTestPermission();
47 bytes, text/x-phabricator-request
Details | Review
Bug 1527505 - Part 9: Make nsIPermissionManager accept ACString arguments for permission types instead of raw C strings;
47 bytes, text/x-phabricator-request
Details | Review
Bug 1527505 - Part 10: Add a variation of TestPermissionFromPrincipal() that knows to not check for the presence of a default pref if the caller knows the permission type doesn't support default prefs, and use it in the anti-tracking backend;
47 bytes, text/x-phabricator-request
Details | Review
Bug 1527505 - Part 11: Avoid looking up the effective TLD service repeatedly inside the permission manager;
47 bytes, text/x-phabricator-request
Details | Review

Bug 1516540 made me look into speeding up testing for permissions some. I have a patch series which speeds up a microbenchmark which I'm adding in the first part of the series by an order of magnitude. There are a number of optimizations and the work needed has been done in several parts, so I suggest having a look at the overall series before reviewing each patch in depth since that should help paint the overall picture of where each individual patch in the series is going.

I'll post the patches after a final round of testing on the try server.

Priority: -- → P1

We choose 100,000 iterations so that on a fast processor the test takes
times in the order of seconds.

Depends on D20229

This patch alone decreases the runtime of the loop on my machine from about
1700 to 200-400ms ranges. It turns out that computing the type index is
the most expensive part. So perhaps we should look into improving that as
well.

The first thing that comes to mind is whether we can inline the loop in
GetTypeIndex(). The next part takes care of that, and it does help a bit.
But we need to do more still.

The next obvious thing is to optimize the memory access patterns. Right
now we iterate over an array of dynamically allocated strings to compare
them, which amounts to pointer chasing to read a bit of memory, kind of
the worst possible way to access memory. Then we look at replacing that
with fully sequential memory reads in the common cases.

Depends on D20231

This last patch in the series ensures that the order of operations in recursive
invocations of CommonTestPermission() remains consistent compared to before the
patch series, even though it is not strictly needed for the performance
improvements that the series focuses on.

The core idea behind CommonPrepareToTestPermission() now is to do the checks
that do not depend on the host name being tested, and for
CommonTestPermissionInternal() itself to focus on the rest of the checks,
that is looking up our hashtable entry based on the host name being tested,
and everything else that's needed from that point on.

Depends on D20235

This will mean that in places like the tight loop in GetTypeIndex()
we would no longer require calling strlen() on the input type argument
once per loop iteration.

Depends on D20236

This gives us the additional benefit that it will provide a clean inlined path
for the common case of testing for permissions from the anti-tracking backend.

Depends on D20237

Pushed by eakhgari@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/93e82f542352 Part 1: Add a platform-bench test for measuring the performance of TestPermissionFromPrincipal() in the common case that the anti-tracking backend uses it; r=nika https://hg.mozilla.org/integration/autoland/rev/39fcd32789bb Part 2: Inline the IsSystemPrincipal check; r=nika https://hg.mozilla.org/integration/autoland/rev/dc3ac9b1a0a8 Part 3: Compute the origin hash once when storing permissions; r=nika https://hg.mozilla.org/integration/autoland/rev/6d0d4698d35a Part 4: Compute the type index in the recursive loop only once; r=nika https://hg.mozilla.org/integration/autoland/rev/6593aa8f25dd Part 5: Inline nsPermissionManager::GetTypeIndex(); r=nika https://hg.mozilla.org/integration/autoland/rev/05278d2d4e73 Part 6: Use inline storage for storing the type array in order to speed up searching though it in the common case; r=nika https://hg.mozilla.org/integration/autoland/rev/da66636d6d3e Part 7: Compute the default permission in the recursive loop only once; r=nika https://hg.mozilla.org/integration/autoland/rev/bebbfddb851f Part 8: Lift the handling of expanded principals out of CommonTestPermissionInternal() into CommonPrepareToTestPermission(); r=nika https://hg.mozilla.org/integration/autoland/rev/c7260676a0f7 Part 9: Make nsIPermissionManager accept ACString arguments for permission types instead of raw C strings; r=nika https://hg.mozilla.org/integration/autoland/rev/baf310ea412c Part 10: Add a variation of TestPermissionFromPrincipal() that knows to not check for the presence of a default pref if the caller knows the permission type doesn't support default prefs, and use it in the anti-tracking backend; r=nika https://hg.mozilla.org/integration/autoland/rev/1502462b388b Part 11: Avoid looking up the effective TLD service repeatedly inside the permission manager; r=nika
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/39117300a256 Port bug 1527505, part 9: Use nsCStrings instead of C literals in permission manager tests. rs=bustage-fix
Depends on: 1535877
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: