[Webkit-unassigned] [Bug 143872] [SOUP] Add initial implementation of NetworkProcess disk cache

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=143872

--- Comment #19 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150424/c9acaae4/attachment.html>