Bug 233442 – Add SharedBufferBuilder class

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Jean-Yves Avenard [:jya]]

This is part 2 of the task started in bug 233030 "Distinguish contiguous SharedBuffer from non-contiguous one and guarantee immutability"

As discussed from https://bugs.webkit.org/show_bug.cgi?id=233030#c2 ; Analogous to String being built through a StringBuilder ; a SharedBuffer would be built through a SharedBufferBuilder

SharedBuffer objects themselves becoming immutable once created by the SharedBufferBuilder.

[Radar WebKit Bug Importer]

<rdar://problem/85693939>

[Jean-Yves Avenard [:jya]]

Created attachment 445253 [details]
SharedBufferBuilder for EWS

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Jean-Yves Avenard [:jya]]

Created attachment 445256 [details]
SharedBufferBuilder for EWS

[Jean-Yves Avenard [:jya]]

Created attachment 445259 [details]
SharedBufferBuilder for EWS

fix gtk/wpe

[Jean-Yves Avenard [:jya]]

Created attachment 445267 [details]
SharedBufferBuilder for EWS

fix windows/ios

[Fujii Hironori]

Created attachment 445386 [details]
scripthash-tests-crash-log.txt

[Jean-Yves Avenard [:jya]]

Created attachment 445465 [details]
Patch for EWS

fix intermittent crash, FetchBodyConsumer::takeData() must return null if no data was previously added.

[Jean-Yves Avenard [:jya]]

Created attachment 445505 [details]
Patch for EWS

[Jean-Yves Avenard [:jya]]

I can reproduce imported/w3c/web-platform-tests/fetch/stale-while-revalidate/stale-css.html intermittents on a clean ToT.

[Jean-Yves Avenard [:jya]]

Created attachment 445524 [details]
Patch for EWS

Fix API test and windows build

[Jean-Yves Avenard [:jya]]

Created attachment 445549 [details]
Patch for EWS

Fix intermittent crash, make passing SharedBuffer const

[Jean-Yves Avenard [:jya]]

Created attachment 445556 [details]
Patch for EWS

[Jean-Yves Avenard [:jya]]

the api-mac failure is unrelated ; none of the code path modified is hit during that run

[Jean-Yves Avenard [:jya]]

Created attachment 445636 [details]
Patch

[Jean-Yves Avenard [:jya]]

Created attachment 445705 [details]
Patch for EWS

[Jean-Yves Avenard [:jya]]

Created attachment 445706 [details]
Patch

[Jean-Yves Avenard [:jya]]

Created attachment 445707 [details]
Patch

[Jean-Yves Avenard [:jya]]

Created attachment 445708 [details]
Patch for EWS

[Jean-Yves Avenard [:jya]]

Created attachment 445709 [details]
Patch for EWS

[Jean-Yves Avenard [:jya]]

Created attachment 445711 [details]
Patch

[Jean-Yves Avenard [:jya]]

Created attachment 445826 [details]
Patch for EWS

[Jean-Yves Avenard [:jya]]

Created attachment 445827 [details]
Patch

[Jean-Yves Avenard [:jya]]

Created attachment 445832 [details]
Patch for EWS

[Jean-Yves Avenard [:jya]]

Created attachment 445833 [details]
Patch

rebase

[Jean-Yves Avenard [:jya]]

Created attachment 445836 [details]
Patch for EWS

[Jean-Yves Avenard [:jya]]

Created attachment 445837 [details]
Patch

fix gtk build (again)

[Jean-Yves Avenard [:jya]]

Created attachment 445851 [details]
Patch for EWS

[Jean-Yves Avenard [:jya]]

Created attachment 445852 [details]
Patch for EWS with no windows symbols resolution

[Jean-Yves Avenard [:jya]]

Created attachment 445922 [details]
Patch for EWS

[Jean-Yves Avenard [:jya]]

Created attachment 445925 [details]
Patch

[Jean-Yves Avenard [:jya]]

Created attachment 445926 [details]
Patch for EWS

[Jean-Yves Avenard [:jya]]

Created attachment 445942 [details]
Patch

[Jean-Yves Avenard [:jya]]

Created attachment 445949 [details]
Patch

rebase

[Jean-Yves Avenard [:jya]]

Created attachment 445950 [details]
Patch for EWS

