Bug 115364 – [SOUP] Move default buffer handling from ResourceHandleClient to ResourceHandleSoup

• Component: Platform
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

I don't think ResourceHandleClient that is cross-platform file is the right place for the default ResourceHandle read buffer. We can make GetBuffer return 0 by default and handle it in ResourceHandleSoup, creating a default buffer when the client doesn't provide one.

[Carlos Garcia Campos]

Created attachment 200029 [details]
Patch

[Martin Robinson]

Comment on attachment 200029 [details]
Patch

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

This is a really good change! I have just a couple small questions.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:56
> -        virtual char* getBuffer(int, int*);
> +        virtual char* getOrCreateReadBuffer(size_t requestedSize, size_t& actualSize);

I wonder if this should return a PassRefPtr<SharedBuffer> which would make it more similar to the rest of the networking code and allow for removing the actualSize argument.

Another small comment is that you probably want this to be called createBuffer. It seems that the ResourceHandle is always responsible for freeing the buffer (unless I have misread this patch), so this method is essentially a constructor since it passes ownership. That's another reason why I suggested a return value wrapped in PassRefPtr above. If we had a PassGOwnPtr it would make sense here. In fact, this reminds me to investigate removing GOwnPtr completely again. :)

[Carlos Garcia Campos]

(In reply to comment #2)
> (From update of attachment 200029 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200029&action=review
> 
> This is a really good change! I have just a couple small questions.
> 
> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:56
> > -        virtual char* getBuffer(int, int*);
> > +        virtual char* getOrCreateReadBuffer(size_t requestedSize, size_t& actualSize);
> 
> I wonder if this should return a PassRefPtr<SharedBuffer> which would make it more similar to the rest of the networking code and allow for removing the actualSize argument.

This was added for performance issues when reading media streams. What we want is a pointer to a buffer that can be used with read operations (g_input_stream_read_async). A shared buffer has its own memory handling and a lot of logic we don't really need. What we want here is that the GST source element can create a GstBuffer and pass the pointer to the network layer to be used directly for reading, avoiding any intermediate allocation-copy-deallocation. I think other networks bakends receive already a buffer from their network layer, so they have to copy it anyway. In such cases a SharedBuffer is perfect to fill it with data and read it incrementally. 

> Another small comment is that you probably want this to be called createBuffer. It seems that the ResourceHandle is always responsible for freeing the buffer (unless I have misread this patch), so this method is essentially a constructor since it passes ownership. That's another reason why I suggested a return value wrapped in PassRefPtr above. If we had a PassGOwnPtr it would make sense here. In fact, this reminds me to investigate removing GOwnPtr completely again. :)

You have misread the patch :-) The method is in the client and the owner of the buffer is the client. But if the client doesn't provide a buffer, the resource handle creates its own one, and is the owner of that one, of course. Depending on how the buffer handling is implemented, the function will return a new buffer or reuse a existing one (like we do with the default buffer), that's why it's called getOrCreateReadBuffer.

[Martin Robinson]

Comment on attachment 200029 [details]
Patch

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

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:321
> +    } else if (d->m_readBufferSize < defaultReadBufferSize) {
> +        d->m_readBuffer.set(static_cast<char*>(g_malloc(defaultReadBufferSize)));
> +        d->m_readBufferPtr = d->m_readBuffer.get();
> +        d->m_readBufferSize = defaultReadBufferSize;
> +    } else
> +        d->m_readBufferPtr = d->m_readBuffer.get();

In what case is d->m_readBufferSize ever less than the defaultReadBufferSize? Reading this code, I can see only one situation, which is if getOrCreateReadBuffer returns one that is smaller than the default size. If that's the case that you're dealing with here, why not check the size instead of just the return value of bufferPtr? Likewise, I don't get the final else case. If d->m_readBuffer is allocated at all, shouldn't d->m_readBufferPtr point to it anyway -- since when you get a valid return value from getOrCreateReadBuffer you reset m_readBuffer and m_readBufferPtr. Are you expecting ResourceHandleInternal to be shared between ResourceHandles? Even in that case, the internal buffer (m_readBuffer) is always the same size.

> You have misread the patch :-) The method is in the client and the owner of the buffer is the client. But if the client doesn't provide a buffer, the resource handle creates its own one, and is the owner of that one, of course. Depending on how the buffer handling is implemented, the function will return a new buffer or reuse a existing one (like we do with the default buffer), that's why it's called getOrCreateReadBuffer.

I see now that d->m_readBuffer is not set to the return value of getOrCreateReadBuffer. The use of the word "Create" is a bit confusing here since it's used for constructors quite a bit in WebKit. Perhaps just "getReadBuffer" or "getClientReadBuffer." Another naming nit is that the distinction between readBuffer and readBufferPtr is a bit unclear. How do you feel about Cairo terminology: m_readBuffer/m_embeddedReadBuffer or m_readBufferPtr/m_embeddedReadBuffer.

