1411984 - Switch to using buffered IO for writing event stream in print IPC

Status: RESOLVED FIXED

(Firefox :: PDF Viewer, enhancement, P1)

Description

[Alex Gaynor [:Alex_Gaynor]]

Follow up to bug 1409768.

Right now when we write the event steam to a file when we print, it's done using PRFileDesc*. This does not do buffered IO, which is sad, because it means we spend more time doing writes than we should. Example profile: https://perfht.ml/2ySakDn

Relevant code: https://searchfox.org/mozilla-central/source/layout/printing/DrawEventRecorder.h#47-52

Even the most trivial buffering imaginable would probably be a decent win. I'm hoping that somewhere in our codebase is something for buffered file i/o that can be instantiated from an existing file descriptor/HANDLE.

Comment 1

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1411984 - Use buffered IO in the content process when writing print data for IPC;

Review commit: https://reviewboard.mozilla.org/r/193540/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/193540/

Comment 2

[Alex Gaynor [:Alex_Gaynor]]

Couldn't find an existing IO system that was both buffered and allow instantiation from an FD/HANDLE, so I threw my own simple one together.

It cuts write() time from the profile on macOS from ~50% down to ~15%.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Thanks for the patch! Redirecting to haik because I'm a bit overloaded at this time.

Comment 4

[Haik Aftandilian [:haik]]

Comment on attachment 8922464 [details]
Bug 1411984 - Use buffered IO in the content process when writing print data for IPC;

https://reviewboard.mozilla.org/r/193540/#review200582

::: layout/printing/DrawEventRecorder.h:17
(Diff revision 1)
>  
>  namespace mozilla {
>  namespace layout {
>  
>  class PRFileDescStream : public mozilla::gfx::EventStream {
> +  static const size_t kBufferSize = 1024;

Are the writes always much lower than 1024? Perhaps add a comment explaining 1024 and that writes larger than the buffer size are not buffered. Or realloc() the buffer if writes are larger than the buffer size.

::: layout/printing/DrawEventRecorder.h:46
(Diff revision 1)
>  
>    void Flush() {
> -    // For std::ostream this flushes any internal buffers. PRFileDesc's IO isn't
> -    // buffered, so nothing to do here.
> +    // We need to be API compatible with std::ostream, and so we silently handle
> +    // flushes on a closed FD.
> +    if (IsOpen() && mBufferPos > 0) {
> +      PR_Write(mFd, static_cast<const void*>(mBuffer.get()), mBufferPos);

We don't check the return value of PR_Write() here or in the existing code. That seems like an existing bug. Instead we could have failed writes set mGood and then update recording code to check good().

::: layout/printing/DrawEventRecorder.h:67
(Diff revision 1)
> +        Flush();
> -      PR_Write(mFd, static_cast<const void*>(aData), aSize);
> +        PR_Write(mFd, static_cast<const void*>(aData), aSize);
> +      // If our write could fit in our buffer, but doesn't because the buffer is
> +      // partially full, write to the buffer, flush the buffer, and then write
> +      // the rest of the data to the buffer.
> +      } else if (aSize > (kBufferSize - mBufferPos)) {

A macro or method for buffer available space would help readability.

::: layout/printing/DrawEventRecorder.h:69
(Diff revision 1)
> +      // If our write could fit in our buffer, but doesn't because the buffer is
> +      // partially full, write to the buffer, flush the buffer, and then write
> +      // the rest of the data to the buffer.
> +      } else if (aSize > (kBufferSize - mBufferPos)) {
> +        size_t length = kBufferSize - mBufferPos;
> +        WriteToBuffer(aData, length);

WriteToBuffer() call not necessary if length = 0 -- when the buffer is completely full.

::: layout/printing/DrawEventRecorder.h:81
(Diff revision 1)
>    void read(char* aOut, size_t aSize) {
>      PRInt32 res = PR_Read(mFd, static_cast<void*>(aOut), aSize);
>      mGood = res >= 0 && ((size_t)res == aSize);
>    }

Does read() also need to take the buffer into account? i.e., so that data written into the buffer and not flushed can be returned with read().

Is it implied that Seek() is always called in between a write() and read()?

Comment 5

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8922464 [details]
Bug 1411984 - Use buffered IO in the content process when writing print data for IPC;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/193540/diff/1-2/

Comment 6

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8922464 [details]
Bug 1411984 - Use buffered IO in the content process when writing print data for IPC;

https://reviewboard.mozilla.org/r/193540/#review200582

> We don't check the return value of PR_Write() here or in the existing code. That seems like an existing bug. Instead we could have failed writes set mGood and then update recording code to check good().

Do you want that in this patch? I don't want to expand the scope of this too much.

> WriteToBuffer() call not necessary if length = 0 -- when the buffer is completely full.

True, but it works fine, so I'm disinclined to care :-)

> Does read() also need to take the buffer into account? i.e., so that data written into the buffer and not flushed can be returned with read().
> 
> Is it implied that Seek() is always called in between a write() and read()?

Currently no users interleave reads and writes, but I added a `Flush()` call so it's always correct.

Comment 7

[Haik Aftandilian [:haik]]

Comment on attachment 8922464 [details]
Bug 1411984 - Use buffered IO in the content process when writing print data for IPC;

https://reviewboard.mozilla.org/r/193540/#review200680

r+ with nits.

::: layout/printing/DrawEventRecorder.h:72
(Diff revisions 1 - 2)
>          PR_Write(mFd, static_cast<const void*>(aData), aSize);
>        // If our write could fit in our buffer, but doesn't because the buffer is
>        // partially full, write to the buffer, flush the buffer, and then write
>        // the rest of the data to the buffer.
> -      } else if (aSize > (kBufferSize - mBufferPos)) {
> +      } else if (aSize > AvailableBufferSpace()) {
>          size_t length = kBufferSize - mBufferPos;

Use AvailableBufferSpace() here too.

::: layout/printing/DrawEventRecorder.h:102
(Diff revisions 1 - 2)
> +    return kBufferSize - mBufferPos;
> +  }
> +
>    void WriteToBuffer(const char* aData, size_t aSize) {
>      MOZ_ASSERT(aSize <= kBufferSize);
>      MOZ_ASSERT(aSize <= (kBufferSize - mBufferPos));

AvailableBufferSpace()

Comment 8

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8922464 [details]
Bug 1411984 - Use buffered IO in the content process when writing print data for IPC;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/193540/diff/2-3/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8e247550d6cf
Use buffered IO in the content process when writing print data for IPC; r=haik

Comment 10

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3568e6d34bf6
Backed out changeset 8e247550d6cf for bustage.

Comment 11

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8922464 [details]
Bug 1411984 - Use buffered IO in the content process when writing print data for IPC;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/193540/diff/3-4/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/19903553695c
Use buffered IO in the content process when writing print data for IPC; r=haik

Comment 13

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/19903553695c