[Jean-Yves Avenard [:jya]]

Created attachment 445996 [details]
Patch

rebase

[Jean-Yves Avenard [:jya]]

Created attachment 445997 [details]
Patch for EWS

[Jean-Yves Avenard [:jya]]

Created attachment 446021 [details]
Patch for EWS

fix api ios test

[Jean-Yves Avenard [:jya]]

Created attachment 446022 [details]
Patch

[Jean-Yves Avenard [:jya]]

Created attachment 446129 [details]
Patch for EWS

rebase following changes in bug 233030

[Jean-Yves Avenard [:jya]]

Created attachment 446130 [details]
Patch

[youenn fablet]

Comment on attachment 446130 [details]
Patch

Some nits below.
Overall, this seems good.
Two questions though:
1. In most part of the code we go from a Ref<SharedBuffer> (1 allocation) to a UniqueRef<SharedBufferBuilder> (2 allocations).
Shouldn't we just use a SharedBufferBuilder with an initiated Ref<SharedBuffer> to keep to 1 allocation?

2. I understand the difference between empty and reset but I had to look at the code to understand it.
Maybe we should just have empty (and call it reset).
If it is really important to have m_data be back to a RefPtr, I guess we could call take() or use something like builder = { }.

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

> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:350
> +    return m_buffer.take()->tryCreateArrayBuffer();

Looking at take, it creates a shared buffer if needed so we probably do not need the m_buffer check.
Maybe though it would be better for take to return a RefPtr and the caller would then do a null check?
Or have a takeAndCreateIfNeeded if both patterns are equally used?
Or have a takeAsArrayBuffer if that is used in several places.

> Source/WebCore/Modules/model-element/HTMLModelElement.cpp:152
> +    m_data.empty();

What is the difference between reset and empty?

> Source/WebCore/inspector/NetworkResourcesData.cpp:50
> +    , m_dataBuffer(makeUniqueRef<SharedBufferBuilder>())

Why making it a unique ref contrary to other places?

> Source/WebCore/inspector/NetworkResourcesData.h:125
>          RefPtr<SharedBuffer> m_buffer;

Is m_buffer needed?

> Source/WebCore/loader/ResourceLoader.cpp:79
> +    , m_resourceData { makeUniqueRef<SharedBufferBuilder>() }

Why UniqueRef? Why not unique_ptr to lazily allocate it or just a direct member variable?

> Source/WebCore/platform/SharedBuffer.h:301
> +    explicit SharedBufferBuilder(RefPtr<SharedBuffer>&& buffer)

Is this one actually needed? Ditto for the Ref version? Or do you envision in the future usage of these?
I would tend to keep the Ref<> version if so, not sure the RefPtr version is useful.

> Source/WebCore/platform/SharedBuffer.h:305
> +        if (!buffer || (buffer->hasOneRef() && !buffer->isContiguous())) {

I guess I would write it as if (!buffer) return; ...

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:544
> +    return m_data.take();

To be consistent with existing code, we should probably do:
if (!m_data)
    return nullptr;
return m_data.take();

[Jean-Yves Avenard [:jya]]

Comment on attachment 446130 [details]
Patch

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

>> Source/WebCore/inspector/NetworkResourcesData.cpp:50
>> +    , m_dataBuffer(makeUniqueRef<SharedBufferBuilder>())
> 
> Why making it a unique ref contrary to other places?

I used a UniqueRef<SharedBufferBuilder> wherever SharedBuffer were only forward declared and the SharedBufferBuilder.h header was only included in the code.

Seeing that there were a lot of efforts made to speed up compilation time, I thought it was appropriate to continue that trend.

It does make the code more confusing I agree, I will remove that.

>> Source/WebCore/inspector/NetworkResourcesData.h:125
>>          RefPtr<SharedBuffer> m_buffer;
> 
> Is m_buffer needed?

I tried the keep the existing logic as much as possible. Like above, we have hasData() and hasBufferedData() doing the same thing, this smells very fishy to me.
NetworkResourceData appears to have the ability to be assigned a SharedBuffer (via setBuffer) and has a getter (buffer())  for it. I didn't investigate to see how that was used.

>> Source/WebCore/loader/ResourceLoader.cpp:79
>> +    , m_resourceData { makeUniqueRef<SharedBufferBuilder>() }
> 
> Why UniqueRef? Why not unique_ptr to lazily allocate it or just a direct member variable?

as above, SharedBuffer was forward declared, so I made SharedBufferBuilder forward declared

>> Source/WebCore/platform/SharedBuffer.h:301
>> +    explicit SharedBufferBuilder(RefPtr<SharedBuffer>&& buffer)
> 
> Is this one actually needed? Ditto for the Ref version? Or do you envision in the future usage of these?
> I would tend to keep the Ref<> version if so, not sure the RefPtr version is useful.

This is to replace code where we had:
RefPtr<SharedBuffer> m_buffer and in the constructor:
ScriptBuffer(RefPtr<SharedBuffer>&& buffer) : m_buffer(WTFMove<buffer))


