Closed Bug 1685677 Opened 5 months ago Closed 4 months ago

Refactor QuotaManager to improve maintainability

Categories

(Core :: Storage: Quota Manager, task)

task

Tracking

()

RESOLVED FIXED
87 Branch
Tracking Status
firefox86 --- wontfix
firefox87 --- fixed

People

(Reporter: sg, Assigned: sg)

References

(Blocks 1 open bug)

Details

Attachments

(7 files)

Refactor QuotaManager to improve maintainability by

  • reducing statefulness
  • splitting up large member functions
Depends on: 1685679

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

  • LockedGetOriginInfosExceedingGroupLimits
  • LockedGetOriginInfosExceedingOverallLimits
  • RemoveQuotaForOrigins

Depends on D101144

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

Pushed by sgiesecke@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/206afbb44fad
Make QuotaManager members const/InitializedOnce where possible. r=dom-workers-and-storage-reviewers,janv
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → 86 Branch
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Blocks: 1689967
Attachment #9196042 - Attachment description: Bug 1685677 - Extract functions from CheckTemporaryStorageLimits. r=#dom-workers-and-storage → Bug 1685677 - Extract functions from CheckTemporaryStorageLimits and rename it to CleanupTemporaryStorage. r=#dom-workers-and-storage
Attachment #9196043 - Attachment description: Bug 1685677 - Harmonize LockedGetOriginInfosExceedingGroupLimits and LockedGetOriginInfosExceedingOverallLimits. r=#dom-workers-and-storage → Bug 1685677 - Harmonize LockedGetOriginInfosExceedingGroupLimit and LockedGetOriginInfosExceedingGlobalLimit. r=#dom-workers-and-storage
Pushed by sgiesecke@mozilla.com:
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
Pushed by sgiesecke@mozilla.com:
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
Pushed by sgiesecke@mozilla.com:
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
Pushed by sgiesecke@mozilla.com:
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

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);

Pushed by sgiesecke@mozilla.com:
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

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.

Flags: needinfo?(sgiesecke)
Pushed by sgiesecke@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/548e1eae9ef0
Refactor QuotaManager::CollectOriginsForEviction. r=dom-workers-and-storage-reviewers,janv
Blocks: 1689680
Blocks: 1691070
Keywords: leave-open
Pushed by sgiesecke@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/587852a245bc
Extract functions from EnsureStorageIsInitialized. r=dom-workers-and-storage-reviewers,janv
Status: REOPENED → RESOLVED
Closed: 5 months ago4 months ago
Resolution: --- → FIXED
Target Milestone: 86 Branch → 87 Branch
You need to log in before you can comment on or make changes to this bug.