1685677 - Refactor QuotaManager to improve maintainability

Status: RESOLVED FIXED

(Core :: Storage: Quota Manager, task)

Description

[Simon Giesecke [:sg] [he/him]]

Refactor QuotaManager to improve maintainability by

• reducing statefulness
• splitting up large member functions

Comment 1

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1685677 - Make QuotaManager members const/InitializedOnce where possible. r=#dom-workers-and-storage

Depends on D101143

Comment 2

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1685677 - Extract functions from CheckTemporaryStorageLimits and rename it to CleanupTemporaryStorage. r=#dom-workers-and-storage

Extracts parts of CheckTemporaryStorageLimits into the following new private
member functions of QuotaManager:

• LockedGetOriginInfosExceedingGroupLimits
• LockedGetOriginInfosExceedingOverallLimits
• RemoveQuotaForOrigins

Depends on D101144

Comment 3

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1685677 - Harmonize LockedGetOriginInfosExceedingGroupLimit and LockedGetOriginInfosExceedingGlobalLimit. r=#dom-workers-and-storage

Instead of inserting all elements into OriginInfo array, and then removing
or skipping irrelevant entries, only insert the relevant ones in the first
place, using a new utility function template MaybeAppendOriginInfos.

Sort them by the LRU timestamp.

Iterate over them to identify the threshold.

Finally, truncate the array to the threshold position.

Depends on D101145

Comment 4

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1685677 - Don't unnecessarily materialize a flattened array of OriginInfo*. r=#dom-workers-and-storage

Depends on D101146

Comment 5

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1685677 - Reduce scope of variables only used within an if statement. r=#dom-workers-and-storage

Depends on D101151

Comment 6

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1685677 - Refactor QuotaManager::CollectOriginsForEviction. r=#dom-workers-and-storage

Depends on D101181

Comment 7

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1685677 - Extract functions from EnsureStorageIsInitialized. r=#dom-workers-and-storage

Depends on D101182

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/206afbb44fad
Make QuotaManager members const/InitializedOnce where possible. r=dom-workers-and-storage-reviewers,janv

Comment 9

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/206afbb44fad

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/121e93b3787f
Extract functions from CheckTemporaryStorageLimits and rename it to CleanupTemporaryStorage. r=dom-workers-and-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/ea958694bd17
Harmonize LockedGetOriginInfosExceedingGroupLimit and LockedGetOriginInfosExceedingGlobalLimit. r=dom-workers-and-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/81a737afce4f
Don't unnecessarily materialize a flattened array of OriginInfo*. r=dom-workers-and-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/263d593f8609
Reduce scope of variables only used within an if statement. r=jstutte

Comment 11

[Sandor Molnar[:smolnar]]

Backed out for causing xpchell failures in test_storagePressure

Backout link: https://hg.mozilla.org/integration/autoland/rev/082a0d699c97597eed40e6344ddcba2669777266

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception%2Crunning%2Cpending%2Crunnable&searchStr=linux%2C18.04%2Cx64%2Copt%2Cxpcshell%2Ctests%2Ctest-linux1804-64%2Fopt-xpcshell-e10s%2Cx2&revision=263d593f86096a9282446d1b33d88bd24ebde0ad&selectedTaskRun=H59gksKFTY-pE7l6E07VfA.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=328449861&repo=autoland&lineNumber=3069

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ed3a62552625
Extract functions from CheckTemporaryStorageLimits and rename it to CleanupTemporaryStorage. r=dom-workers-and-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/26e9adf102c8
Harmonize LockedGetOriginInfosExceedingGroupLimit and LockedGetOriginInfosExceedingGlobalLimit. r=dom-workers-and-storage-reviewers,janv

Comment 13

[Atila Butkovits]

Backed out for causing xpcshell failure.

Backout link: https://hg.mozilla.org/integration/autoland/rev/e993b42aae33f4283e9e62d28454f403bf8dd66f

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=fYBek7yoSoK2q1xZY9tIbw.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&searchStr=linux%2C18.04%2Cx64%2Casan%2Copt%2Cxpcshell%2Ctests%2Ctest-linux1804-64-asan%2Fopt-xpcshell-e10s%2Cx1&revision=26e9adf102c86236a74d087028a49b2f90649317

