Bug 143872 - [SOUP] Add initial implementation of NetworkProcess disk cache
Summary: [SOUP] Add initial implementation of NetworkProcess disk cache
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk, Soup
Depends on:
Blocks: 142821
  Show dependency treegraph
 
Reported: 2015-04-17 04:15 PDT by Carlos Garcia Campos
Modified: 2015-04-26 23:18 PDT (History)
7 users (show)

See Also:


Attachments
Patch (34.35 KB, patch)
2015-04-17 04:21 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch (34.23 KB, patch)
2015-04-20 00:59 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Another update (34.27 KB, patch)
2015-04-20 01:47 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated to use fast malloc/free (34.54 KB, patch)
2015-04-24 01:29 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Use fast malloc/free in IOChannel too (34.65 KB, patch)
2015-04-24 09:13 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (34.78 KB, patch)
2015-04-26 02:15 PDT, Carlos Garcia Campos
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2015-04-17 04:15:49 PDT
Add initial implementation of NetworkProcess disk cache for Soup.
Comment 1 Carlos Garcia Campos 2015-04-17 04:21:29 PDT
Created attachment 251014 [details]
Patch

It's enabled by default only for EWS, but I don't plan to enable it in this patch.
Comment 2 WebKit Commit Bot 2015-04-17 04:23:30 PDT
Attachment 251014 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:89:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:123:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:137:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:170:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:194:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:69:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 6 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Carlos Garcia Campos 2015-04-17 04:24:53 PDT
Found 14 tests; running 14, skipping 0.
Running 1 WebKitTestRunner.     

All 14 tests ran as expected.

All tests pass, but I haven't tested it much more than the layout tests and some navigation with the MiniBrowser to dump the cache to a json file.
Comment 4 Chris Dumez 2015-04-17 11:53:08 PDT
Comment on attachment 251014 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251014&action=review

> Source/WebKit2/NetworkProcess/cache/NetworkCacheData.h:114
> +    Data(SoupBuffer*, Backing = Backing::Buffer);

Could this be a GRefPtr<SourceBuffer>&& to make memory management clearer?

> Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:56
> +    return { buffer.get() };

then this would be a return { WTF::move(buffer) };
Comment 5 Carlos Garcia Campos 2015-04-20 00:58:15 PDT
(In reply to comment #4)
> Comment on attachment 251014 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251014&action=review
> 
> > Source/WebKit2/NetworkProcess/cache/NetworkCacheData.h:114
> > +    Data(SoupBuffer*, Backing = Backing::Buffer);
> 
> Could this be a GRefPtr<SourceBuffer>&& to make memory management clearer?

Yes.

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:56
> > +    return { buffer.get() };
> 
> then this would be a return { WTF::move(buffer) };

Yes.
Comment 6 Carlos Garcia Campos 2015-04-20 00:59:02 PDT
Created attachment 251151 [details]
Updated patch
Comment 7 WebKit Commit Bot 2015-04-20 01:02:02 PDT
Attachment 251151 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:89:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:123:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:137:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:170:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:194:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:69:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 6 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Sergio Villar Senin 2015-04-20 01:26:35 PDT
Comment on attachment 251014 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251014&action=review

Awesome stuff Carlos!

> Source/WebKit2/NetworkProcess/cache/NetworkCacheBlobStorage.cpp:34
> +#include <fcntl.h>

Why this?

> Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:29
> +#if ENABLE(NETWORK_CACHE)

Don't we need some USE(SOUP) or PLATFORM(LINUX) and the like?

> Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:129
> +    ASSERT(map && map != MAP_FAILED);

Perhaps better to split this in 2 ASSERTs in order to better spot the actual cause of failure.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:58
> +    ASSERT(m_outputStream || m_inputStream || m_ioStream);

Same here about splitting the ASSERT

> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:82
> +    }

