Bug 25436 – Refactor appCache for use in multi-process browsers

• Component: WebCore Misc.
• Status: RESOLVED WONTFIX

[Michael Nordman]

This bug is to track the first step in getting appCache working in
Chromium.  The high level plan was discussed offline with Brady, Maciej,
Anders, and Sam.

The basic idea is to have a clear split between a "front end" and a "back end"
of the appCache. The two will call each other in such a way that a proxy layer
can be added in between.

This document provides an overview of this approach.
http://docs.google.com/Doc?id=dvvc3n9_0dkfddscs
The language in that doc (at this moment) is a little stale and doesn't reflect our desire to land this in webkit sooner rather than later.

The front/back split should be relatively straight forward to pull off for the scriptable API. I have some code in progress that I'll attach to the bug report to demonstrate that part of things.

The front/back split for resource loading is less straight forward. I've been working towards an impl for chromium in which the loading from an appCache (including the loading of fallback resources) take place inside of ResourceHandle from WebCore's point of view. I have not looked closely at how to pull that part off on WebCore's side of the wire (so to speak).

[Michael Nordman]

Created attachment 29920 [details]
outline of some interfaces

Hello, The attached patch is obviously not ready for submission. I'm fishing for feedback on the general direction and the interfaces.

Some comments on how I see these classes being used in a single-process model vs a multi-process model.

When built into a single process

* the 'backend' appache code is NOT used on the 'main' webkit thread
* for the sake of discussion, lets call the thread it is used on the
'backend' webkit thread
* the single process 'frontend' classes post tasks to the backend thread
* sync apis are handled by waiting on a WaitableEvent in the frontend
classes, which the will get signaled by the backend thread when done
* async api completion is handled by posting tasks to the 'main'
thread (probably easier to setup than having the main thead wait for
multple waitable event handles in the main message loop)
* generally, backend classes running on the 'backend' thread don't
block that thread for file io. Any blocking file io work gets pushed
off to a seperate file io handling thread(s). (This is similar to what
we require of code that executes on chrome's IPC thread in the main
browser process)

When built into a multi-process

* the 'backend' code is NOT used in the renderer at all
* it is used on the IPC thread in the main browser process (and it
doesn't block that thread)
* the multiprocess 'frontend' classes send IPC message instead of
posting thread tasks (sync and async)
* the 'frontend' code is NOT used in the browser process all all

[Alexey Proskuryakov]

(In reply to comment #1)
> When built into a single process

Having a separate thread for appcache doesn't feel natural to me. It's just a part of the loader, why should it have a life of its own?

It is true that some appcache operations are rather time consuming, and it may be nice to move them to a secondary thread. Two distinct examples are loading from disk database and prefix lookup for fallback.

The patch does not include any examples of how the new interfaces will be used in loader. It is not clear if there are many calls that can be made asynchronous. E.g., ResourceLoader::load() needs to look up appcache resources synchronously before creating a ResourceHandle, and ResourceLoader::willSendRequest() needs to do the same before returning.

> When built into a multi-process

I don't know how resource loading is implemented in Chromium sufficiently well to comment. Maybe it would make sense to discuss this on the mailing list in order to determine whether this should be a model for other projects potentially using WebKit in multi-process configuration.

[Jeremy Orlow]

> I don't know how resource loading is implemented in Chromium sufficiently well
> to comment. Maybe it would make sense to discuss this on the mailing list in
> order to determine whether this should be a model for other projects
> potentially using WebKit in multi-process configuration.

One of our goals indeed is to be useful to other multi-process configurations in the future. 

As for your comment about using the mailing list, this is somewhat ironic since we've been told that it's much more helpful to have these kinds of discussions in bugs rather than the mailing list.  :-)

[Michael Nordman]

 > Having a separate thread for appcache doesn't feel natural to me. It's just a
> part of the loader, why should it have a life of its own?

The point of this refactoring is to be able to reuse the same core appcache code out of process. When these classes are used out of process, they will be running on a seperate thread. So I'd like to have the single process impl be similarly arranged.

The intricate dependencies between the 'loader' and the appcache make reuse very impractical. A big part of this proposal involves moving those points of integration (for resource loading) closer to ResourceHandle (inside of the ResourceHandle really since the cache hit detection and retrieval will actually occur on the far side of the ipc wire in chrome). 

> The patch does not include any examples of how the new interfaces will be used
> in loader. It is not clear if there are many calls that can be made
> asynchronous. E.g., ResourceLoader::load() needs to look up appcache resources
> synchronously before creating a ResourceHandle, and
> ResourceLoader::willSendRequest() needs to do the same before returning.

Right, how to use the ApplicationCacheInterceptor interface in webkit (single-process) is the crux of this (for the single-process impl). Looking for some webkit expertise to provide some insight. My best guess is something like

Introduce a new ResourceHandle class that wraps the existing ResourceHandle class (call the latter ResourceHandleRaw or something?). Have the new wrapper use the interceptor interfaces and perhaps never create the 'raw' handle or drop the raw handle in the fallback case.

Note that this work would NOT be required in order to use these classes in Chrome.


> I don't know how resource loading is implemented in Chromium sufficiently well
> to comment.

Chrome is actually pretty well setup to employ this flavor of ApplicationCacheInterceptor interface. See http://codereview.chromium.org/79036 for for how this fits in there.

> Maybe it would make sense to discuss this on the mailing list in
> order to determine whether this should be a model for other projects
> potentially using WebKit in multi-process configuration.

What jeremy said ;)

