Bug 98541 – Switch CachedResource over from SharedBuffer to a new ResourceBuffer

• Component: Page Loading
• Status: RESOLVED FIXED

[Brady Eidson]

Switch CachedResource over from SharedBuffer to a new ResourceBuffer

The ResourceBuffer class is a shared data buffer - much like SharedBuffer - explicitly meant for the resource cache.

From the start it will be a simple wrapper around SharedBuffer and will represent an in-process data buffer.

But its purpose is to be the foundation for a buffer than can optionally be hosted differently from a SharedBuffer - Such as shared memory.

[Adam Barth]

Perhaps we should discuss these changes on webkit-dev first?  I'd be interested in know what changes you're planning on making to WebCore.

[Brady Eidson]

(In reply to comment #1)
> Perhaps we should discuss these changes on webkit-dev first?  I'd be interested in know what changes you're planning on making to WebCore.

The patch I'm preparing for this particular bug is straight forward.  It adds a new layer on top of SharedBuffer with zero change in behavior for ports that don't want it (which I assume is all ports besides WK2 ports that opt in to the feature.)

[Adam Barth]

I guess I'm interested in the bigger picture of what changes you'll be making to WebCore in this area.  In particular, I'd like a chance to convince you that a shared memory cache is overengineering.  :)

[Brady Eidson]

(In reply to comment #3)
> I guess I'm interested in the bigger picture of what changes you'll be making to WebCore in this area.  In particular, I'd like a chance to convince you that a shared memory cache is overengineering.  :)

Some refactoring to WebCore boils down to almost direct replacement-style refactoring and should bother few folks, if any.  Once such patch should be coming to this bug soon and I have a few others in the pipeline right behind it.

There's almost certainly going to have to be some sort of structural changes to the CachedResource/ResourceLoader family of classes.  I think everyone can agree that the WebCore/loader directory is a mess of code and needs some love anyways.  So it follows we intend for such refactoring to be an opportunity to clean the code, not just shoehorn in what we need.

As far as convincing us a shared memory cache is over-engineering...  we simply have data that shows it's a win with some basic process models.

If you have data that shows it is *NOT* a win, that'd be super interesting.

[Adam Barth]

I would prefer to discuss this topic on webkit-dev since the scope is larger than this one bug.  In particular, I'd be interested in seeing the data you refer to.

[Brady Eidson]

Created attachment 167606 [details]
Patch v1 - Check EWS

[Brady Eidson]

Since we've reached agreement on webkit-dev on the nature of this effort...  Moving forward with the ResourceBuffer refactoring.  r?

[Brady Eidson]

And I'll start with a new patch since this one apparently isn't applying.

[Brady Eidson]

Created attachment 167843 [details]
Patch v2 - Updated and should apply/build.

[Build Bot]

Comment on attachment 167843 [details]
Patch v2 - Updated and should apply/build.

Attachment 167843 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14245240

[Brady Eidson]

Created attachment 167873 [details]
Patch v3 - Try to fix WebKit/win build error

[Build Bot]

Comment on attachment 167873 [details]
Patch v3 - Try to fix WebKit/win build error

Attachment 167873 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14244397

[Brady Eidson]

Created attachment 167887 [details]
Patch v4 - More windows build fixing

[Build Bot]

Comment on attachment 167887 [details]
Patch v4 - More windows build fixing

Attachment 167887 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14256009

New failing tests:
http/tests/misc/location-origin.html

[Brady Eidson]

(In reply to comment #14)
> (From update of attachment 167887 [details])
> Attachment 167887 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/14256009
> 
> New failing tests:
> http/tests/misc/location-origin.html

Seems like a real failure I can reproduce with *only* this patch rolled in, as opposed to the 2 others I have built on top of it.  Will be an easy fix, working on it now.

[Brady Eidson]

(In reply to comment #15)
> (In reply to comment #14)
> > (From update of attachment 167887 [details] [details])
> > Attachment 167887 [details] [details] did not pass mac-ews (mac):
> > Output: http://queues.webkit.org/results/14256009
> > 
> > New failing tests:
> > http/tests/misc/location-origin.html
> 
> Seems like a real failure I can reproduce with *only* this patch rolled in, as opposed to the 2 others I have built on top of it.  Will be an easy fix, working on it now.

Nevermind, can't reproduce...  Digging into the crash log on the bots.

[Brady Eidson]

Created attachment 168024 [details]
Patch v5

I could reproduce.  Fixed, and this patch should be final.

[Brady Eidson]

http://trac.webkit.org/changeset/130947