Closed
Bug 1389390
Opened 7 years ago
Closed 6 years ago
Add a test to verify eviction works fine on persisted origin
Categories
(Core :: Storage: Quota Manager, enhancement, P2)
Core
Storage: Quota Manager
Tracking
()
RESOLVED
FIXED
mozilla64
Tracking | Status | |
---|---|---|
firefox64 | --- | fixed |
People
(Reporter: tt, Assigned: tt)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 9 obsolete files)
6.16 KB,
patch
|
Details | Diff | Splinter Review | |
4.32 KB,
patch
|
janv
:
review+
|
Details | Diff | Splinter Review |
We decide to move the test in bug 1298329 to here.
Assignee | ||
Comment 1•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Status: NEW → ASSIGNED
Updated•7 years ago
|
Priority: -- → P2
Assignee | ||
Updated•7 years ago
|
Assignee: ttung → nobody
Status: ASSIGNED → NEW
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → shes050117
Status: NEW → ASSIGNED
Priority: P2 → P3
Assignee | ||
Comment 2•6 years ago
|
||
Attachment #8896120 -
Attachment is obsolete: true
Assignee | ||
Comment 3•6 years ago
|
||
Attachment #9004273 -
Attachment is obsolete: true
Assignee | ||
Comment 4•6 years ago
|
||
Attachment #9004274 -
Attachment is obsolete: true
Assignee | ||
Comment 5•6 years ago
|
||
Comment on attachment 9004482 [details] [diff] [review]
Implement a test to verify the behavior of eviction.
Hi Jan,
I add this test to try to verify the persisted origin won't be evicted, but general origin will.
Could you help me to review that? Thanks!
Attachment #9004482 -
Flags: review?(jvarga)
Assignee | ||
Comment 6•6 years ago
|
||
Comment on attachment 9004482 [details] [diff] [review]
Implement a test to verify the behavior of eviction.
Drop the review request for now. I'll revise it based on the patch in bug 1493908 and then send a review request again.
Attachment #9004482 -
Flags: review?(jvarga)
Assignee | ||
Comment 7•6 years ago
|
||
Revise the patch:
- Using requestFinished
- Rebase on the top of patch in bug 1493908
Attachment #9004482 -
Attachment is obsolete: true
Assignee | ||
Comment 8•6 years ago
|
||
Comment on attachment 9011830 [details] [diff] [review]
Implement a test to verify the behavior of eviction.
Jan, I rebase this patch based on your patch in Bug 1493908. This patch tests two eviction mechanism in QM.
The function testEviction() store the given origin at first to make it become the oldest origin. Then, store four other origins to full the disk so that the eviction will be triggered by storing one byte more in another origin. At the end of this function, it checks the directory of oldest/given origin to see whether it was evicted. This test expected that if the oldest origin is not a persisted origin, then it should be evicted. If it's, then it shouldn't be evicted.
Attachment #9011830 -
Flags: review?(jvarga)
Assignee | ||
Comment 9•6 years ago
|
||
Comment on attachment 9011830 [details] [diff] [review]
Implement a test to verify the behavior of eviction.
Based on Bug 1493908 comment 6, it seems that this patch might need to use async/await as well.
Attachment #9011830 -
Flags: review?(jvarga)
Assignee | ||
Comment 10•6 years ago
|
||
Using async/await
Assignee | ||
Updated•6 years ago
|
Attachment #9011830 -
Attachment is obsolete: true
Updated•6 years ago
|
Priority: P3 → P2
Assignee | ||
Comment 11•6 years ago
|
||
Attachment #9012098 -
Attachment is obsolete: true
Assignee | ||
Comment 12•6 years ago
|
||
Attachment #9012803 -
Attachment is obsolete: true
Assignee | ||
Comment 13•6 years ago
|
||
Jan, I think this patch is ready to be reviewed. Could you review that? Thanks!
This patch adds a test to verify a temporary storage is evicted when the usage is about to exceed the limit and a persisted storage isn't in the same condition.
Attachment #9012804 -
Attachment is obsolete: true
Attachment #9012808 -
Flags: review?(jvarga)
Comment 14•6 years ago
|
||
Working on it, I'm almost done.
Comment 15•6 years ago
|
||
I think the basic idea is very simple (if the oldest origin is persisted then it shouldn't be evicted) so the test can look much simpler too.
I'll send review comments later.
Comment 16•6 years ago
|
||
Comment on attachment 9012808 [details] [diff] [review]
Implement a test to verify the behavior of eviction.
Review of attachment 9012808 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/quota/test/unit/test_eviction.js
@@ +6,5 @@
> +const groupLimitKB = 10 * 1024;
> +// The group limit is calculated as 20% of the global limit.
> +const globalLimitKB = groupLimitKB * 5;
> +
> +async function openSimpleDBWithSize(origin, size) {
The name of this function indicates that a database will be opened and returned in that state. However, it does something else. It opens a database, writes data, closes the database and returns nothing. I would rather name it fillOrigin().
@@ +11,5 @@
> + let database = getSimpleDatabase(getPrincipal(origin));
> + let request = database.open("dummyName");
> + await requestFinished(request);
> +
> + let buffer = getRandomBuffer(size);
getRandomBuffer() is very slow for big sizes, I don't think random data is important here. getBuffer() should be enough.
@@ +26,5 @@
> +}
> +
> +async function testEviction(originToTest, dirToTest, evictionExpected) {
> + info("Step 1: Storing a origin with full usage");
> + await openSimpleDBWithSize(originToTest, groupLimitKB * 1024);
This 1st origin can be filled in one loop together with those 4 origins below.
@@ +33,5 @@
> + "https://group1.example1.com",
> + "https://group2.example2.com",
> + "https://group3.example3.com",
> + "https://group4.example4.com"
> + ];
There's obviously a simple pattern, so we can dynamically create these urls in a loop.
@@ +41,5 @@
> + for (let origin of originsToFullDisk) {
> + await openSimpleDBWithSize(origin, groupLimitKB * 1024);
> + }
> +
> + info("Step 3: Attempting to trigger eviction");
"Attempting" indicates that it may or may not happen. The thing is that eviction will be triggered no matter what.
If the oldest origin is not persisted then the first origin will be evicted.
If the oldest origin is persisted then the second origin will be evicted.
@@ +42,5 @@
> + await openSimpleDBWithSize(origin, groupLimitKB * 1024);
> + }
> +
> + info("Step 3: Attempting to trigger eviction");
> + const originMaybeTriggerEviction = "http://url.to.trigger.com";
dtto
@@ +43,5 @@
> + }
> +
> + info("Step 3: Attempting to trigger eviction");
> + const originMaybeTriggerEviction = "http://url.to.trigger.com";
> + await openSimpleDBWithSize(originMaybeTriggerEviction, 1);
This 6th origin can be filled in one loop together with those 4 origins above.
@@ +49,5 @@
> + info("Step 4: Verifying if the oldest origin was evicted or not");
> + let oldestOriginDir = getRelativeFile(dirToTest);
> + ok(!oldestOriginDir.exists() == evictionExpected,
> + evictionExpected ? "The origin " + originToTest + " was evicted."
> + : "The origin " + originToTest + " was not evicted.");
It would be safer to check all origin directories.
@@ +59,5 @@
> + {
> + origin: "http://best-effort.example.com",
> + directory: "storage/default/http+++best-effort.example.com",
> + evictionExpected: true,
> + persisted: false,
persisted is redundant since it can be expressed as !evictionExpected
@@ +69,5 @@
> + persisted: true,
> + }
> + ];
> +
> + setGlobalLimit(globalLimitKB);
setGlobalLimit and resetGlobalLimit should be always paired with clear() or reset(), otherwise it won't take any effect.
::: dom/quota/test/unit/xpcshell.ini
@@ +22,5 @@
>
> [test_basics.js]
> [test_bad_origin_directory.js]
> [test_defaultStorageUpgrade.js]
> +[test_eviction.js]
I don't think this a generic test for eviction. I would name it test_persist_eviction.js
Attachment #9012808 -
Flags: review?(jvarga)
Assignee | ||
Comment 17•6 years ago
|
||
(In reply to Jan Varga [:janv] from comment #16)
Thanks for the review! I still have a lot of things to learn.
Some comments can be applied on patches in bug 1389378 and bug 1389380. I will drop the requests of them and revise them again as well.
I wanted to make this test generic so that it could be extended easily, but it made the test lose its efficacy. Also, that's no any reason to do that now, I could make them generic if it's needed in the future.
> > +async function openSimpleDBWithSize(origin, size) {
>
> The name of this function indicates that a database will be opened and
> returned in that state. However, it does something else. It opens a
> database, writes data, closes the database and returns nothing. I would
> rather name it fillOrigin().
I see. OpenXXXDatabase() does imply to return the handle of a database.
> @@ +11,5 @@
> > + let buffer = getRandomBuffer(size);
>
> getRandomBuffer() is very slow for big sizes, I don't think random data is
> important here. getBuffer() should be enough.
I see.
I saw test_simpledb.js use that, but it's because it compared the content of databases after that. I also checked the functions in "unit/head.js", but I neglected to check functions in "head-shared.js". I should search all the function name and compare the relatives before using them.
> > + await openSimpleDBWithSize(originToTest, groupLimitKB * 1024);
>
> This 1st origin can be filled in one loop together with those 4 origins
> below.
I considered about that. I was worry that it's hard to point out the certain order of origin without a magic number, but it seems that it may not be as bad as I thought.
> > + "https://group4.example4.com"
> > + ];
>
> There's obviously a simple pattern, so we can dynamically create these urls
> in a loop.
That's a good point, I should have noticed that.
> + info("Step 3: Attempting to trigger eviction");
>
> "Attempting" indicates that it may or may not happen. The thing is that
> eviction will be triggered no matter what.
> If the oldest origin is not persisted then the first origin will be evicted.
> If the oldest origin is persisted then the second origin will be evicted.
Indeed, I was trying to focus on the testing origin. I guess I should have written: "Attempting to evict XXX origin".
> @@ +42,5 @@
> > + await openSimpleDBWithSize(origin, groupLimitKB * 1024);
> > + }
> > +
> > + info("Step 3: Attempting to trigger eviction");
> > + const originMaybeTriggerEviction = "http://url.to.trigger.com";
>
> dtto
>
> @@ +43,5 @@
> > + }
> > +
> > + info("Step 3: Attempting to trigger eviction");
> > + const originMaybeTriggerEviction = "http://url.to.trigger.com";
> > + await openSimpleDBWithSize(originMaybeTriggerEviction, 1);
>
> This 6th origin can be filled in one loop together with those 4 origins
> above.
I was trying to reduce the size of buffer written into the disk.
> @@ +49,5 @@
> > + info("Step 4: Verifying if the oldest origin was evicted or not");
> > + let oldestOriginDir = getRelativeFile(dirToTest);
> > + ok(!oldestOriginDir.exists() == evictionExpected,
> > + evictionExpected ? "The origin " + originToTest + " was evicted."
> > + : "The origin " + originToTest + " was not evicted.");
>
> It would be safer to check all origin directories.
That's true.
> @@ +59,5 @@
> > + persisted: false,
>
> persisted is redundant since it can be expressed as !evictionExpected
That's correct.
> @@ +69,5 @@
> > + persisted: true,
> > + }
> > + ];
> > +
> > + setGlobalLimit(globalLimitKB);
>
> setGlobalLimit and resetGlobalLimit should be always paired with clear() or
> reset(), otherwise it won't take any effect.
I didn't call rest() at the end of this test (I assumed every test call clear() at the beginning of themselves, but I found it's problematic to assume that), but here clear() was called after that in the for-loop (or it's because it might confuse others since it's not right after that?).
> ::: dom/quota/test/unit/xpcshell.ini
> @@ +22,5 @@
> >
> > [test_basics.js]
> > [test_bad_origin_directory.js]
> > [test_defaultStorageUpgrade.js]
> > +[test_eviction.js]
>
> I don't think this a generic test for eviction. I would name it
> test_persist_eviction.js
Sure
Assignee | ||
Comment 18•6 years ago
|
||
Addressed comments, merged the inter-diff patch and modified commit message
Attachment #9012808 -
Attachment is obsolete: true
Comment 19•6 years ago
|
||
Comment on attachment 9013231 [details] [diff] [review]
Bug 1389390 - Add a test to verify a persisted origin won't be evicted; r=janv
Review of attachment 9013231 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good, thanks!
Attachment #9013231 -
Flags: review+
Assignee | ||
Comment 20•6 years ago
|
||
Comment 21•6 years ago
|
||
Pushed by shes050117@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ed04fa44d07
Add a test to verify a persisted origin won't be evicted; r=janv
Comment 22•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in
before you can comment on or make changes to this bug.
Description
•