[Carlos Garcia Campos]

(In reply to comment #4)
> (From update of attachment 200029 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200029&action=review
> 
> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:321
> > +    } else if (d->m_readBufferSize < defaultReadBufferSize) {
> > +        d->m_readBuffer.set(static_cast<char*>(g_malloc(defaultReadBufferSize)));
> > +        d->m_readBufferPtr = d->m_readBuffer.get();
> > +        d->m_readBufferSize = defaultReadBufferSize;
> > +    } else
> > +        d->m_readBufferPtr = d->m_readBuffer.get();
> 
> In what case is d->m_readBufferSize ever less than the defaultReadBufferSize?

The first time.

> Reading this code, I can see only one situation, which is if getOrCreateReadBuffer returns one that is smaller than the default size.

In such case the default buffer is not used. The caller is not supposed to change the size parameter unless a valid pointer is returned.

> If that's the case that you're dealing with here, why not check the size instead of just the return value of bufferPtr?

The default implementation returns NULL and the size out parameter is not changed. We only expect to receive a size when a valid pointer is returned by the client.

> Likewise, I don't get the final else case. If d->m_readBuffer is allocated at all, shouldn't d->m_readBufferPtr point to it anyway -- since when you get a valid return value from getOrCreateReadBuffer you reset m_readBuffer and m_readBufferPtr.

That's for redirections, in case of redirection the current soup request is cleaned up (and Ptr is set to 0) and a new one is started for the new request, but we still want to reuse the buffer instead of free/alloc.

> Are you expecting ResourceHandleInternal to be shared between ResourceHandles?

No.

> Even in that case, the internal buffer (m_readBuffer) is always the same size.

That's true, we could just check if the m_buffer is NULL or not like the current code does, instead of checking the size. I added the size check in case the size could change in the future, but maybe it's confusing, so I can change it to check the default buffer pointer instead.

> > You have misread the patch :-) The method is in the client and the owner of the buffer is the client. But if the client doesn't provide a buffer, the resource handle creates its own one, and is the owner of that one, of course. Depending on how the buffer handling is implemented, the function will return a new buffer or reuse a existing one (like we do with the default buffer), that's why it's called getOrCreateReadBuffer.
> 
> I see now that d->m_readBuffer is not set to the return value of getOrCreateReadBuffer. The use of the word "Create" is a bit confusing here since it's used for constructors quite a bit in WebKit. Perhaps just "getReadBuffer" or "getClientReadBuffer." Another naming nit is that the distinction between readBuffer and readBufferPtr is a bit unclear. How do you feel about Cairo terminology: m_readBuffer/m_embeddedReadBuffer or m_readBufferPtr/m_embeddedReadBuffer.

I don't really mind to change the name, but the method, creates a buffer or returns an existing one, it's pretty clear to me. Regarding the variable name, embedded sounds confusing to me. What bout m_readBufferPtr and m_defaultReadBuffer?

[Martin Robinson]

(In reply to comment #5)

> > Likewise, I don't get the final else case. If d->m_readBuffer is allocated at all, shouldn't d->m_readBufferPtr point to it anyway -- since when you get a valid return value from getOrCreateReadBuffer you reset m_readBuffer and m_readBufferPtr.
> 
> That's for redirections, in case of redirection the current soup request is cleaned up (and Ptr is set to 0) and a new one is started for the new request, but we still want to reuse the buffer instead of free/alloc.

Ah, interesting. You need to set m_readBufferPtr to null in order to trigger the creation of a new client buffer. Tricky.

> > Even in that case, the internal buffer (m_readBuffer) is always the same size.
> 
> That's true, we could just check if the m_buffer is NULL or not like the current code does, instead of checking the size. I added the size check in case the size could change in the future, but maybe it's confusing, so I can change it to check the default buffer pointer instead.

I think a NULL check would be a bit clearer, since the size applies to both the embedded/default buffer and the one from the client.

> I don't really mind to change the name, but the method, creates a buffer or returns an existing one, it's pretty clear to me. Regarding the variable name, embedded sounds confusing to me. What bout m_readBufferPtr and m_defaultReadBuffer?

m_defaultReadBuffer, m_internalReadBuffer, m_embeddedReadBuffer all sound good to me. Just so there's a distinction somehow. Thanks.

[Martin Robinson]

Comment on attachment 200029 [details]
Patch

Looks good! All my questions are answered. I'd prefer a few renames, but there's nothing blocking this patch other than that.

[Carlos Garcia Campos]

(In reply to comment #7)
> (From update of attachment 200029 [details])
> Looks good! All my questions are answered. I'd prefer a few renames, but there's nothing blocking this patch other than that.

Thanks, I'll do the renames and will change the pointer check before landing.

[Carlos Garcia Campos]

Committed r149383: <http://trac.webkit.org/changeset/149383>