Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(11)

Issue 21485: Bitmap transport (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 7 months ago by agl
Modified:
6 years, 4 months ago
CC:
chromium-reviews_googlegroups.com
Visibility:
Public.

Description

Bitmap transport This patch reworks bitmap transport on all platforms. Linux and Mac are switched from serialising bitmaps over the IPC channel to using shared memory. All platforms gain a shared memory mapping cache on the host side. The concept of a TransportDIB (device independent bitmap) is added to encapsulate most of the platform specifics. On Linux, we use SysV shared memory. This is because X shared pixmaps, which predate POSIX SHM, can only use SysV. By using SysV between renderer and browser, we open up the possibility to map the shared memory directly from the renderer to the X server. On Mac, we use POSIX shared memory. However, since this needs filesystem access and the Mac renderer is sandboxed from the filesystem, we add two new messages from renderer -> browser: The first, AllocTransportDIB, synchronously creates a transport DIB in the browser and passes a handle back to the renderer. The second, FreeTransportDIB, asynchronously, notifies the browser that it may close its handle to the shared memory region. On Mac, the shared memory regions are identified by their inode numbers on the wire. This means that the browser must keep handles open to all the allocated shared memory regions (since an inode number is insufficient to map the region). The alternative design is that the renderer passes the file descriptor with each paint operation. Since passing file descriptors is special case in the code, I felt that it would be best to minimise their use. Creating and freeing transport DIBs are relatively rare operations relative to paints and scrolls. On Windows, most of the code remains the same, except that Windows now uses the mapping cache added in this patch. This allows the browser to maintain a shared memory mapping for a transport DIB over several paints. Previously it mapped and unmapped for every operation, causing lots of TLB and VM churn.

Patch Set 1 #

Patch Set 2 : ... #

Patch Set 3 : Sync to master #

Total comments: 32

Patch Set 4 : ... #

Patch Set 5 : Update XCode project #

Total comments: 50

Patch Set 6 : Addressing Darin's comments #

Patch Set 7 : Some Mac build fixes #

Total comments: 9

Patch Set 8 : Darin's comments / Mac&Windows build fixes #

Total comments: 8

Patch Set 9 : Fix some mac crashes #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+1204 lines, -289 lines) Patch
M base/shared_memory.h View 1 2 3 4 5 4 chunks +15 lines, -2 lines 0 comments Download
M base/shared_memory_posix.cc View 4 chunks +16 lines, -0 lines 0 comments Download
M base/timer.h View 6 7 3 chunks +61 lines, -2 lines 0 comments Download
M base/timer_unittest.cc View 6 7 5 chunks +115 lines, -3 lines 0 comments Download
M chrome/browser/renderer_host/backing_store.h View 1 2 3 4 5 4 chunks +4 lines, -4 lines 0 comments Download
M chrome/browser/renderer_host/backing_store.cc View 1 2 3 4 5 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/renderer_host/backing_store_posix.cc View 1 2 3 4 5 4 chunks +10 lines, -15 lines 0 comments Download
M chrome/browser/renderer_host/backing_store_win.cc View 1 2 3 4 5 4 chunks +5 lines, -14 lines 1 comment Download
M chrome/browser/renderer_host/browser_render_process_host.h View 1 2 3 4 5 4 chunks +18 lines, -0 lines 0 comments Download
M chrome/browser/renderer_host/browser_render_process_host.cc View 1 2 3 4 5 6 7 3 chunks +65 lines, -1 line 0 comments Download
M chrome/browser/renderer_host/mock_render_process_host.h View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/renderer_host/mock_render_process_host.cc View 6 7 8 2 chunks +14 lines, -0 lines 0 comments Download
M chrome/browser/renderer_host/render_process_host.h View 1 2 3 4 5 2 chunks +10 lines, -0 lines 0 comments Download
M chrome/browser/renderer_host/render_widget_helper.h View 6 7 8 6 chunks +34 lines, -1 line 0 comments Download
M chrome/browser/renderer_host/render_widget_helper.cc View 6 7 2 chunks +59 lines, -0 lines 0 comments Download
M chrome/browser/renderer_host/render_widget_host.h View 3 chunks +4 lines, -4 lines 0 comments Download
M chrome/browser/renderer_host/render_widget_host.cc View 1 2 3 4 5 4 chunks +30 lines, -10 lines 0 comments Download
M chrome/browser/renderer_host/render_widget_host_unittest.cc View 4 chunks +4 lines, -24 lines 0 comments Download
M chrome/browser/renderer_host/resource_message_filter.h View 6 7 8 3 chunks +8 lines, -1 line 0 comments Download
M chrome/browser/renderer_host/resource_message_filter.cc View 6 7 2 chunks +18 lines, -0 lines 0 comments Download
M chrome/chrome.xcodeproj/project.pbxproj View 4 chunks +4 lines, -0 lines 0 comments Download
M chrome/common/bitmap_wire_data.h View 1 chunk +0 lines, -35 lines 0 comments Download
M chrome/common/common.scons View 1 2 1 chunk +15 lines, -0 lines 0 comments Download
M chrome/common/common.vcproj View 1 2 2 chunks +9 lines, -1 line 0 comments Download
A chrome/common/ipc_maybe.h View 1 chunk +19 lines, -0 lines 0 comments Download
M chrome/common/ipc_message_utils.h View 1 2 3 4 5 6 7 8 2 chunks +51 lines, -0 lines 0 comments Download
M chrome/common/render_messages.h View 1 2 3 chunks +3 lines, -3 lines 0 comments Download
M chrome/common/render_messages_internal.h View 1 2 3 4 5 6 7 8 2 chunks +17 lines, -0 lines 0 comments Download
A chrome/common/transport_dib.h View 1 2 3 4 5 6 7 1 chunk +112 lines, -0 lines 0 comments Download
A chrome/common/transport_dib_linux.cc View 1 2 3 4 5 6 7 1 chunk +85 lines, -0 lines 0 comments Download
A chrome/common/transport_dib_mac.cc View 1 2 3 4 5 6 7 8 1 chunk +63 lines, -0 lines 0 comments Download
A chrome/common/transport_dib_win.cc View 1 2 3 4 5 6 7 1 chunk +72 lines, -0 lines 0 comments Download
M chrome/renderer/render_process.h View 1 2 3 4 5 6 7 4 chunks +39 lines, -25 lines 0 comments Download
M chrome/renderer/render_process.cc View 1 2 3 4 5 6 7 8 4 chunks +107 lines, -56 lines 0 comments Download
M chrome/renderer/render_process_unittest.cc View 2 chunks +9 lines, -5 lines 0 comments Download
M chrome/renderer/render_widget.h View 3 chunks +5 lines, -8 lines 0 comments Download
M chrome/renderer/render_widget.cc View 1 2 3 4 5 7 chunks +21 lines, -55 lines 0 comments Download
M chrome/renderer/renderer_glue.cc View 1 2 3 4 5 2 chunks +4 lines, -2 lines 0 comments Download
M skia/ext/bitmap_platform_device_linux.h View 1 chunk +1 line, -1 line 0 comments Download
M skia/ext/bitmap_platform_device_mac.cc View 1 chunk +17 lines, -15 lines 0 comments Download
M skia/ext/platform_canvas_linux.h View 1 chunk +1 line, -0 lines 0 comments Download
M skia/ext/platform_canvas_linux.cc View 2 chunks +19 lines, -0 lines 0 comments Download
M skia/ext/platform_canvas_mac.h View 1 chunk +3 lines, -0 lines 0 comments Download
M skia/ext/platform_canvas_mac.cc View 8 2 chunks +35 lines, -0 lines 0 comments Download
Trybot results:
Commit queue not available (can’t edit this change).

Messages

Total messages: 9 (0 generated)
agl
8 years, 7 months ago (2009-02-18 23:14:08 UTC) #1
darin (slow to review)
looks awesome overall. sorry for the pile of nits. the "spammage of revoked tasks" issue ...
8 years, 7 months ago (2009-02-19 01:13:52 UTC) #2
agl
Big changes: * Added DelayTimer (base/timer.h) * Removed the Task storm and switched to DelayTimer ...
8 years, 7 months ago (2009-02-19 04:47:42 UTC) #3
darin (slow to review)
LGreatTM! If you could do the follow-up change to use DelayTimer in more places, that'd ...
8 years, 7 months ago (2009-02-19 17:41:43 UTC) #4
agl
http://codereview.chromium.org/21485/diff/3118/122 File base/timer.h (right): http://codereview.chromium.org/21485/diff/3118/122#newcode214 Line 214: DelayTimer(Receiver* receiver, ReceiverMethod method, TimeDelta delay) On 2009/02/19 ...
8 years, 7 months ago (2009-02-19 20:16:05 UTC) #5
darin (slow to review)
http://codereview.chromium.org/21485/diff/3119/174 File base/timer.h (right): http://codereview.chromium.org/21485/diff/3119/174#newcode252 Line 252: Receiver *const receiver_; extraneous constness that is not ...
8 years, 7 months ago (2009-02-19 20:18:21 UTC) #6
pink (ping after 24hrs)
http://codereview.chromium.org/21485/diff/3119/213 File skia/ext/platform_canvas_mac.cc (right): http://codereview.chromium.org/21485/diff/3119/213#newcode56 Line 56: CGColorSpaceRef colourSpace; we should probably be consistent with ...
8 years, 7 months ago (2009-02-19 20:27:59 UTC) #7
agl
Thanks for the Mac review, I'm going to try and get a Mac to debug ...
8 years, 7 months ago (2009-02-19 21:04:18 UTC) #8
cpu_(ooo_6.6-7.5)
8 years, 7 months ago (2009-02-20 00:28:07 UTC) #9
http://codereview.chromium.org/21485/diff/3123/230
File chrome/browser/renderer_host/backing_store_win.cc (right):

http://codereview.chromium.org/21485/diff/3123/230#newcode34
Line 34: TransportDIB* bitmap,
When merging my change and your change, we should try to map just the size as
computed in line 44, which is unused in this CL, I think is possible, we just
need to be lazy about the moment we map.
Also the mapping on the browser size is read-only and it would be very nice to
keep it this way.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b