This is used to initialise an `empty` but not `null` SharedBufferBuilder
In various places of the code, there's a distinction between a RefPtr<SharedBufer> where both state (unset or empty (that is a size of 0) are handled differently to Ref<SharedBuffer> where.

I will document that a SharedBufferBuilder is really a replacement for RefPtr<SharedBuffer>

>> Source/WebCore/platform/SharedBuffer.h:305
>> +        if (!buffer || (buffer->hasOneRef() && !buffer->isContiguous())) {
> 
> I guess I would write it as if (!buffer) return; ...

I agree.

[Jean-Yves Avenard [:jya]]

Comment on attachment 446130 [details]
Patch

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

>> Source/WebCore/Modules/model-element/HTMLModelElement.cpp:152
>> +    m_data.empty();
> 
> What is the difference between reset and empty?

reset() is equivalent to doing RefPtr<SharedBuffer> buffer; buffer = nullptr;
empty() is Ref<SharedBuffer> buffer = SharedBuffer::create(); buffer->clear();

As SharedBufferBuilder is a replacement to a RefPtr<SharedBuffer> the null state has to be differentiated from the "empty" one. In future change I hope to be able to remove this distinction, but this was needed to make the change smaller and the logic easier with no chance of introducing regression.

[Jean-Yves Avenard [:jya]]

Comment on attachment 446130 [details]
Patch

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

>>> Source/WebCore/platform/SharedBuffer.h:301
>>> +    explicit SharedBufferBuilder(RefPtr<SharedBuffer>&& buffer)
>> 
>> Is this one actually needed? Ditto for the Ref version? Or do you envision in the future usage of these?
>> I would tend to keep the Ref<> version if so, not sure the RefPtr version is useful.
> 
> This is to replace code where we had:
> RefPtr<SharedBuffer> m_buffer and in the constructor:
> ScriptBuffer(RefPtr<SharedBuffer>&& buffer) : m_buffer(WTFMove<buffer))
> 
> 
> This is used to initialise an `empty` but not `null` SharedBufferBuilder
> In various places of the code, there's a distinction between a RefPtr<SharedBufer> where both state (unset or empty (that is a size of 0) are handled differently to Ref<SharedBuffer> where.
> 
> I will document that a SharedBufferBuilder is really a replacement for RefPtr<SharedBuffer>

Sorry, the proper answer to this question was that I wanted to make the constructor explicit, so I needed an implementation for both RefPtr<SharedBuffer> and Ref<SharedBuffer>

[Jean-Yves Avenard [:jya]]

Created attachment 446521 [details]
Patch for EWS

Apply comments

[Jean-Yves Avenard [:jya]]

Created attachment 446522 [details]
Patch

[Jean-Yves Avenard [:jya]]

Created attachment 446526 [details]
Patch for EWS

rebase

[Jean-Yves Avenard [:jya]]

Created attachment 446527 [details]
Patch for review

[Jean-Yves Avenard [:jya]]

Created attachment 446532 [details]
Patch for EWS

rebase 2

[Jean-Yves Avenard [:jya]]

Created attachment 446533 [details]
Patch for review

[Jean-Yves Avenard [:jya]]

Created attachment 446540 [details]
Patch for EWS

[Jean-Yves Avenard [:jya]]

Created attachment 446542 [details]
Patch for review

[youenn fablet]

Comment on attachment 446542 [details]
Patch for review

LGTM.

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

> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:335
> +    m_buffer.append(WTFMove(data));

m_buffer = WTFMove(data) might be better?