[Alexey Proskuryakov]

(In reply to comment #3)
> As for your comment about using the mailing list, this is somewhat ironic since
> we've been told that it's much more helpful to have these kinds of discussions
> in bugs rather than the mailing list.  :-)

I imagine this was probably said about a discussion of some particular bug, not of a general design issue.

[Jeremy Orlow]

(In reply to comment #5)
> (In reply to comment #3)
> > As for your comment about using the mailing list, this is somewhat ironic since
> > we've been told that it's much more helpful to have these kinds of discussions
> > in bugs rather than the mailing list.  :-)
> 
> I imagine this was probably said about a discussion of some particular bug, not
> of a general design issue.
> 

I sent an email to the list on the subject.  Back to the topic at hand...  :-)

[Michael Nordman]

> Having a separate thread for appcache doesn't feel natural to me.
>
> It is true that some appcache operations are rather time consuming, and it may
> be nice to move them to a secondary thread. Two distinct examples are loading
> from disk database and prefix lookup for fallback.

The requirement (in my proposal) that the backend thread doesn't do blocking IO (or anything that makes this thread unresponsive for an extended period) seems somewhat unnatural, its a background thread so why not let it block. But considering that this thread will be servicing requests for all pages, I think it does make sense.

New topic... tactics...

Assuming we'll proceed with a refactor involving a front/back split (details of that split aside for a moment), what's a good way to have the refactored impl co-exist with the existing impl until we're ready to make a switch. (I think its safe to say that this will take more than a single patch to pull off). Would this be acceptable?

1. Define a new ENABLE(APPLICATION_CACHE) flag for this feature distinct from ENABLE(OFFLINE_WEB_APPLICATIONS) flag. Make them be mutually exclusive.  Perhaps call it EXPERIMENTAL_APPLICATION_CACHE until enough is in place to be functional.

2. Submit the front-end classes that don't do anything yet.
2a. Land the WebApplicationCache and WebApplicationCacheContext base classes. And have the default factory method return a NOOP implementation.
2b. Rework DOMApplicationCache (and related points of integration to support event targeting and the window.applicationCache property) to call the new front-end, to respect the new ENABLE flag.

3. Start work on the back-end

[Alexey Proskuryakov]

> New topic... tactics...

For the record, I do not feel that my concerns in comment 2 have been fully addressed. Since they are not particularly concrete, this does not block refactoring, but I'd be happier if we shared a common vision about how this code should work.