Failure log: https://treeherder.mozilla.org/logviewer?job_id=328483074&repo=autoland&lineNumber=3140

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e1e26422161e
Extract functions from CheckTemporaryStorageLimits and rename it to CleanupTemporaryStorage. r=dom-workers-and-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/d1fbaf6882a3
Harmonize LockedGetOriginInfosExceedingGroupLimit and LockedGetOriginInfosExceedingGlobalLimit. r=dom-workers-and-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/e3223c398959
Don't unnecessarily materialize a flattened array of OriginInfo*. r=dom-workers-and-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/0110eb26213b
Reduce scope of variables only used within an if statement. r=jstutte

Comment 15

[Sandor Molnar[:smolnar]]

Backed out for causing build bustage in test/gtest/TestFlatten.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/b10d7b687fd4ba85459d7cecac7a8d6130a197f5

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=E5EVXv1DTYGsmMpqesfIaA.0&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&revision=0110eb26213b9498d9695866275847cdbece4c69

Failure log: https://treeherder.mozilla.org/logviewer?job_id=328558399&repo=autoland&lineNumber=9791

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bcbba85964cb
Extract functions from CheckTemporaryStorageLimits and rename it to CleanupTemporaryStorage. r=dom-workers-and-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/cc4f20677a7e
Harmonize LockedGetOriginInfosExceedingGroupLimit and LockedGetOriginInfosExceedingGlobalLimit. r=dom-workers-and-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/3c1b1f51f19c
Don't unnecessarily materialize a flattened array of OriginInfo*. r=dom-workers-and-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/d354b3d5312c
Reduce scope of variables only used within an if statement. r=jstutte

Comment 17

[Natalia Csoregi [:nataliaCs]]

Backed out for bustage on TestFlatten.cpp

backout: https://hg.mozilla.org/integration/autoland/rev/49fff125656d073e955a3066327bc7c405d69cc7

push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&revision=129b45a961fb5bf9a71a217da71ac8079fbc4657&searchStr=build&selectedTaskRun=Vt3sNVvXTfC61EBxM-aiDQ.0

failure log: https://treeherder.mozilla.org/logviewer?job_id=328577529&repo=autoland&lineNumber=36336

