Closed Bug 1527505 Opened 6 months ago Closed 6 months ago

Speed up testing for permissions

Categories

(Core :: Permission Manager, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox67 --- fixed

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(11 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
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

Depends on D20232

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.