Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(220)

Issue 288005: First fix to minimize copying of image data. (Closed)

Created:
11 years, 2 months ago by Elliot Glaysher
Modified:
9 years, 7 months ago
Reviewers:
brettw
CC:
chromium-reviews_googlegroups.com
Visibility:
Public.

Description

First fix to minimize copying of image data. This is the first of multiple patches that clean up handling of memory regarding images. Previously, the code did several memcpy()s or equivalents. This change: - Creates an abstract interface RefCountedMemory which provides access to the front() of a memory range and the size() of it. It is a RefCountedThreadSafe. - Adds a RefCountedStaticMemory class which isa RefCountedMemory. - Pushes RefCountedBytes up into base/ from chrome/ and make it conform to RefCountedMemory. - Have ResourceBundle return RefCountedStaticMemory to the mmaped() or DLL loaded resources instead of memcpy()ing them. - General cleanups to minimize copies in constructing RefCountedBytes. - Use the above consistent interface in the BrowserThemeProvider, along with special casing the loading of the new tab page background. This patch is mostly cleanups and there should only be a slight performance gain if any. Most of the real speedups should come in subsequent patches. BUG=http://crbug.com/24493 TEST=Slightly faster on Perf bot; does not introduce crashes. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=29412

Patch Set 1 #

Patch Set 2 : Modify gyp #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+239 lines, -158 lines) Patch
M app/resource_bundle.h View 3 chunks +9 lines, -10 lines 0 comments Download
M app/resource_bundle.cc View 2 chunks +14 lines, -14 lines 0 comments Download
M app/resource_bundle_linux.cc View 3 chunks +13 lines, -15 lines 0 comments Download
M app/resource_bundle_mac.mm View 1 chunk +8 lines, -10 lines 0 comments Download
M app/resource_bundle_win.cc View 1 chunk +6 lines, -9 lines 0 comments Download
M app/theme_provider.h View 2 chunks +4 lines, -4 lines 0 comments Download
M base/base.gyp View 1 chunk +1 line, -0 lines 0 comments Download
M base/ref_counted.h View 1 chunk +0 lines, -2 lines 0 comments Download
A base/ref_counted_memory.h View 1 1 chunk +75 lines, -0 lines 1 comment Download
M chrome/browser/browser_theme_provider.h View 3 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/browser_theme_provider.cc View 5 chunks +32 lines, -20 lines 2 comments Download
M chrome/browser/cocoa/bookmark_bar_toolbar_view_unittest.mm View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/dom_ui/chrome_url_data_manager.h View 3 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/dom_ui/chrome_url_data_manager.cc View 6 chunks +8 lines, -8 lines 0 comments Download
M chrome/browser/dom_ui/dom_ui_factory.h View 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/dom_ui/dom_ui_factory.cc View 1 1 chunk +6 lines, -6 lines 0 comments Download
M chrome/browser/dom_ui/dom_ui_favicon_source.h View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/dom_ui/dom_ui_favicon_source.cc View 1 chunk +3 lines, -3 lines 0 comments Download
M chrome/browser/dom_ui/dom_ui_theme_source.h View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/dom_ui/dom_ui_theme_source.cc View 1 2 chunks +3 lines, -9 lines 0 comments Download
M chrome/browser/dom_ui/dom_ui_theme_source_unittest.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/dom_ui/dom_ui_thumbnail_source.h View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/dom_ui/dom_ui_thumbnail_source.cc View 2 chunks +6 lines, -6 lines 0 comments Download
M chrome/browser/dom_ui/downloads_ui.h View 1 chunk +3 lines, -1 line 0 comments Download
M chrome/browser/dom_ui/downloads_ui.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/dom_ui/fileicon_source.cc View 2 chunks +4 lines, -6 lines 0 comments Download
M chrome/browser/dom_ui/history_ui.h View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/dom_ui/history_ui.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/fav_icon_helper.h View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/favicon_service.h View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/favicon_service.cc View 1 chunk +17 lines, -5 lines 0 comments Download
M chrome/browser/history/top_sites.h View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/sync/sync_setup_wizard.cc View 1 chunk +4 lines, -3 lines 0 comments Download
M chrome/common/ref_counted_util.h View 1 chunk +0 lines, -4 lines 0 comments Download
M views/widget/default_theme_provider.h View 1 chunk +1 line, -3 lines 0 comments Download

Messages

Total messages: 2 (0 generated)
Elliot Glaysher
11 years, 2 months ago (2009-10-16 19:32:00 UTC) #1
brettw
11 years, 2 months ago (2009-10-16 23:03:12 UTC) #2
LGTM with some minor updates.

http://codereview.chromium.org/288005/diff/2001/1010
File base/ref_counted_memory.h (right):

http://codereview.chromium.org/288005/diff/2001/1010#newcode72
Line 72: DISALLOW_COPY_AND_ASSIGN(RefCountedBytes);
It's not your fault, but can you fix this? Without "private"
DISALLOW_COPY_AND_ASSIGN does nothing. The similar code left in ref_counted_util
should have a private added as well.

http://codereview.chromium.org/288005/diff/2001/1011
File chrome/browser/browser_theme_provider.cc (right):

http://codereview.chromium.org/288005/diff/2001/1011#newcode460
Line 460: }
You should also remove the {} for this conditional to match the other ones.

http://codereview.chromium.org/288005/diff/2001/1011#newcode811
Line 811: // loaded data, saving a trip to disk.
In person, I asked for some clarification of the caching behaviors of the two
functions since why one is used in some cases wasn't clear to me.

Powered by Google App Engine
This is Rietveld 408576698