New issue
Advanced search Search tips

Issue 638775 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2016
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Remove ComponentLoader's unnecessary string copy.

Project Member Reported by lazyboy@chromium.org, Aug 17 2016

Issue description

Just ran across this:

If you look at how component extensions are added, e.g.:

ComponentLoader::AddDefaultComponentExtensionsWithBackgroundPages(
  ..
  Add(IDR_FEEDBACK_MANIFEST, base::FilePath(..))
  ..

ComponentLoader::Add(int manifest_id, ...) gets the resource from ResourceBundle and then converts the StringPiece to std::string (copy) and feeds them to ComponentLoader::ParseManifest(). ParseManifest construct a StringPiece from the std::string since JSONStringValueDeserializer is capable of working with StringPiece-s. Anyhoo, the intermediate copy to string isn't necessary at all.


From my tests, this affects ChromeOS more than desktop. I can see 30 component extensions being loaded, which totals 49254 bytes.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 18 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3cdbf74891491f09473f87ebf215d327ff27296d

commit 3cdbf74891491f09473f87ebf215d327ff27296d
Author: lazyboy <lazyboy@chromium.org>
Date: Thu Aug 18 01:29:05 2016

Remove ComponentLoader's unnecessary string copy.

ComponentLoader::Add(int manifest_id, ...) is how most components extensions
get added. Couple of exceptions are: pdf_extension and IME. Otherwise,
ComponentLoader::Add gets the resource from ResourceBundle and then converts the
StringPiece representing the resource to std::string (copy) and feeds them to
ComponentLoader::ParseManifest().
ParseManifest construct a StringPiece from the std::string since
JSONStringValueDeserializer is capable of working with StringPiece-s.

This CL removes the intermediate conversion from StringPiece to string for
most of the cases.
This probably helps chormeos more than non-chromeos, since I saw 30 component
extension being loaded through ComponentLoader resulting in 49254 bytes
of unnecessary copy.

BUG= 638775 
Test=No observable change. FYI: this can affect built in chromeos extensions.

Review-Url: https://codereview.chromium.org/2252253003
Cr-Commit-Position: refs/heads/master@{#412719}

[modify] https://crrev.com/3cdbf74891491f09473f87ebf215d327ff27296d/chrome/browser/extensions/component_loader.cc
[modify] https://crrev.com/3cdbf74891491f09473f87ebf215d327ff27296d/chrome/browser/extensions/component_loader.h

Status: Fixed (was: Started)
Project Member

Comment 3 by bugdroid1@chromium.org, Aug 18 2016

Labels: merge-merged-2832
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3cdbf74891491f09473f87ebf215d327ff27296d

commit 3cdbf74891491f09473f87ebf215d327ff27296d
Author: lazyboy <lazyboy@chromium.org>
Date: Thu Aug 18 01:29:05 2016

Remove ComponentLoader's unnecessary string copy.

ComponentLoader::Add(int manifest_id, ...) is how most components extensions
get added. Couple of exceptions are: pdf_extension and IME. Otherwise,
ComponentLoader::Add gets the resource from ResourceBundle and then converts the
StringPiece representing the resource to std::string (copy) and feeds them to
ComponentLoader::ParseManifest().
ParseManifest construct a StringPiece from the std::string since
JSONStringValueDeserializer is capable of working with StringPiece-s.

This CL removes the intermediate conversion from StringPiece to string for
most of the cases.
This probably helps chormeos more than non-chromeos, since I saw 30 component
extension being loaded through ComponentLoader resulting in 49254 bytes
of unnecessary copy.

BUG= 638775 
Test=No observable change. FYI: this can affect built in chromeos extensions.

Review-Url: https://codereview.chromium.org/2252253003
Cr-Commit-Position: refs/heads/master@{#412719}

[modify] https://crrev.com/3cdbf74891491f09473f87ebf215d327ff27296d/chrome/browser/extensions/component_loader.cc
[modify] https://crrev.com/3cdbf74891491f09473f87ebf215d327ff27296d/chrome/browser/extensions/component_loader.h

Sign in to add a comment