Bug 233442

Summary: Add SharedBufferBuilder class
Product: WebKit Reporter: Jean-Yves Avenard [:jya] <jean-yves.avenard>
Component: WebCore Misc.Assignee: Jean-Yves Avenard [:jya] <jean-yves.avenard>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, beidson, benjamin, berto, calvaris, cdumez, cgarcia, changseok, eric.carlson, esprehn+autocc, ews-watchlist, galpeter, glenn, gustavo, gyuyoung.kim, hi, Hironori.Fujii, hta, japhet, jbedard, jer.noble, joepeck, jsbell, kangil.han, macpherson, menard, mifenton, mmaxfield, pangle, philipj, pnormand, sergio, tommyw, vjaquez, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 233030    
Bug Blocks: 233922, 234215, 233677    
Attachments:
Description Flags
SharedBufferBuilder for EWS
ews-feeder: commit-queue-
SharedBufferBuilder for EWS
ews-feeder: commit-queue-
SharedBufferBuilder for EWS
none
SharedBufferBuilder for EWS
none
scripthash-tests-crash-log.txt
none
Patch for EWS
none
Patch for EWS
ews-feeder: commit-queue-
Patch for EWS
none
Patch for EWS
ews-feeder: commit-queue-
Patch for EWS
none
Patch
none
Patch for EWS
none
Patch
none
Patch
none
Patch for EWS
none
Patch for EWS
none
Patch
none
Patch for EWS
none
Patch
none
Patch for EWS
ews-feeder: commit-queue-
Patch
none
Patch for EWS
none
Patch
none
Patch for EWS
none
Patch for EWS with no windows symbols resolution
none
Patch for EWS
none
Patch
none
Patch for EWS
none
Patch
none
Patch
none
Patch for EWS
none
Patch
none
Patch for EWS
none
Patch for EWS
none
Patch
none
Patch for EWS
none
Patch
none
Patch for EWS
none
Patch
none
Patch for EWS
ews-feeder: commit-queue-
Patch for review
none
Patch for EWS
none
Patch for review
none
Patch for EWS
none
Patch for review
none
Patch for EWS
none
Patch for EWS
none
Patch for landing
none
Patch for EWS
ews-feeder: commit-queue-
Patch
none
Patch for EWS
none
Patch for Landing
none
Patch for EWS
none
Patch for landing
none
Patch for EWS
ews-feeder: commit-queue-
Patch for EWS
none
Patch for Landing
none
Patch none

Description Jean-Yves Avenard [:jya] 2021-11-23 00:32:45 PST
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.
Comment 1 Radar WebKit Bug Importer 2021-11-23 00:33:16 PST
<rdar://problem/85693939>
Comment 2 Jean-Yves Avenard [:jya] 2021-11-28 23:55:55 PST
Created attachment 445253 [details]
SharedBufferBuilder for EWS
Comment 3 EWS Watchlist 2021-11-28 23:56:58 PST
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
Comment 4 Jean-Yves Avenard [:jya] 2021-11-29 00:45:01 PST
Created attachment 445256 [details]
SharedBufferBuilder for EWS
Comment 5 Jean-Yves Avenard [:jya] 2021-11-29 02:01:04 PST
Created attachment 445259 [details]
SharedBufferBuilder for EWS

fix gtk/wpe
Comment 6 Jean-Yves Avenard [:jya] 2021-11-29 04:58:21 PST
Created attachment 445267 [details]
SharedBufferBuilder for EWS

fix windows/ios
Comment 7 Fujii Hironori 2021-11-29 23:41:14 PST
Created attachment 445386 [details]
scripthash-tests-crash-log.txt
Comment 8 Jean-Yves Avenard [:jya] 2021-11-30 13:39:48 PST
Created attachment 445465 [details]
Patch for EWS

fix intermittent crash, FetchBodyConsumer::takeData() must return null if no data was previously added.
Comment 9 Jean-Yves Avenard [:jya] 2021-11-30 19:39:43 PST
Created attachment 445505 [details]
Patch for EWS
Comment 10 Jean-Yves Avenard [:jya] 2021-11-30 21:46:08 PST
I can reproduce imported/w3c/web-platform-tests/fetch/stale-while-revalidate/stale-css.html intermittents on a clean ToT.
Comment 11 Jean-Yves Avenard [:jya] 2021-11-30 22:27:16 PST
Created attachment 445524 [details]
Patch for EWS