I think that the best way forward is to discuss general multi-process loader architecture on the mailing list, with appcache design being a natural part of it.

[Michael Nordman]

(In reply to comment #8)

Thnx for responding Alexey.

> For the record, I do not feel that my concerns in comment 2 have been fully
> addressed.

I don't think they have either, this is not nearly settled. Honestly I'm not sure we'll have those concerns worked thru until we have some experimental code to look at that functions to some degree in single-process and multi-process browsers.

> Since they are not particularly concrete, this does not block
> refactoring, but I'd be happier if we shared a common vision about how this
> code should work.

Me too about sharing a common vision.

Glad to hear you don't want to block coding progress on resolving all of the questions. I think an initial patch as outlined in 'tactics' opens the door to some experimental coding to figure out the shape of the answer. How 'bout i put together that initial patch (for the front-end machinations) and send it out for review? WDYT?

> I think that the best way forward is to discuss general multi-process loader
> architecture on the mailing list, with appcache design being a natural part of
> it.

Ok, sounds good, i'll post something to the list and refer to this bug.

[Alexey Proskuryakov]

> some experimental coding to figure out the shape of the answer. How 'bout i put
> together that initial patch (for the front-end machinations) and send it out
> for review? WDYT?

A reviewer (myself or someone else) will need to understand a plan for how the new interfaces will fit in existing code and logic, both in multi-process and single-process cases. It is not necessary or desirable to do all the work at once, and it is of course fine to correct details of the initial design once they become clearer.

[Michael Nordman]

 > A reviewer (myself or someone else) will need to understand a plan for how the
> new interfaces will fit in existing code and logic, both in multi-process and
> single-process cases.

Lets start with just the 'frontend' pieces. The interfaces in the 'frontend' subdirectory (of the patch attached here) are used by Frame, HTMLHtmlElement, and DOMApplicationCache.

* Frame owns a WebApplicationCacheContext instance. (unlike the existing code)

* DOMApplicationCache gets a reference to WebApplicationCache via Frame.appcachecontext(), script invoked method  calls result in the corresponding call to WebApplicationCache methods. (similar to the existing code)

* DOMApplicationCache ISA WebApplicationCacheDelegate, delegate method overrides surface the corresponding event to script. (decoupled from the backend, unlike the existing code)

* HTMLHtmlElement invokes selectAppCacheWithManifest (similar to the existing appcache code)

Concrete impls of the WebApplicationCacheContext and WebApplicationCache classes serve as proxies to communicate with the 'backend'. They contain very little logic. In a single-process browser, the proxy boils down to thread messaging. In a multi-process browser, the proxy employs IPC. The settable factory method for WebApplicationCacheContext creates the appropiate impl for the hosting browser.

Anything there unclear?


Also, a potential reviewer has some extra credit work that could be done as well.
* Read thru this, http://docs.google.com/Doc?id=dvvc3n9_0dkfddscs
* Look at the patch "outline of some interfaces" patch attached to this issue
* Look at this chromium patch, http://codereview.chromium.org/79036
* Ask questions to clarify

[Michael Nordman]

Another possible way to split things that could result in greater code reuse would be to formalize the internal interface between the front and back pieces. Define that in an abstract class. Create a different impl of that interface depending.

I haven't gotten that far yet. So far I've been formalizing the interface between stuff outside of the appcache system and the appcache system itself.

[Michael Nordman]

Created attachment 30714 [details]
ApplicationCacheFrontend interface and usage

[Michael Nordman]

Created attachment 30715 [details]
ApplicationCacheFrontend interface and usage

[Michael Nordman]

Hi,

I've uploaded a first patch along these lines. There is not much meat in this patch, mostly it's about setting up to do parallel development of a new implementation of this feature set.

I've setup added a new loader/appcache2 directory to contain the new impl, and have begun svn copying things into it starting with DOMApplicationCache*. And setup a new ENABLE(APPLICATiON_CACHE) flag.

I've named the 'frontend' class ApplicationCacheFrontend, but I'm very open to suggestion on nameing. I'd like to reserve the obvious names (like ApplicationCache and ApplicationCacheContext) for use as the names of the 'backend' classes as that is where most of the 'model' logic will be.

This patch does not yet address how/where resource loading out of the appcache is handled.

This builds and the tests pass in two different environments.
1) Standard/webkit client with ENABLE(OFFLINE_WEB_APPLICATIONS) defined.
2) Chromium/webkit client with ENABLE(APPLICATION_CACHE) defined.

