771420 - Don't write database until an new offline cache entry has been downloaded and deactivated.

Status: RESOLVED WONTFIX

(Core :: Networking: Cache, defect)

Description

[Thinker Li [:sinker]]

For most offline cache update request, they actually don't do anything but download the manifest file.  For the cases, the manifest that already in the cache is up-to-date, no any real update will be performed.  It causes needless entry creations and deletions.  By postponing entry creations until deactivating them, we can avoid these overhead.

Comment 1

[Andreas Gal :gal]

How is the patch coming along for this?

Comment 2

[Thinker Li [:sinker]]

(In reply to Andreas Gal :gal from comment #1)
> How is the patch coming along for this?

I just start it last afternoon.  I will upload a patch later.

Comment 3

[Honza Bambas (:mayhemer)]

Thinker, this is a good idea.  Just make sure in your patch the manifest gets properly cached in the new cache version when it has changed.

Comment 4

[Thinker Li [:sinker]]

Attachment: WIP

test_changeManifest.html is failed.  I will look into it later.

Comment 5

[Thinker Li [:sinker]]

Attachment: WIP v2

Fix the issue of test_changeManifest.html.  DB IO is doing in IO thread, it is not sync with main thread.  So, offlinecacheupdate may be completed before DB IO was done.

Comment 6

[Thinker Li [:sinker]]

Attachment: Postponing creation of DB record for new caches

Keep all IO requests in an NewCacheInfo for the cache entries of a new cache without running them.  These queued requests were only ran for activating the cache.

Comment 7

[Thinker Li [:sinker]]

Attachment: Postponing creation of DB record for new caches, v2

fix a typo

Comment 8

[Thinker Li [:sinker]]

Attachment: Postponing creation of DB record for new caches, v2

bit-roted, rebase to the tip.

Comment 9

[Honza Bambas (:mayhemer)]

I started the review, please don't obsolete the patch.

Thinker, can you please add an outline of what the patch changes, and mainly adds, and also why?  I see a lot of new code added in the patch w/o a single line of any explanation in the bug.  Thanks.

Also, I thought this bug was about absolutely different thing you wanted to do.  I thought you wanted to delay creation of the new cache only after we checked the manifest and it had actually changed - that is how I read the description.  So, it is not clear to me at all what the patch is actually doing.

Comment 10

[Thinker Li [:sinker]]

The major change of this patch is keep information of new cache in an instance of NewCacheInfo.  It includes cache entries and DB requests.  Information of cache entries are kept by holding their nsOfflineCacheBinding.  DB requests are queued by introducing LazyIOQueue, LazyIOStatement, ..., etc.  For nsOfflineCacheDevice, I add 3 set of functions, to manage NewCacheInfo, to track size of new cache entries, and to manage LazyIO*.  Most modifications in existing code, before this patch, are based on these functions.

The set for managing NewCacheInfo is used to create a new instance for every cache returned by nsOfflineCacheDevice::CreateApplicationCache().  The instance of a cache would be free as the cache is Activated or EvictEntries(), aka doomed.  This set maintain a map from clientIDs to instances of NewCacheInfo.  In another word, by checking this map, we can tell if a cache is a new cache returned by CreateApplicationCache().

The set for managing LazyIO create LazyIOStatements for DB requests that may should be queued and postponed.  LazyIOStatement implements a wrapper class of mozIStorageStatement.  According a cache is new or not, by checking existing NewCacheInfo against clientID, it decides to queue the statement or to execute the statement immediately when a LazyIOStatement were committing, calling CommitLazyIOStatement().  The queued statements are executed when the cache was being activated, or they are dropped if the cache was evicted.

The set for tracking cache size track a total size for every new cache returned by CreateApplicationCache().  It makes sure CacheSize() always return a correct number even new caches was not wrote to database.

nsIApplicationCache::ActivateAsync() was added for executing statements in Cache IO Thread.

The signature of nsOfflineCacheDevice::SetParentDirectory() was changed for returning right value from nsIApplicationCache::cacheDirectory.  According what is said in IDL, it was set only if the cache is placed in different directory from current profile.  So, only custom caches should return a value.  Without this change, the cache device was created for multiple times since cache service think it was a custom cache, cacheDirectory was not null.  With this mistaken, method calls on offlinecache, not custom cache, were dispatched to different instance according some random rules.

nsOfflineCacheBinding::mTypeBits is added for making nsApplicationCache::GetTypes() work.  nsOfflinecacheUpdate use this function to check if an entry was doomed.

Comment 11

[Thinker Li [:sinker]]

(In reply to Honza Bambas (:mayhemer) from comment #9)
> Also, I thought this bug was about absolutely different thing you wanted to
> do.  I thought you wanted to delay creation of the new cache only after we
> checked the manifest and it had actually changed - that is how I read the
> description.  So, it is not clear to me at all what the patch is actually
> doing.
It is exactly what I have done in the patch.  But,postponing database writing introduces other issues, or hits old bugs.  So, I did more changes.

Comment 12

[Honza Bambas (:mayhemer)]

I have to repeat a second time: before you write so much code, first ask if you are on the right path.


We both agree the goal is to not create a new offline cache sooner then we find out the manifest has actually changed, right?

Based on that, I would expect a simple change to first fetch the manifest, check content (actually hash) and if different, create a new app cache (CreateApplicationCache()) and start the usual offline cache update process.

When I see the patch and read comment 10, I am not convinced we need all this huge machinery at all.

Thinker, rather please reconsider if this simple goal could not be done in much simpler way.  Or rephrase the bugs description, maybe I just still don't understand what you are doing.

Also, not to pollute this bug with many comments, we may meet on IRC or just email me on bugzilla mail.  We should talk first outbound about the patch.

Thanks.

Comment 13

[Honza Bambas (:mayhemer)]

So, just to post some of my opinions here, thought I didn't find time to check the patch more deeply:

- I don't think that caching statements this way is what we should do
- I more tend to use sqlite transaction for this, also since the update is actually a transaction it self
- if we want this in-memory caching do our self (because of e.g. a still bad performance or problems with table locking during transaction) then it could be part of mozstorage code as a generic tool
- a small change like first checking just the manifest and keep it in in-memory HTTP cache for the following fetch-to-offline-cache load when we detect it has changed could be enough to gain the performance benefit here

Comment 14

[Honza Bambas (:mayhemer)]

Thinker, how actual is the current patch?  I don't remember exactly what we have deal on the last time to move on with this bug.

Just want to let you know that we have a new API that only checks for updates.  I think it could be used internally to precheck for manifest change and save us a lot of performance by avoiding creation of a new app cache.  It is happening in bug 751754.

Comment 15

[Thinker Li [:sinker]]

You don't want to postpone all I/O for all files being downloaded.  Postpone I/O for manifest is enough.  So, I need two days to revise current patch and make it available for review.

Comment 16

[Honza Bambas (:mayhemer)]

(In reply to Thinker Li [:sinker] from comment #15)
> You don't want to postpone all I/O for all files being downloaded.  Postpone
> I/O for manifest is enough.  So, I need two days to revise current patch and
> make it available for review.

Cool.  Please first look at the patch in bug 751754 though.  We may be OK with it for now.

Can you please outline, what you want to do in those two days before you start?

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Should we try to finish up the work here and get in for v1?

It seems like there's a non-trivial number of apps out there that actually use the appcache which is great, but also means that this bug is more important.

*If* we try to fix this for v1 we need to come up with as simple of a patch as possible. We've already branched for aurora and since this code is shared between Firefox desktop and B2G, we have to keep any work very low-risk.

Honza mentioned that the patch in bug 751754 should make it easier to come up with a lower-risk patch here.

Comment 18

[Honza Bambas (:mayhemer)]

Comment on attachment 646815 [details] [diff] [review]
Postponing creation of DB record for new caches, v2

dropping r since a newer approach is planned.