Fix API test and windows build
Comment 12 Jean-Yves Avenard [:jya] 2021-12-01 04:08:07 PST
Created attachment 445549 [details]
Patch for EWS

Fix intermittent crash, make passing SharedBuffer const
Comment 13 Jean-Yves Avenard [:jya] 2021-12-01 04:33:17 PST
Created attachment 445556 [details]
Patch for EWS
Comment 14 Jean-Yves Avenard [:jya] 2021-12-01 14:27:33 PST
the api-mac failure is unrelated ; none of the code path modified is hit during that run
Comment 15 Jean-Yves Avenard [:jya] 2021-12-01 16:57:52 PST
Created attachment 445636 [details]
Patch
Comment 16 Jean-Yves Avenard [:jya] 2021-12-02 04:14:41 PST
Created attachment 445705 [details]
Patch for EWS
Comment 17 Jean-Yves Avenard [:jya] 2021-12-02 04:15:21 PST
Created attachment 445706 [details]
Patch
Comment 18 Jean-Yves Avenard [:jya] 2021-12-02 04:26:06 PST
Created attachment 445707 [details]
Patch
Comment 19 Jean-Yves Avenard [:jya] 2021-12-02 04:27:44 PST
Created attachment 445708 [details]
Patch for EWS
Comment 20 Jean-Yves Avenard [:jya] 2021-12-02 04:35:28 PST
Created attachment 445709 [details]
Patch for EWS
Comment 21 Jean-Yves Avenard [:jya] 2021-12-02 04:36:12 PST
Created attachment 445711 [details]
Patch
Comment 22 Jean-Yves Avenard [:jya] 2021-12-03 03:06:10 PST
Created attachment 445826 [details]
Patch for EWS
Comment 23 Jean-Yves Avenard [:jya] 2021-12-03 03:06:48 PST
Created attachment 445827 [details]
Patch
Comment 24 Jean-Yves Avenard [:jya] 2021-12-03 03:40:39 PST
Created attachment 445832 [details]
Patch for EWS
Comment 25 Jean-Yves Avenard [:jya] 2021-12-03 03:41:44 PST
Created attachment 445833 [details]
Patch

rebase
Comment 26 Jean-Yves Avenard [:jya] 2021-12-03 04:01:15 PST
Created attachment 445836 [details]
Patch for EWS
Comment 27 Jean-Yves Avenard [:jya] 2021-12-03 04:02:04 PST
Created attachment 445837 [details]
Patch

fix gtk build (again)
Comment 28 Jean-Yves Avenard [:jya] 2021-12-03 07:28:01 PST
Created attachment 445851 [details]
Patch for EWS
Comment 29 Jean-Yves Avenard [:jya] 2021-12-03 07:43:27 PST
Created attachment 445852 [details]
Patch for EWS with no windows symbols resolution
Comment 30 Jean-Yves Avenard [:jya] 2021-12-03 17:01:24 PST
Created attachment 445922 [details]
Patch for EWS
Comment 31 Jean-Yves Avenard [:jya] 2021-12-03 17:02:42 PST
Created attachment 445925 [details]
Patch
Comment 32 Jean-Yves Avenard [:jya] 2021-12-03 17:04:11 PST
Created attachment 445926 [details]
Patch for EWS
Comment 33 Jean-Yves Avenard [:jya] 2021-12-03 19:52:34 PST
Created attachment 445942 [details]
Patch
Comment 34 Jean-Yves Avenard [:jya] 2021-12-03 20:14:45 PST
Created attachment 445949 [details]
Patch

rebase
Comment 35 Jean-Yves Avenard [:jya] 2021-12-03 20:18:26 PST
Created attachment 445950 [details]
Patch for EWS
Comment 36 Jean-Yves Avenard [:jya] 2021-12-05 21:52:35 PST
Created attachment 445996 [details]
Patch

rebase
Comment 37 Jean-Yves Avenard [:jya] 2021-12-05 21:53:53 PST
Created attachment 445997 [details]
Patch for EWS
Comment 38 Jean-Yves Avenard [:jya] 2021-12-06 03:31:59 PST
Created attachment 446021 [details]
Patch for EWS

