Bug 33339 – Add a switch to turn on/off segmented SharedBuffer

• Component: WebCore Misc.
• Status: RESOLVED WONTFIX

[Yong Li]

Add a switch ENABLE(SEGMENTED_SHAREDBUFFER) in SharedBuffer.cpp to simply enable/disable segmenting

[Yong Li]

Created attachment 46081 [details]
the patch

Is it too simple?

With this patch, SharedBuffer doesn't allocate any segment. getSomeData will always return the entire contiguous data.

If we want to totally screen all code with the switch, it could be too boring.

[Sam Weinig]

Why is the ENABLE necessary?  Can you give some background on why one would want to turn this off?  Has someone asked to turn this off?

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 46081 [details] without any errors.

[Yong Li]

(In reply to comment #2)
> Why is the ENABLE necessary?  Can you give some background on why one would
> want to turn this off?  Has someone asked to turn this off?

yes, Peter Kasting says "Chromium would probably want the non-segmented behavior".

I'm also not sure everyone like segmented SharedBuffer.

[Eric Seidel (no email)]

Would be nice to have concrete data as to which ports would and would not want this before adding a switch.

[Darin Adler]

Comment on attachment 46081 [details]
the patch

I do not think we want this switch. We don't want different code paths for different ports.

I understand that Peter has said “Chromium doesn’t want this”, but we’ll have to get more specific than that.

[Peter Kasting]

Actually, I wanted even more than this: I wanted the patch to also return to the copy-free code in the PNG decoder when segmenting was disabled, ensure there is no perf overhead in SharedBuffer (haven't looked closely to see if there still is with this patch), and potentially to disable segmenting by default and turn it on for the ports that expect to be run in low-memory situations.

For ports expecting to be run with large amounts of memory, we (Chromium) believe the segmented SharedBuffer is a net perf loss for two reasons.  First, some of the open-source image decoders have to perform an extra copy (e.g. PNGImageDecoder), and/or run more decode iterations.  Second, the segmented nature of the data means that the SharedBuffer is scattered across pages that have less coherency, and thus when the image is cached, there are negative paging and (processor-level) caching effects.

The biggest proposed benefit of segmented SharedBuffer is that you avoid needing to allocate a block of memory to hold the entire received image.  But this is only really a big deal on highly memory-constrained ports.

So, to summarize, Chromium does not want this, and we suspect most other ports won't either.  I don't know what Safari wants since it doesn't use the public ImageDecoders.

[Yong Li]

(In reply to comment #7)
> Actually, I wanted even more than this: I wanted the patch to also return to
> the copy-free code in the PNG decoder when segmenting was disabled,

PNG decoder is still copy-free. I guess you mean JPEG decoder, and this patch is enough for this purpose.

[Peter Kasting]

(In reply to comment #8)
> (In reply to comment #7)
> > Actually, I wanted even more than this: I wanted the patch to also return to
> > the copy-free code in the PNG decoder when segmenting was disabled,
> 
> PNG decoder is still copy-free. I guess you mean JPEG decoder, and this patch
> is enough for this purpose.

Yes, sorry, the JPEG decoder.  See the patch on bug 33268 for details.

[Yong Li]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > Actually, I wanted even more than this: I wanted the patch to also return to
> > > the copy-free code in the PNG decoder when segmenting was disabled,
> > 
> > PNG decoder is still copy-free. I guess you mean JPEG decoder, and this patch
> > is enough for this purpose.
> Yes, sorry, the JPEG decoder.  See the patch on bug 33268 for details.

If SharedBuffer is not segmented, JPEG decoder won't copy anything additionally.

[Yong Li]

as bug 33268 is closed, this bug is no longer necessary.