SharedBufferChunkReader should be refactored

• Component: Platform
• Status: RESOLVED FIXED

[Jean-Yves Avenard [:jya]]

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.

[Radar WebKit Bug Importer]

<rdar://problem/85733358>

[Jean-Yves Avenard [:jya]]

this task would allow for bug 233030 to be a tad smaller and is enough to do

[Jean-Yves Avenard [:jya]]

Created attachment 445164 [details]
Patch

[Cameron McCormack (:heycam)]

Comment on attachment 445164 [details]
Patch

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. :-)

[Jean-Yves Avenard [:jya]]

Created attachment 445166 [details]
Patch

Apply comment

[EWS]

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].