fix api ios test
Comment 39 Jean-Yves Avenard [:jya] 2021-12-06 03:32:55 PST
Created attachment 446022 [details]
Patch
Comment 40 Jean-Yves Avenard [:jya] 2021-12-06 23:24:43 PST
Created attachment 446129 [details]
Patch for EWS

rebase following changes in bug 233030
Comment 41 Jean-Yves Avenard [:jya] 2021-12-06 23:26:44 PST
Created attachment 446130 [details]
Patch
Comment 42 youenn fablet 2021-12-09 01:21:18 PST
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();
Comment 43 Jean-Yves Avenard [:jya] 2021-12-09 02:12:40 PST
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.
Comment 44 Jean-Yves Avenard [:jya] 2021-12-09 03:35:18 PST
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.
Comment 45 Jean-Yves Avenard [:jya] 2021-12-09 03:38:07 PST
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>
Comment 46 Jean-Yves Avenard [:jya] 2021-12-09 04:54:32 PST
Created attachment 446521 [details]
Patch for EWS

Apply comments
Comment 47 Jean-Yves Avenard [:jya] 2021-12-09 04:55:40 PST
Created attachment 446522 [details]
Patch
Comment 48 Jean-Yves Avenard [:jya] 2021-12-09 05:30:54 PST
Created attachment 446526 [details]
Patch for EWS

rebase
Comment 49 Jean-Yves Avenard [:jya] 2021-12-09 05:31:58 PST
Created attachment 446527 [details]
Patch for review
Comment 50 Jean-Yves Avenard [:jya] 2021-12-09 06:01:28 PST
Created attachment 446532 [details]
Patch for EWS

rebase 2
Comment 51 Jean-Yves Avenard [:jya] 2021-12-09 06:04:08 PST
Created attachment 446533 [details]
Patch for review
Comment 52 Jean-Yves Avenard [:jya] 2021-12-09 06:42:14 PST
Created attachment 446540 [details]
Patch for EWS
Comment 53 Jean-Yves Avenard [:jya] 2021-12-09 06:43:33 PST
Created attachment 446542 [details]
Patch for review
Comment 54 youenn fablet 2021-12-09 13:47:12 PST
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
Comment 55 Jean-Yves Avenard [:jya] 2021-12-10 02:24:36 PST
Created attachment 446693 [details]
Patch for EWS
Comment 56 Jean-Yves Avenard [:jya] 2021-12-10 03:14:50 PST
Created attachment 446700 [details]
Patch for EWS
Comment 57 Jean-Yves Avenard [:jya] 2021-12-10 03:19:18 PST
Created attachment 446701 [details]
Patch for landing
Comment 58 Jean-Yves Avenard [:jya] 2021-12-11 04:16:23 PST
Created attachment 446871 [details]
Patch for EWS

Rebase
Comment 59 Jean-Yves Avenard [:jya] 2021-12-11 04:18:45 PST
Created attachment 446872 [details]
Patch

Rebase
Comment 60 Jean-Yves Avenard [:jya] 2021-12-11 06:06:43 PST
Created attachment 446880 [details]
Patch for EWS
Comment 61 Jean-Yves Avenard [:jya] 2021-12-11 06:11:53 PST
Created attachment 446881 [details]
Patch for Landing
Comment 62 Jean-Yves Avenard [:jya] 2021-12-12 19:03:07 PST
Created attachment 446958 [details]
Patch for EWS
Comment 63 Jean-Yves Avenard [:jya] 2021-12-12 19:04:24 PST
Created attachment 446959 [details]
Patch for landing
Comment 64 Jean-Yves Avenard [:jya] 2021-12-12 19:34:26 PST
Created attachment 446960 [details]
Patch for EWS
Comment 65 Jean-Yves Avenard [:jya] 2021-12-12 21:57:27 PST
Created attachment 446967 [details]
Patch for EWS

rebase
Comment 66 Jean-Yves Avenard [:jya] 2021-12-12 21:58:41 PST
Created attachment 446968 [details]
Patch for Landing

rebase
Comment 67 Jean-Yves Avenard [:jya] 2021-12-13 08:25:24 PST
Created attachment 447004 [details]
Patch

fix GTK crash
Comment 68 EWS 2021-12-13 14:35:56 PST
Committed r286983 (?): <https://commits.webkit.org/r286983>

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