Speed up testing for permissions
Categories
(Core :: Permission Manager, enhancement, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox67 | --- | fixed |
People
(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)
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.
Updated•5 years ago
|
Assignee | ||
Comment 1•5 years ago
|
||
We choose 100,000 iterations so that on a fast processor the test takes
times in the order of seconds.
Assignee | ||
Comment 2•5 years ago
|
||
Depends on D20229
Assignee | ||
Comment 3•5 years ago
|
||
Depends on D20230
Assignee | ||
Comment 4•5 years ago
|
||
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
Assignee | ||
Comment 5•5 years ago
|
||
Depends on D20232
Assignee | ||
Comment 6•5 years ago
|
||
Depends on D20233
Assignee | ||
Comment 7•5 years ago
|
||
Depends on D20234
Assignee | ||
Comment 8•5 years ago
|
||
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
Assignee | ||
Comment 9•5 years ago
|
||
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
Assignee | ||
Comment 10•5 years ago
|
||
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
Assignee | ||
Comment 11•5 years ago
|
||
Depends on D20238
Comment 12•5 years ago
|
||
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
Comment 13•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/93e82f542352
https://hg.mozilla.org/mozilla-central/rev/39fcd32789bb
https://hg.mozilla.org/mozilla-central/rev/dc3ac9b1a0a8
https://hg.mozilla.org/mozilla-central/rev/6d0d4698d35a
https://hg.mozilla.org/mozilla-central/rev/6593aa8f25dd
https://hg.mozilla.org/mozilla-central/rev/05278d2d4e73
https://hg.mozilla.org/mozilla-central/rev/da66636d6d3e
https://hg.mozilla.org/mozilla-central/rev/bebbfddb851f
https://hg.mozilla.org/mozilla-central/rev/c7260676a0f7
https://hg.mozilla.org/mozilla-central/rev/baf310ea412c
https://hg.mozilla.org/mozilla-central/rev/1502462b388b
Comment 14•5 years ago
|
||
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
Comment 15•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/93e82f542352
https://hg.mozilla.org/mozilla-central/rev/39fcd32789bb
https://hg.mozilla.org/mozilla-central/rev/dc3ac9b1a0a8
https://hg.mozilla.org/mozilla-central/rev/6d0d4698d35a
https://hg.mozilla.org/mozilla-central/rev/6593aa8f25dd
https://hg.mozilla.org/mozilla-central/rev/05278d2d4e73
https://hg.mozilla.org/mozilla-central/rev/da66636d6d3e
https://hg.mozilla.org/mozilla-central/rev/bebbfddb851f
https://hg.mozilla.org/mozilla-central/rev/c7260676a0f7
https://hg.mozilla.org/mozilla-central/rev/baf310ea412c
https://hg.mozilla.org/mozilla-central/rev/1502462b388b
Description
•