Bug 233481

Summary: SharedBufferChunkReader should be refactored
Product: WebKit Reporter: Jean-Yves Avenard [:jya] <jean-yves.avenard>
Component: PlatformAssignee: Jean-Yves Avenard [:jya] <jean-yves.avenard>
Severity: Normal CC: annulen, ews-watchlist, gyuyoung.kim, heycam, ryuan.choi, sergio, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=170956
Bug Depends on:    
Bug Blocks: 233030    
Description Flags
Patch none

Description Jean-Yves Avenard [:jya] 2021-11-24 14:00:08 PST
SharedBufferChunkReader is used to read the content of a SharedBuffer into a Vector.

It flattens the SharedBuffer which is inefficient.

It was first noted in bug 170956 that it should be rewritten and use the SharedBuffer's iterator instead.

Following bug 233030 where SharedBuffer are no longer mutable ; it required the SharedBufferChunkReader to keep a strong ref to a ContiguousSharedBuffer which would increase total memory use.
Comment 1 Radar WebKit Bug Importer 2021-11-24 14:00:54 PST
Comment 2 Jean-Yves Avenard [:jya] 2021-11-25 17:19:13 PST
this task would allow for bug 233030 to be a tad smaller and is enough to do
Comment 3 Jean-Yves Avenard [:jya] 2021-11-25 17:44:02 PST
Created attachment 445164 [details]
Comment 4 Cameron McCormack (:heycam) 2021-11-25 19:22:05 PST
Comment on attachment 445164 [details]

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

> Source/WebCore/ChangeLog:10
> +        SharedBufferChunkReader required the SharedBuffer to be flatten which
> +        would mutate the underlying SharedBuffer.

"to be flattened"

But I'd rewrite this to describe the change being made, e.g.:

Avoid flattening a SharedBuffer when reading reading it through SharedBufferChunkReader.

And maybe this can be the top line of the ChangeLog in place of "Rewrite SharedBufferChunkReader", which is a bit non-specific.

> Source/WebCore/ChangeLog:11
> +        the peek method was also incorrect if the data read overlapped multiple

Similarly change this to describe the change being made:

Fix a bug in the peek method where too many characters would be read if the requested size overlaps multiple segments.

> Source/WebCore/ChangeLog:17
> +        We add it to makefiles and export the header so that API tests can be written.

(Probably doesn't need to be mentioned.)

> Source/WebCore/platform/SharedBuffer.cpp:307
> +        size_t segmentLength = std::min(currentSegment->segment->size(), remaining);
> +        data.append(currentSegment->segment->data(), segmentLength);

"segmentLength" sounds like the length of the segment, but I don't immediately have a better suggestion.

> Source/WebCore/platform/SharedBufferChunkReader.cpp:78
>              auto currentCharacter = m_segment[m_segmentIndex++];
>              if (currentCharacter != m_separator[m_separatorIndex]) {
>                  if (m_separatorIndex > 0) {

The existing code to check for separators doesn't work correctly with arbitrary separator strings. Not sure if it matters. But for example if the buffer contains "aaab" and the separator string is "aab", then it won't detect it as a separator, since at m_segmentIndex = 2, it'll detect a mismatch, and reset m_segmentIndex to 0.

Leave a FIXME in here?

> Source/WebCore/platform/SharedBufferChunkReader.cpp:98
> +        if (++m_iteratorCurrent == m_iteratorEnd) {
>              m_reachedEndOfFile = true;

Maybe set m_segment to nullptr just to be clean in here.

> Source/WebCore/platform/SharedBufferChunkReader.cpp:137
>          if (segmentLength > requestedSize)
>              segmentLength = requestedSize;

Replace this with a std::min, since that's what we do elsewhere?

> Source/WebCore/platform/SharedBufferChunkReader.h:63
> +    const size_t m_bufferSize { 0 };

Can drop the initializer since both constructors initialize this. (And since it's const, the compiler will complain if a constructor stops initializing it.)

> Source/WebCore/platform/SharedBufferChunkReader.h:66
> +    bool m_reachedEndOfFile { false };

I think we can get rid of m_reachedEndOfFile, and check `m_iteratorCurrent != m_iteratorEnd` instead. (In a private helper function, if you like.)

> Tools/TestWebKitAPI/Tests/WebCore/SharedBuffer.cpp:237
> +    const char* const simpleText = "This is a stupid test.";

s/stupid/simple/ if you want to be more positive. :-)
Comment 5 Jean-Yves Avenard [:jya] 2021-11-25 20:09:20 PST
Created attachment 445166 [details]

Apply comment
Comment 6 EWS 2021-11-25 21:04:36 PST
Committed r286171 (244554@main): <https://commits.webkit.org/244554@main>

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