Bug 33339 - Add a switch to turn on/off segmented SharedBuffer
Summary: Add a switch to turn on/off segmented SharedBuffer
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-07 12:49 PST by Yong Li
Modified: 2010-09-23 13:22 PDT (History)
6 users (show)

See Also:


Attachments
the patch (1.23 KB, patch)
2010-01-07 13:24 PST, Yong Li
darin: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yong Li 2010-01-07 12:49:27 PST
Add a switch ENABLE(SEGMENTED_SHAREDBUFFER) in SharedBuffer.cpp to simply enable/disable segmenting
Comment 1 Yong Li 2010-01-07 13:24:33 PST
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.
Comment 2 Sam Weinig 2010-01-07 13:25:57 PST
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?
Comment 3 WebKit Review Bot 2010-01-07 13:28:20 PST
style-queue ran check-webkit-style on attachment 46081 [details] without any errors.
Comment 4 Yong Li 2010-01-07 13:37:37 PST
(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.
Comment 5 Eric Seidel (no email) 2010-01-08 12:42:08 PST
Would be nice to have concrete data as to which ports would and would not want this before adding a switch.
Comment 6 Darin Adler 2010-01-11 09:55:44 PST
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.
Comment 7 Peter Kasting 2010-01-11 11:16:12 PST
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.
Comment 8 Yong Li 2010-01-11 12:09:21 PST
(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.
Comment 9 Peter Kasting 2010-01-11 12:50:50 PST
(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.
Comment 10 Yong Li 2010-01-12 06:47:57 PST
(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.
Comment 11 Yong Li 2010-09-23 13:22:00 PDT
as bug 33268 is closed, this bug is no longer necessary.