As far as other class names go... something along these lines is what i have in mind.

class ApplicationCacheFrontend;  // in this patch

class ApplicationCacheBackendProxy;
class ApplicationCacheFrontendProxy;

class ApplicationCacheBackend;  // i had been calling this ApplicationCacheService previously
class ApplicationCacheContext;
class ApplicationCacheRequestHandler;  // i had been calling this ApplicationCacheInterceptor previously
class ApplicationCacheGroup;
class ApplicationCacheUpdateTask;  // factored out of ApplicationCacheGroup, the update and validate logic
class ApplicationCache;
class ApplicationCacheEntry;
class ApplicationCacheDatabase;

[Michael Nordman]

Created attachment 30744 [details]
ApplicationCacheFrontend interface and usage (rev 3)

Uploaded a new patch in response to some review comments from this patches sister in chromium-land...

http://codereview.chromium.org/113554

This rev gets rid of an abstract interface per the discussion on the webkit list about avoiding virtual methods whenever possible.

[Michael Nordman]

How do I assign this bug to myself?

[Michael Nordman]

> // Called during frame navigation when a new document is committed.
> virtual void selectInitialCache(const KURL& documentURL,
>                                 ApplicationCacheID cacheDocumentWasLoadedFrom);

ooops... i missed a 'virtual' keyword

[Alexey Proskuryakov]

I still don't understand how we can use an out of process appcache with synchronous ResourceLoader code.

Maybe we should first make appcache asynchronous without splitting it into frontend and backend or moving it out of process? That would need to be done anyway, and it seems that the result of such refactoring will make it easier to define the best interface for a split appcache.

[Michael Nordman]