> Source/WebCore/platform/SharedBuffer.cpp:544
> +    ensureBuffer();

No need to call reset here.
m_buffer = SharedBuffer::create might be enough.

> Source/WebCore/platform/SharedBuffer.cpp:550
> +    return m_buffer.releaseNonNull();

Could be rewritten to:
return m_buffer ? m_buffer.releaseNonNull() : SharedBuffer::create();

> Source/WebCore/platform/SharedBuffer.h:314
> +        : SharedBufferBuilder(RefPtr<SharedBuffer> { WTFMove(buffer) })

Here we are doing a if(!buffer) unnecessary check.
If we want to share code, we could simply add a:
initialize(Ref<SharedBuffer>&&) that we would call here and in the above constructor (initialize(buffer.releaseNonNull()).

> Source/WebCore/platform/SharedBuffer.h:337
> +    RefPtr<SharedBuffer> get() const { return m_buffer; }

Looking a more places, I would be tempted to rename it as buffer().
Given the size of the patch, this could be done in a follow-up if you think that is valuable enough.

> Source/WebCore/platform/cf/SharedBufferCF.cpp:38
> +    ASSERT(!m_contiguous);

Is it needed? I guess this is initialised in SharedBuffer m_contiguous declaration.

> Source/WebCore/platform/graphics/gstreamer/eme/GStreamerEMEUtilities.h:61
> +            if (auto parsedPayload = extractCencIfNeeded(payload->makeContiguous()))

I am missing some other patches defining makeContiguous.
InitData is taking a RefPtr&& which does no longer seem needed, except if doing WTFMove(payload)->makeContiguous() would make it more efficient.

> Source/WebCore/workers/ScriptBuffer.h:51
> +    const SharedBuffer* buffer() const { return m_buffer.get().get(); }

.get().get() is a bit weird.
I guess the first .get() could be renamed to .buffer().
But then we would have to rename m_buffer to m_bufferBuilder.
Oh well, might be good enough for now.

> Source/WebCore/workers/WorkerFontLoadRequest.cpp:85
> +            m_data.append(*contiguousData);

As I read it, we called takeAsContiguous so m_data is null.
It seems like we could be able to optimise if we were able to call m_data constructor instead of append.

> Source/WebKit/WebProcess/WebCoreSupport/ios/WebPreviewLoaderClient.cpp:70
> +    webPage->didFinishLoadForQuickLookDocumentInMainFrame(m_buffer->take().get());

Probably s/->/./

> Source/WebKit/WebProcess/WebCoreSupport/ios/WebPreviewLoaderClient.cpp:75
> +    m_buffer->reset();

s/->/./

> Source/WebKit/WebProcess/WebCoreSupport/ios/WebPreviewLoaderClient.h:33
> +#include <wtf/UniqueRef.h>

No longer need for UniqueRef.h

[Jean-Yves Avenard [:jya]]

Created attachment 446693 [details]
Patch for EWS

[Jean-Yves Avenard [:jya]]

Created attachment 446700 [details]
Patch for EWS

[Jean-Yves Avenard [:jya]]

Created attachment 446701 [details]
Patch for landing

[Jean-Yves Avenard [:jya]]

Created attachment 446871 [details]
Patch for EWS

Rebase

[Jean-Yves Avenard [:jya]]

Created attachment 446872 [details]
Patch

Rebase

[Jean-Yves Avenard [:jya]]

Created attachment 446880 [details]
Patch for EWS

[Jean-Yves Avenard [:jya]]

Created attachment 446881 [details]
Patch for Landing

[Jean-Yves Avenard [:jya]]

Created attachment 446958 [details]
Patch for EWS

[Jean-Yves Avenard [:jya]]

Created attachment 446959 [details]
Patch for landing

[Jean-Yves Avenard [:jya]]

Created attachment 446960 [details]
Patch for EWS

[Jean-Yves Avenard [:jya]]

Created attachment 446967 [details]
Patch for EWS

rebase

[Jean-Yves Avenard [:jya]]

Created attachment 446968 [details]
Patch for Landing

rebase

[Jean-Yves Avenard [:jya]]

Created attachment 447004 [details]
Patch

fix GTK crash

[EWS]

Committed r286983 (?): <https://commits.webkit.org/r286983>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447004 [details].