Can't we use Data directly instead of having to copy it from a SoupBuffer?
Comment 9 Carlos Garcia Campos 2015-04-20 01:42:39 PDT
(In reply to comment #8)
> Comment on attachment 251014 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251014&action=review
> 
> Awesome stuff Carlos!

Thanks.

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheBlobStorage.cpp:34
> > +#include <fcntl.h>
> 
> Why this?

It's needed for open and its flags (O_CREAT, O_RDWR, etc.) in Linux, I guess it's not needed in mac, but doesn't hurt anyway.

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:29
> > +#if ENABLE(NETWORK_CACHE)
> 
> Don't we need some USE(SOUP) or PLATFORM(LINUX) and the like?

I assumed NETWORK_CACHE is already enabled depending on the port.

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:129
> > +    ASSERT(map && map != MAP_FAILED);
> 
> Perhaps better to split this in 2 ASSERTs in order to better spot the actual
> cause of failure.

Ok, I think I copied this one from mac impl :-)

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:58
> > +    ASSERT(m_outputStream || m_inputStream || m_ioStream);
> 
> Same here about splitting the ASSERT

Sure.

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:82
> > +    }
> 
> Can't we use Data directly instead of having to copy it from a SoupBuffer?

I'm not sure I understand the question. We create a Data with a SoupBuffer to avoid copying the data. The read soup buffer will be reused on next read, so we need to copy that data appending it to the Data.
Comment 10 Carlos Garcia Campos 2015-04-20 01:47:58 PDT
Created attachment 251154 [details]
Another update
Comment 11 WebKit Commit Bot 2015-04-20 01:50:50 PDT
Attachment 251154 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:90:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:124:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:138:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:171:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:195:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:69:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 6 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Martin Robinson 2015-04-23 10:47:42 PDT
Comment on attachment 251154 [details]
Another update

View in context: https://bugs.webkit.org/attachment.cgi?id=251154&action=review

Looks good to me, but we should chat briefly about benefits and drawbacks of using WebKit's builtin memory manager.

> Source/WebCore/platform/network/soup/GRefPtrSoup.cpp:24
> +template <> SoupBuffer* refGPtr(SoupBuffer* ptr)

Nit: Do you mind using 'buffer' here instead of 'ptr'?

> Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:41
> +    : m_buffer(adoptGRef(soup_buffer_new(SOUP_MEMORY_COPY, data, size)))

We should probably discuss whether we want to use the WebKit memory allocator.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:94
> +    uint8_t* data = static_cast<uint8_t*>(g_malloc(size));

Ditto.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:101
> +    if (bytesRead == -1) {
> +        asyncData->completionHandler(asyncData->data, -1);
> +        delete asyncData;
> +        return;
> +    }

It is annoying that GFileError cannot be used here for the error code, because the enum starts at zero. Perhaps a comment here about that could be informative.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:144
> +    gssize bytesRead;

This can move into the do-while loop, I think.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:149
> +        bytesRead = g_input_stream_read(m_inputStream.get(), const_cast<char*>(readBuffer->data), bytesToRead, nullptr, nullptr);