(In reply to comment #19)
> I still don't understand how we can use an out of process appcache with
> synchronous ResourceLoader code.

Understood. I'll point out that chrome supports the synchronous ResourceLoader interfaces today, out of process. We can use an out of process appcache in the same fashion.

There is very little in the patch I've submitted for review related to this part of things, the only bits are the additional fields added to ResourceResponseBase and ResourceRequestBase (behind the the new ENABLE(APPLICATION_CACHE) flag).

I'm looking for a review of this modest patch at this time. I realize the details of resource loading are still obscure. Not having a place to submit code (a flag to put code behind), is blocking me.

(more on resource loading later)

> Maybe we should first make appcache asynchronous without splitting it into
> frontend and backend or moving it out of process? That would need to be done
> anyway, and it seems that the result of such refactoring will make it easier to
> define the best interface for a split appcache.

There is a milestone I would like to reach sooner rather than later.
* This feature being functional in chrome.

I'm not too keen on doing a ton of work, only to then be in a position to do a ton more work prior to hitting that milestone. Every other webkit based browser has a functional (if not ideal) implementation of this feature set while Chrome is out in the cold.  If we followed the phases you outline, every other webkit based browser would have a slightly more functional appcache implmentation available to it, while Chrome would still be out in the cold.

I'd like to bring up a new implementation of this system with Chrome as a first class citizen. Let's walk and chew gum at the same time, make it async and make it accommodate a remoting layer.

The remoting layer should be a small part provided the 'backend' doesn't have insidious dependencies on Frame* and Document* (and vice versa).

------------------

I'll try again to describe what I have in mind for appcache resource loading.

Make appcahe resource loading a function of ResourceHandle and not a function of the "loader". The additional fields of ResourceRequestBase and ResourceResponseBase in the patch I've submitted for review are sufficient inputs and outputs to facilitate that. (ResourceHandle is remotable and is already remoted in chrome).

Change when the appcache is consulted during resource loads. Currently the appcache is consulted prior to consulting the memory cache. Reverse that order and make the memory cache sensitive to which appcache a cached resource was loaded from. (There would no longer be a need for the appcache to perform additional memory caching as it does in its current form).

The "loader" does not perform reads from the appcache directly, it does so indirectly thru ResourceHandle.

------------------

If that is not clear, please ask me specific questions to clarify what I'm driving at.

What I'm not clear on are the details of how the memory cache and the loader are plugged together, and how to make the memory cache sensitive to appcaches.

[Michael Nordman]

> If that is not clear, please ask me specific questions to clarify what I'm
> driving at.

Also, if you see an alternative approach, please do share.

[Alexey Proskuryakov]

Discussing this with Michael offline - I'm not convinced that ResourceHandle is the right place to have appcache logic.

[Alexey Proskuryakov]

Comment on attachment 30744 [details]
ApplicationCacheFrontend interface and usage (rev 3)

We've discussed the refactoring on IRC for some time, and I think that we have some agreement on how to move forward.

- ResourceHandle should be left unchanged, as it is primarily just an interface to platform-specific code.
- A new class should be introduced, that will incorporate both network and appcache loading. This is pretty much what Michael suggested above, in fact.
- Application cache implementation should not use global data, so that it could support Chrome's incognito mode.
- As much as possible, we should move in small steps, and avoid ifdefs that are forking the logic.

Getting appcache work on Chrome is definitely a priority. What needs to be done is figuring out what small incremental steps that are needed to get it work can be done separately. One example we could come up with was getting rid of global data in appcache - with luck, there is a path with very little divergence in logic needed at any point (and once Chrome works, further refactoring will remove the rest).

[Michael Nordman]

(In reply to comment #23)
> (From update of attachment 30744 [details] [review])
> We've discussed the refactoring on IRC for some time, and I think that we have
> some agreement on how to move forward.
> 
> - ResourceHandle should be left unchanged, as it is primarily just an interface
> to platform-specific code.
> - A new class should be introduced, that will incorporate both network and
> appcache loading. This is pretty much what Michael suggested above, in fact.
> - Application cache implementation should not use global data, so that it could
> support Chrome's incognito mode.
> - As much as possible, we should move in small steps, and avoid ifdefs that are
> forking the logic.
> 
> Getting appcache work on Chrome is definitely a priority. What needs to be done
> is figuring out what small incremental steps that are needed to get it work can
> be done separately. One example we could come up with was getting rid of global
> data in appcache - with luck, there is a path with very little divergence in
> logic needed at any point (and once Chrome works, further refactoring will
> remove the rest).

Alexey may have overstated the amount of agreement we have on some points :) In summary we agreed on what to build, but not on the steps to take to build it.

We have been discussing this on IRC/email for the last few days, a few hours total. We did agree in principal on the end result, the design of the resulting code. I'm real happy about that much. Until earlier today, I have had little indication of whether where I'm trying to go with this would be acceptable or not.

We don't entirely agree on the blocking and tackling tactics (sequencing of the work) to get there.

Alexey is lobbying for incremental steps the move the current code base to the new design. I'm lobbying for an initially clean slate, and incrementally putting new code, and branching existing code with modifications, onto the new slate. There are pros and cons each way.

Cons to the incremental approach.

* I would have to study whats there and plot a course (sequence of patches) that maintains functional parity with the current system every step of the way. I haven't done that and I'm not sure how twisty or turny such a course would be. I do think it will take some doing just to figure out what the options are. Time and effort that could be spent more productively going in a straight line starting from a clean slate. Honestly, I'm not sure what the first step along this path should be?

* Each incremental change puts at risk existing users of this feature. Functional parity and stability should be maintained at every step of the way... but will that be achieved... bugs happen?

* While the refactoring is happening, other unrelated mods will be happening too. A big part of the refactoring will be removing dependencies between appcache core classes and other webcore classes. This could be like a game of whack-a-mole depending on what else is going on in the interim.

* To my way of thinking, this is an extra layer of complexity that doesn't need to be there. Its an artificial constraint.

* Chrome has nothing on this feature yet. Every other webkit based browser has a functional appcache. Even a barely functional and buggy cut at this in chrome is of value. Getting to this milestone sooner rather than later would be good for the community. The incremental approach (calling for full functionality and stability at every step of the way) is at odds with this goal of getting chrome something usable (if not of shippable quality) quickly.

Cons to the clean-slate approach

* Context switch when doing code reviews.

* The need to resolve conflicts with incompatible changes because many others will not be compiling with the new flag.

Anyway... that's where we left our discussion. I'm not convinced the extra complexity is a good thing, it adds a new dimension to the puzzle that needs additional solving.


 

[Jeremy Orlow]

Something that might be a con of the clean slate approach is that the final product will probably be different in some non-essential ways from the current AppCache implementation.  This will make it harder for others to make changes in the future.

Overall, I can DEFINITELY empathize.  I think I could have had a pretty good LocalStorage implementation done by now had I started from scratch.  Instead, I had to spend a LOT of time studying what's there now.  And I've been hit by pretty much every issue you mentioned in the cons for the incremental approach.

(In reply to comment #24)
> * I would have to study whats there and plot a course (sequence of patches)
> that maintains functional parity with the current system every step of the way.
> I haven't done that and I'm not sure how twisty or turny such a course would
> be. I do think it will take some doing just to figure out what the options are.
> Time and effort that could be spent more productively going in a straight line
> starting from a clean slate. Honestly, I'm not sure what the first step along
> this path should be?

This definitely was an issue for me.  The only way I could move forward was by writing a lot of throw-away code to basically prototype everything I was planning on doing.  I've even stopped and thrown away code as I write each patch because I realize things are getting too complicated and I need to break things into smaller functional parts.

And I say all of this knowing that LocalStorage is a much simpler and more atomic chunk of code than AppCache.  Taking everything into account, I actually think Michael's approach is probably the best one here.

[Alexey Proskuryakov]

Another option that wasn't mentioned yet is to create a development branch in the WebKit repository - that way, the trunk won't be polluted with branched code.

[Michael Nordman]

Created attachment 31726 [details]
ApplicationCacheFrontent + ApplicationCacheBridge

[Michael Nordman]

Hello, here's an update on this.

I've continued working down a straight line approach to help flesh out details and hopefully inform me of a way to move this into the project more incrementatlly. The newly uploaded patch shows that work. New in the patch is an ApplicationCacheBridge interface and concrete impl for use in a single-process browser. The 'frontend' talks thru the 'bridge' to the 'backend'. In chrome, the bridge is an ipc layer.

I've also been working out what series of incrementation steps will get this feature where it wants to go. I don't have the path from start to finish mapped out, but i do think i see some initial steps to take. The 'fat' resource handle is key, so I've been focusing on that more recently.

1) v8 bindings for DOMApplicationCache, uttlerly independent of common code, can go in anytime

2) ResourceHandle 'interceptor' infrastructure. I've spun off a new bug for this specifically, https://bugs.webkit.org/show_bug.cgi?id=26655.

3) TBD

[Michael Nordman]

(In reply to comment #26)
> Another option that wasn't mentioned yet is to create a development branch in
> the WebKit repository - that way, the trunk won't be polluted with branched
> code.

This individual feature isn't worth a branch. That would complicate chrome/webkit integration workflow considerably. The chrome team just spent a lot of time unbranching/forking, we want to work on tip-of-tree.

[Michael Nordman]

Closing...  turns out this was a big digression... we're (chrome and safari) not going to share a common implementation after all... at least not for the foreseeable future.

In lieu of having a common impl, we're putting in place an interface that the two impls can both live behind. I've opened a seperate bug to track that work and patches.
https://bugs.webkit.org/show_bug.cgi?id=27821

The conclusion we got to is reasonable, the time taken to reach that conclusion was not.