[task 2021-02-02T15:02:17.775Z] 15:02:17 INFO - In file included from /builds/worker/workspace/obj-build/dist/include/gtest/gtest.h:383:0,
[task 2021-02-02T15:02:17.775Z] 15:02:17 INFO - from /builds/worker/checkouts/gecko/dom/quota/test/gtest/TestCheckedUnsafePtr.cpp:7,
[task 2021-02-02T15:02:17.775Z] 15:02:17 INFO - from Unified_cpp_dom_quota_test_gtest0.cpp:2:
[task 2021-02-02T15:02:17.775Z] 15:02:17 INFO - /builds/worker/checkouts/gecko/dom/quota/test/gtest/TestFlatten.cpp: In member function 'virtual void mozilla::dom::quota::Flatten_NestedInnerSingulars_Test::TestBody()':
[task 2021-02-02T15:02:17.775Z] 15:02:17 ERROR - /builds/worker/checkouts/gecko/dom/quota/test/gtest/TestFlatten.cpp:60:29: error: class template argument deduction failed:
[task 2021-02-02T15:02:17.787Z] 15:02:17 INFO - EXPECT_EQ((nsTArray{{1, 2}}), flattened);
[task 2021-02-02T15:02:17.787Z] 15:02:17 INFO - ^
[task 2021-02-02T15:02:17.787Z] 15:02:17 INFO - /builds/worker/workspace/obj-build/dist/include/gtest/gtest_pred_impl.h:76:52: note: in definition of macro 'GTEST_ASSERT_'
[task 2021-02-02T15:02:17.787Z] 15:02:17 INFO - if (const ::testing::AssertionResult gtest_ar = (expression))
[task 2021-02-02T15:02:17.787Z] 15:02:17 INFO - ^~~~~~~~~~
[task 2021-02-02T15:02:17.787Z] 15:02:17 INFO - /builds/worker/workspace/obj-build/dist/include/gtest/gtest_pred_impl.h:161:3: note: in expansion of macro 'GTEST_PRED_FORMAT2_'
[task 2021-02-02T15:02:17.787Z] 15:02:17 INFO - GTEST_PRED_FORMAT2_(pred_format, v1, v2, GTEST_NONFATAL_FAILURE_)
[task 2021-02-02T15:02:17.788Z] 15:02:17 INFO - ^~~~~~~~~~~~~~~~~~~
[task 2021-02-02T15:02:17.791Z] 15:02:17 INFO - /builds/worker/workspace/obj-build/dist/include/gtest/gtest.h:1968:3: note: in expansion of macro 'EXPECT_PRED_FORMAT2'
[task 2021-02-02T15:02:17.791Z] 15:02:17 INFO - EXPECT_PRED_FORMAT2(::testing::internal::
[task 2021-02-02T15:02:17.792Z] 15:02:17 INFO - ^~~~~~~~~~~~~~~~~~~
[task 2021-02-02T15:02:17.792Z] 15:02:17 INFO - /builds/worker/workspace/obj-build/dist/include/gtest/gtest.h:1969:32: note: in expansion of macro 'GTEST_IS_NULL_LITERAL_'
[task 2021-02-02T15:02:17.792Z] 15:02:17 INFO - EqHelper<GTEST_IS_NULL_LITERAL_(val1)>::Compare,
[task 2021-02-02T15:02:17.792Z] 15:02:17 INFO - ^~~~~~~~~~~~~~~~~~~~~~
[task 2021-02-02T15:02:17.792Z] 15:02:17 INFO - /builds/worker/checkouts/gecko/dom/quota/test/gtest/TestFlatten.cpp:60:3: note: in expansion of macro 'EXPECT_EQ'
[task 2021-02-02T15:02:17.792Z] 15:02:17 INFO - EXPECT_EQ((nsTArray{{1, 2}}), flattened);
[task 2021-02-02T15:02:17.792Z] 15:02:17 INFO - ^~~~~~~~~
[task 2021-02-02T15:02:17.792Z] 15:02:17 ERROR - /builds/worker/checkouts/gecko/dom/quota/test/gtest/TestFlatten.cpp:60:29: error: no matching function for call to 'nsTArray(<brace-enclosed initializer list>)'
[task 2021-02-02T15:02:17.793Z] 15:02:17 INFO - EXPECT_EQ((nsTArray{{1, 2}}), flattened);

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f8259958012
Extract functions from CheckTemporaryStorageLimits and rename it to CleanupTemporaryStorage. r=dom-workers-and-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/dc241608de95
Harmonize LockedGetOriginInfosExceedingGroupLimit and LockedGetOriginInfosExceedingGlobalLimit. r=dom-workers-and-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/132d9be64a80
Don't unnecessarily materialize a flattened array of OriginInfo*. r=dom-workers-and-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/e7d8aec27b9f
Reduce scope of variables only used within an if statement. r=jstutte

Comment 19

[Simon Giesecke [:sg] [he/him]]

Sorry for the repeated backouts, I hope it's eventually fine now. I manually triggered the base toolchain builds on https://treeherder.mozilla.org/jobs?repo=try&revision=5ba0de2953ea36b556f6cc5c7c1801a3215deaea&selectedTaskRun=YWWCfD__RJuchOITEOCh1Q.0 and they look good now.

Comment 20

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/6f8259958012
https://hg.mozilla.org/mozilla-central/rev/dc241608de95
https://hg.mozilla.org/mozilla-central/rev/132d9be64a80
https://hg.mozilla.org/mozilla-central/rev/e7d8aec27b9f

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/548e1eae9ef0
Refactor QuotaManager::CollectOriginsForEviction. r=dom-workers-and-storage-reviewers,janv

Comment 22

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/548e1eae9ef0

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/587852a245bc
Extract functions from EnsureStorageIsInitialized. r=dom-workers-and-storage-reviewers,janv

Comment 24

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/587852a245bc