Same issue here with GFileError. :(

> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:181
> +    if (bytesWritten == -1) {
> +        asyncData->completionHandler(-1);
> +        delete asyncData;
> +        return;
> +    }

Ditto.
Comment 13 Carlos Garcia Campos 2015-04-23 10:57:21 PDT
(In reply to comment #12)
> Comment on attachment 251154 [details]
> Another update
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251154&action=review
> 
> Looks good to me, but we should chat briefly about benefits and drawbacks of
> using WebKit's builtin memory manager.

Please note that this is a first implementation of a feature that is disabled, it's a first step to continue working on it.

> > Source/WebCore/platform/network/soup/GRefPtrSoup.cpp:24
> > +template <> SoupBuffer* refGPtr(SoupBuffer* ptr)
> 
> Nit: Do you mind using 'buffer' here instead of 'ptr'?

Sure, we use ptr in all GRefPtrFoo implementations, though.

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:41
> > +    : m_buffer(adoptGRef(soup_buffer_new(SOUP_MEMORY_COPY, data, size)))
> 
> We should probably discuss whether we want to use the WebKit memory
> allocator.

I'm not sure I get what you mean.

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:94
> > +    uint8_t* data = static_cast<uint8_t*>(g_malloc(size));
> 
> Ditto.

Soup uses g_free unconditionally, so when it takes the memory it needs to be allocated with g_malloc(). 

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:101
> > +    if (bytesRead == -1) {
> > +        asyncData->completionHandler(asyncData->data, -1);
> > +        delete asyncData;
> > +        return;
> > +    }
> 
> It is annoying that GFileError cannot be used here for the error code,
> because the enum starts at zero. Perhaps a comment here about that could be
> informative.

It can be used, but I didn't care about that yet, I don't even know if those errors are actually handled, or just used to check whether it failed or not.

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:144
> > +    gssize bytesRead;
> 
> This can move into the do-while loop, I think.

Yes, I think the first impl used that in the while comparison.

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:149
> > +        bytesRead = g_input_stream_read(m_inputStream.get(), const_cast<char*>(readBuffer->data), bytesToRead, nullptr, nullptr);
> 
> Same issue here with GFileError. :(
> 
> > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:181
> > +    if (bytesWritten == -1) {
> > +        asyncData->completionHandler(-1);
> > +        delete asyncData;
> > +        return;
> > +    }
> 
> Ditto.
Comment 14 Martin Robinson 2015-04-23 11:23:08 PDT
Comment on attachment 251154 [details]
Another update

View in context: https://bugs.webkit.org/attachment.cgi?id=251154&action=review

>>> Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:41
>>> +    : m_buffer(adoptGRef(soup_buffer_new(SOUP_MEMORY_COPY, data, size)))
>> 
>> We should probably discuss whether we want to use the WebKit memory allocator.
> 
> I'm not sure I get what you mean.

I'm simply suggesting that it might be worth it to use bmalloc here and then to free the memory properly by using soup_buffer_new_with_owner instead.

>>> Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:94
>>> +    uint8_t* data = static_cast<uint8_t*>(g_malloc(size));
>> 
>> Ditto.
> 
> Soup uses g_free unconditionally, so when it takes the memory it needs to be allocated with g_malloc().

We would need to use soup_buffer_new_with_owner.

>>> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:101
>>> +    }
>> 
>> It is annoying that GFileError cannot be used here for the error code, because the enum starts at zero. Perhaps a comment here about that could be informative.
> 
> It can be used, but I didn't care about that yet, I don't even know if those errors are actually handled, or just used to check whether it failed or not.

Hrm. Isn't the first enum member of GFileError (G_FILE_ERROR_EXIST) defined to be 0?
Comment 15 Carlos Garcia Campos 2015-04-24 01:21:57 PDT
(In reply to comment #14)
> Comment on attachment 251154 [details]
> Another update
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251154&action=review
> 
> >>> Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:41
> >>> +    : m_buffer(adoptGRef(soup_buffer_new(SOUP_MEMORY_COPY, data, size)))
> >> 
> >> We should probably discuss whether we want to use the WebKit memory allocator.
> > 
> > I'm not sure I get what you mean.
> 
> I'm simply suggesting that it might be worth it to use bmalloc here and then
> to free the memory properly by using soup_buffer_new_with_owner instead.

Ah, ok, I thought you were suggesting to replace soup buffer somehow.

> >>> Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:94
> >>> +    uint8_t* data = static_cast<uint8_t*>(g_malloc(size));
> >> 
> >> Ditto.
> > 
> > Soup uses g_free unconditionally, so when it takes the memory it needs to be allocated with g_malloc().
> 
> We would need to use soup_buffer_new_with_owner.

Got it.

> >>> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:101
> >>> +    }
> >> 
> >> It is annoying that GFileError cannot be used here for the error code, because the enum starts at zero. Perhaps a comment here about that could be informative.
> > 
> > It can be used, but I didn't care about that yet, I don't even know if those errors are actually handled, or just used to check whether it failed or not.
> 
> Hrm. Isn't the first enum member of GFileError (G_FILE_ERROR_EXIST) defined
> to be 0?

My point is that I haven't cared about the actual error codes at this point because the caller use the error as a boolean (just to check whether it failed) or for the debug logs, so it doesn't really matter.
Comment 16 Carlos Garcia Campos 2015-04-24 01:29:44 PDT
Created attachment 251543 [details]
Updated to use fast malloc/free
Comment 17 WebKit Commit Bot 2015-04-24 01:31:44 PDT
Attachment 251543 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:90:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:124:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:138:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:170:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:194:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:71:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 6 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 Martin Robinson 2015-04-24 07:34:29 PDT
Comment on attachment 251543 [details]
Updated to use fast malloc/free

View in context: https://bugs.webkit.org/attachment.cgi?id=251543&action=review

> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:129
> +    GRefPtr<SoupBuffer> buffer = adoptGRef(soup_buffer_new(SOUP_MEMORY_TAKE, static_cast<char*>(g_malloc(bufferSize)), bufferSize));

Here it might be better to use fastMalloc as well.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:142
> +    GRefPtr<SoupBuffer> readBuffer = adoptGRef(soup_buffer_new(SOUP_MEMORY_TAKE, static_cast<char*>(g_malloc(bufferSize)), bufferSize));

Ditto.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:162
> +    size_t bytesToRead = bufferSize;
> +    do {
> +        // FIXME: implement offset.
> +        gssize bytesRead = g_input_stream_read(m_inputStream.get(), const_cast<char*>(readBuffer->data), bytesToRead, nullptr, nullptr);
> +        if (bytesRead == -1) {
> +            completionHandler(data, -1);
> +            return;
> +        }
> +
> +        if (!bytesRead)
> +            break;
> +
> +        ASSERT(bytesRead > 0);
> +        fillDataFromReadBuffer(readBuffer.get(), static_cast<size_t>(bytesRead), data);
> +
> +        pendingBytesToRead = size - data.size();
> +        bytesToRead = std::min(pendingBytesToRead, readBuffer->length);
> +    } while (pendingBytesToRead);

Sorry I missed this in the first review. I have a quick question about the synchronous read case. Why is it better to read chunks of data and constantly fill up small buffers? Why not create a buffer the full size of the data and only create a single Data object? Couldn't you avoid all of the concatenation that way?

Otherwise, I suggest a small rename here for the sake of clarity: pendingBytesToRead -> bytesToRead, bytesToRead -> bytesToReadForCurrentChunk.
Comment 19 Carlos Garcia Campos 2015-04-24 09:08:34 PDT
(In reply to comment #18)
> Comment on attachment 251543 [details]
> Updated to use fast malloc/free
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251543&action=review
> 
> > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:129
> > +    GRefPtr<SoupBuffer> buffer = adoptGRef(soup_buffer_new(SOUP_MEMORY_TAKE, static_cast<char*>(g_malloc(bufferSize)), bufferSize));
> 
> Here it might be better to use fastMalloc as well.

Right, I forgot we were also allocating memory here.

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:142
> > +    GRefPtr<SoupBuffer> readBuffer = adoptGRef(soup_buffer_new(SOUP_MEMORY_TAKE, static_cast<char*>(g_malloc(bufferSize)), bufferSize));
> 
> Ditto.

Right.

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:162
> > +    size_t bytesToRead = bufferSize;
> > +    do {
> > +        // FIXME: implement offset.
> > +        gssize bytesRead = g_input_stream_read(m_inputStream.get(), const_cast<char*>(readBuffer->data), bytesToRead, nullptr, nullptr);
> > +        if (bytesRead == -1) {
> > +            completionHandler(data, -1);
> > +            return;
> > +        }
> > +
> > +        if (!bytesRead)
> > +            break;
> > +
> > +        ASSERT(bytesRead > 0);
> > +        fillDataFromReadBuffer(readBuffer.get(), static_cast<size_t>(bytesRead), data);
> > +
> > +        pendingBytesToRead = size - data.size();
> > +        bytesToRead = std::min(pendingBytesToRead, readBuffer->length);
> > +    } while (pendingBytesToRead);
> 
> Sorry I missed this in the first review. I have a quick question about the
> synchronous read case. Why is it better to read chunks of data and
> constantly fill up small buffers? Why not create a buffer the full size of
> the data and only create a single Data object? Couldn't you avoid all of the
> concatenation that way?

Because we don't know the size, NetworkCacheStorage passes std::numeric_limits<size_t>::max() to ensure we read until end of stream. Note that this readSync method is only used for debugging, when a dump.json file is requested to be created by the user, it's not used when retrieving cache resources for loading.

> Otherwise, I suggest a small rename here for the sake of clarity:
> pendingBytesToRead -> bytesToRead, bytesToRead -> bytesToReadForCurrentChunk.
Comment 20 Carlos Garcia Campos 2015-04-24 09:13:27 PDT
Created attachment 251553 [details]
Use fast malloc/free in IOChannel too
Comment 21 WebKit Commit Bot 2015-04-24 09:16:04 PDT
Attachment 251553 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:90:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:124:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:139:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:172:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:196:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:71:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 6 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 22 Antti Koivisto 2015-04-24 10:27:55 PDT
Comment on attachment 251553 [details]
Use fast malloc/free in IOChannel too

View in context: https://bugs.webkit.org/attachment.cgi?id=251553&action=review

> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:201
> +    WriteAsyncData* asyncData = new WriteAsyncData { this, data.soupBuffer(), completionHandler };

It would be nicer and safer to use std::unique_ptr instead of explicit new/delete for memory management.

auto asyncData = std::make_unique<WriteAsyncData>(...)
Comment 23 Carlos Garcia Campos 2015-04-26 01:15:29 PDT
(In reply to comment #22)
> Comment on attachment 251553 [details]
> Use fast malloc/free in IOChannel too
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251553&action=review
> 
> > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:201
> > +    WriteAsyncData* asyncData = new WriteAsyncData { this, data.soupBuffer(), completionHandler };
> 
> It would be nicer and safer to use std::unique_ptr instead of explicit
> new/delete for memory management.
> 
> auto asyncData = std::make_unique<WriteAsyncData>(...)

I didn't do this because the pointer is passed as user data of a C callback.
Comment 24 Carlos Garcia Campos 2015-04-26 02:13:25 PDT
(In reply to comment #23)
> (In reply to comment #22)
> > Comment on attachment 251553 [details]
> > Use fast malloc/free in IOChannel too
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=251553&action=review
> > 
> > > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:201
> > > +    WriteAsyncData* asyncData = new WriteAsyncData { this, data.soupBuffer(), completionHandler };
> > 
> > It would be nicer and safer to use std::unique_ptr instead of explicit
> > new/delete for memory management.
> > 
> > auto asyncData = std::make_unique<WriteAsyncData>(...)
> 
> I didn't do this because the pointer is passed as user data of a C callback.

Well, I can adopt the pointer at the beginning of the clabback and release it before calling the read/write method method again. It's still tricky, because we can't use the wrapped pointer in the read/write call, so we need to save the buffer to a local variable and then release the unique_ptr. I'll update the patch.
Comment 25 Carlos Garcia Campos 2015-04-26 02:15:04 PDT
Created attachment 251679 [details]
Patch
Comment 26 WebKit Commit Bot 2015-04-26 02:17:54 PDT
Attachment 251679 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:90:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:124:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:139:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:172:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:197:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:71:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 6 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 27 Martin Robinson 2015-04-26 17:24:22 PDT
Comment on attachment 251679 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251679&action=review

> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:119
> +    userData = asyncData.release();

You should be able to avoid this assignment by passing ayncData.release() directly.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:192
> +    userData = asyncData.release();

Ditto.
Comment 28 Carlos Garcia Campos 2015-04-26 23:01:37 PDT
(In reply to comment #27)
> Comment on attachment 251679 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251679&action=review

Thanks for the review!

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:119
> > +    userData = asyncData.release();
> 
> You should be able to avoid this assignment by passing ayncData.release()
> directly.

Right, I moved it to make it explicit that using asyncData in the method call would end up in a crash, to avoid a future refactoring thinking, why are we using a variable for the buffer? I'll rather add a comment there, and move the release to the method call.

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:192
> > +    userData = asyncData.release();
> 
> Ditto.
Comment 29 Carlos Garcia Campos 2015-04-26 23:18:27 PDT
Committed r183387: <http://trac.webkit.org/changeset/183387>