Easier implementation of new QuotaManager clients

NEW
Unassigned

Status

()

defect
P5
normal
6 years ago
11 months ago

People

(Reporter: janv, Unassigned)

Tracking

(Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 11 obsolete attachments)

- remove IsFileServiceActivated, IsTransactionServiceActivated, AbortTransactionsForStorage, HasTransactionsForStorage from the Client interfave
- replace Client::WaitForStoragesToComplete with Client::WaitForOriginsToComplete or something similar which doesn't need to pass storages to clients
- add Client::WillShutdownIOThread(), IDB client needs to call FileService::WaitForStoragesToComplete there
- rename Client::ShutdownTransactionService to Client::ShutdownBackgroundThreads or something
- remove IDBDatabase::FromStorage
- deCOM nsIFileStorage and nsIOfflineStorage

- having IsShuttingDown() in nsIFileStorage is also not very clean
Blocks: 742822
Blocks: AsyncIDB
Depends on: 963064
Blocks: 847913
Depends on: 975696
Blocks: 961049
No longer blocks: AsyncIDB, 742822, 847913
Depends on: 980275, 860731
Depends on: 984789
Depends on: 992888
No longer depends on: 992888
Depends on: 1006485
Depends on: 1011510
Depends on: 1029209
Posted patch Patch 1: Cleanup (obsolete) — Splinter Review
Assignee: nobody → Jan.Varga
Status: NEW → ASSIGNED
Posted patch Patch 2: Sync ops fixes (obsolete) — Splinter Review
Attachment #8466187 - Attachment is patch: true
Depends on: 771288
Posted file QuotaManager.h (obsolete) —
This is how the new internal quota manager API looks like.
There are some small differences from what we originally discussed. Instead of plain ref counting of origins, I decided to use directory locks and there's no Client::FinishUp() method, because we can just wait for directory locks to be released before clearing origins. Existing directory locks for an origin are invalidated by quota manager when the origin is to be cleared. When that happens an invalidate callback is called, so a client implementation can abort running operations (for example abort IndexedDB transactions). When all transactions are finished, the client releases the lock.
This new architecture allows to abort even the process of opening/deleting of IndexedDB databases or asm.js caching.
There's one drawback though, some methods are now virtual, but only one is called in a loop, MaybeAllocateMoreSpae(). Anyway disk I/O makes everything much slower, so I don't think those extra virtual methods would slow down things much.
Posted file QuotaManager.h (obsolete) —
Attachment #8478015 - Attachment is obsolete: true
Attachment #8478535 - Flags: feedback?(bent.mozilla)
Attachment #8464030 - Attachment is obsolete: true
Posted patch initial patch (obsolete) — Splinter Review
Attachment #8466187 - Attachment is obsolete: true
Attachment #8466199 - Attachment is obsolete: true
Attachment #8467743 - Attachment is obsolete: true
Attachment #8467745 - Attachment is obsolete: true
Attachment #8478535 - Attachment is obsolete: true
Attachment #8478535 - Flags: feedback?(bent.mozilla)
Posted patch patch v1 (obsolete) — Splinter Review
Bye Bye nsIOfflineStorage and WaitForStoragesToComplete() :)

https://tbpl.mozilla.org/?tree=Try&rev=9f800e0a8593
Attachment #8489721 - Attachment is obsolete: true
Attachment #8490278 - Flags: review?(bent.mozilla)
Posted patch patch v1 (obsolete) — Splinter Review
Attachment #8490278 - Attachment is obsolete: true
Attachment #8490278 - Flags: review?(bent.mozilla)
Attachment #8497085 - Flags: review?(bent.mozilla)
Posted patch patch v1 (obsolete) — Splinter Review
Attachment #8497085 - Attachment is obsolete: true
Attachment #8497085 - Flags: review?(bent.mozilla)
Attachment #8504251 - Flags: review?(bent.mozilla)
Posted patch patch v1Splinter Review
Attachment #8504251 - Attachment is obsolete: true
Attachment #8504251 - Flags: review?(bent.mozilla)
Attachment #8506301 - Flags: review?(bent.mozilla)
Comment on attachment 8506301 [details] [diff] [review]
patch v1

Review of attachment 8506301 [details] [diff] [review]:
-----------------------------------------------------------------

Before reviewing the actual code here I'd prefer the API stick a little closer to our original design. Maybe I'm just missing the rationale for this, but:

1. I like the DirectoryLock idea, but I don't think it should have its own invalidate callback mechanism. Let's leave that on the Client API (that was FinishUp on our sketch). You can still do the thing where you wait for all DirectoryLocks to finish.
2. Why does DirectoryLock have an Unlock method? I thought the whole point was to use reference counting rather than explicit calls?
3. Why do we still have QuotaManager::EnsureOriginIsInitialized? Can't that be rolled into Open?
4. Why is the IO thread still exposed via QuotaManager public methods?
5. Why does Client need OriginClearCompleted, ReleaseIOThreadObjects, or ShutdownWorkThreads?
6. Why does Client::CalculateUsage have a UsageInfo param? Why isn't it just setting a uint64_t like we planned?
7. Why do we still have QuotaObject rather than the simpler QuotaManager::PrepareFileSizeChange?
Attachment #8506301 - Flags: review?(bent.mozilla)
Depends on: 1125102
No longer depends on: 771288
Depends on: 1130775
Is this bug still valid?
Flags: needinfo?(jvarga)
Priority: -- → P3
(In reply to Florian Bender from comment #14)
> Is this bug still valid?

Yes, we can still do more simplification as described in comment 13. However, it's not a priority right now.
Flags: needinfo?(jvarga)
Component: DOM → DOM: Quota Manager
I still want to this eventually, but there are more important things to do at the moment. Giving this a lower priority.
Priority: P3 → P5
Assignee: jvarga → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.