AppList Performance Optimization 2

AppList Performance Optimization 2

CL number 2 in a series of CLs to optimize the creation and showing of
Chrome OS App Launcher.

This CL modernizes the code, and removes a lot of wasted cycles.

CPU Duration savings in creating and showing the launcher for the first time after login:
  On peach_pit:
    - Old: 39 ms.
    - New: 34 ms.
  On kevin:
    - Old: 46 ms.
    - New: 40 ms.

BUG=631516
Review-Url: https://codereview.chromium.org/2701123002
Cr-Commit-Position: refs/heads/master@{#453267}
Committed: https://chromium.googlesource.com/chromium/src/+/7f63839ce573ecdf7628f68a6d7434bf53e282f9

[afakhry, 2017-02-22 01:23:56]

Description was changed from

==========
[WIP] AppList Performance Optimization 2

CL number 2 in a series of CLs to optimize the creation and showing of
Chrome OS App Launcher.

BUG=631516
==========

to

==========
AppList Performance Optimization 2

CL number 2 in a series of CLs to optimize the creation and showing of
Chrome OS App Launcher.

This CL modernizes the code, and removes a lot of wasted cycles.

BUG=631516
==========

[afakhry, 2017-02-22 01:31:07]

Description was changed from

==========
AppList Performance Optimization 2

CL number 2 in a series of CLs to optimize the creation and showing of
Chrome OS App Launcher.

This CL modernizes the code, and removes a lot of wasted cycles.

BUG=631516
==========

to

==========
AppList Performance Optimization 2

CL number 2 in a series of CLs to optimize the creation and showing of
Chrome OS App Launcher.

This CL modernizes the code, and removes a lot of wasted cycles.

CPU Duration savings in creating and showing the launcher for the first time
after login:
  On peach_pit:
    - Old: 39 ms.
    - New: 34 ms.
  On kevin:
    - Old: 46 ms.
    - New: 40 ms.

BUG=631516
==========

[afakhry, 2017-02-22 01:31:53]

The CQ bit was checked by afakhry@chromium.org to run a CQ dry run

[afakhry, 2017-02-22 01:32:32]

Patchset #3 (id:40001) has been deleted

[afakhry, 2017-02-22 01:34:06]

The CQ bit was checked by afakhry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 01:35:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 03:38:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-22 03:38:55]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[afakhry, 2017-02-22 17:12:13]

The CQ bit was checked by afakhry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 17:13:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 17:36:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-22 17:36:56]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[afakhry, 2017-02-22 18:13:56]

The CQ bit was checked by afakhry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 18:14:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 18:59:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-22 18:59:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[afakhry, 2017-02-22 19:11:50]

The CQ bit was checked by afakhry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 19:12:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 20:39:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-22 20:39:57]

Dry run: This issue passed the CQ dry run.

[afakhry, 2017-02-22 21:09:31]

afakhry@chromium.org changed reviewers:
+ xiyuan@chromium.org

[afakhry, 2017-02-22 21:09:32]

Xiyuan, this is the second CL in the series. Can you please take a look? Thank
you!

[xiyuan, 2017-02-23 01:38:15]

Mostly nits

https://codereview.chromium.org/2701123002/diff/100001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/search/app_search_provider.cc (right):

https://codereview.chromium.org/2701123002/diff/100001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/app_search_provider.cc:65:
name_(base::UTF8ToUTF16(name)),
We could even defer UTF8ToUTF16, right?

https://codereview.chromium.org/2701123002/diff/100001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/app_search_provider.cc:74: return
base::MakeUnique<TokenizedString>(name_);
Can we cache the result instead of creating it everytime?

https://codereview.chromium.org/2701123002/diff/100001/chrome/browser/ui/app_...
File
chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc
(right):

https://codereview.chromium.org/2701123002/diff/100001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc:79:
std::vector<SearchResult*> sorted_results;
sorted_results.reserve(suggestions_search_.size()) ?

https://codereview.chromium.org/2701123002/diff/100001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc:81:
sorted_results.emplace_back(result.get());
This is faster than the vector ctor? Interesting.

https://codereview.chromium.org/2701123002/diff/100001/ui/app_list/search/mix...
File ui/app_list/search/mixer.cc (right):

https://codereview.chromium.org/2701123002/diff/100001/ui/app_list/search/mix...
ui/app_list/search/mixer.cc:57: for (const std::unique_ptr<SearchResult>& result
: provider->results()) {
const auto& ?

https://codereview.chromium.org/2701123002/diff/100001/ui/app_list/search_con...
File ui/app_list/search_controller.h (right):

https://codereview.chromium.org/2701123002/diff/100001/ui/app_list/search_con...
ui/app_list/search_controller.h:13: #include "base/memory/scoped_vector.h"
Can we remove this now?

https://codereview.chromium.org/2701123002/diff/100001/ui/app_list/search_pro...
File ui/app_list/search_provider.h (right):

https://codereview.chromium.org/2701123002/diff/100001/ui/app_list/search_pro...
ui/app_list/search_provider.h:13: #include "base/memory/scoped_vector.h"
remove?

[afakhry, 2017-02-23 17:04:09]

The CQ bit was checked by afakhry@chromium.org to run a CQ dry run

[afakhry, 2017-02-23 17:04:22]

https://codereview.chromium.org/2701123002/diff/100001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/search/app_search_provider.cc (right):

https://codereview.chromium.org/2701123002/diff/100001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/app_search_provider.cc:65:
name_(base::UTF8ToUTF16(name)),
On 2017/02/23 01:38:14, xiyuan wrote:
> We could even defer UTF8ToUTF16, right?

Yes, we can, but I found it more convenient to do it here since we will need it
in two places in GetTokenizedIndexedName() and also in UpdateResults().

https://codereview.chromium.org/2701123002/diff/100001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/app_search_provider.cc:74: return
base::MakeUnique<TokenizedString>(name_);
On 2017/02/23 01:38:14, xiyuan wrote:
> Can we cache the result instead of creating it everytime?

This is a bit tricky to decide. For our use cases here, UpdateResults() might
need the tokenized string only if the query is not empty. 

UpdateResults() is most probably preceded by RefreshApps() which "recreates" all
the apps from scratch, so caching won't be very useful in this case. Also
caching would waste a few cycles on a conditional like:

if (cached)
  return cached;

return CalculateNew();

For those reasons I chose not to go for caching. WDYT?

https://codereview.chromium.org/2701123002/diff/100001/chrome/browser/ui/app_...
File
chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc
(right):

https://codereview.chromium.org/2701123002/diff/100001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc:79:
std::vector<SearchResult*> sorted_results;
On 2017/02/23 01:38:14, xiyuan wrote:
> sorted_results.reserve(suggestions_search_.size()) ?

Done.

https://codereview.chromium.org/2701123002/diff/100001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc:81:
sorted_results.emplace_back(result.get());
On 2017/02/23 01:38:14, xiyuan wrote:
> This is faster than the vector ctor? Interesting.

Oh, this is just a unit test file so I didn't measure this. The change here is
needed as I converted the ScopedVector to vector<unique_ptr<>>.

https://codereview.chromium.org/2701123002/diff/100001/ui/app_list/search/mix...
File ui/app_list/search/mixer.cc (right):

https://codereview.chromium.org/2701123002/diff/100001/ui/app_list/search/mix...
ui/app_list/search/mixer.cc:57: for (const std::unique_ptr<SearchResult>& result
: provider->results()) {
On 2017/02/23 01:38:14, xiyuan wrote:
> const auto& ?

Done.

https://codereview.chromium.org/2701123002/diff/100001/ui/app_list/search_con...
File ui/app_list/search_controller.h (right):

https://codereview.chromium.org/2701123002/diff/100001/ui/app_list/search_con...
ui/app_list/search_controller.h:13: #include "base/memory/scoped_vector.h"
On 2017/02/23 01:38:14, xiyuan wrote:
> Can we remove this now?

Done.

https://codereview.chromium.org/2701123002/diff/100001/ui/app_list/search_pro...
File ui/app_list/search_provider.h (right):

https://codereview.chromium.org/2701123002/diff/100001/ui/app_list/search_pro...
ui/app_list/search_provider.h:13: #include "base/memory/scoped_vector.h"
On 2017/02/23 01:38:14, xiyuan wrote:
> remove?

Done.

[commit-bot: I haz the power, 2017-02-23 17:04:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 17:49:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-23 17:49:11]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-02-24 05:59:46]

https://codereview.chromium.org/2701123002/diff/100001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/search/app_search_provider.cc (right):

https://codereview.chromium.org/2701123002/diff/100001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/app_search_provider.cc:65:
name_(base::UTF8ToUTF16(name)),
On 2017/02/23 17:04:22, afakhry wrote:
> On 2017/02/23 01:38:14, xiyuan wrote:
> > We could even defer UTF8ToUTF16, right?
> 
> Yes, we can, but I found it more convenient to do it here since we will need
it
> in two places in GetTokenizedIndexedName() and also in UpdateResults().

Acknowledged.

https://codereview.chromium.org/2701123002/diff/100001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/app_search_provider.cc:74: return
base::MakeUnique<TokenizedString>(name_);
On 2017/02/23 17:04:22, afakhry wrote:
> On 2017/02/23 01:38:14, xiyuan wrote:
> > Can we cache the result instead of creating it everytime?
> 
> This is a bit tricky to decide. For our use cases here, UpdateResults() might
> need the tokenized string only if the query is not empty. 
> 
> UpdateResults() is most probably preceded by RefreshApps() which "recreates"
all
> the apps from scratch, so caching won't be very useful in this case. Also
> caching would waste a few cycles on a conditional like:
> 
> if (cached)
>   return cached;
> 
> return CalculateNew();
> 
> For those reasons I chose not to go for caching. WDYT?

App is not a SearchResult. It is the indexed data that AppSearchProvider uses to
match against a query.  RefreshApps() should only be called when app is
installed/uninstalled etc.  I would think the most probable case is from Start()
call, which is called on every char user typed and would schedule an
UpdateResults call. And I don't think we should index app name frequently on
user types.

https://codereview.chromium.org/2701123002/diff/100001/chrome/browser/ui/app_...
File
chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc
(right):

https://codereview.chromium.org/2701123002/diff/100001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc:81:
sorted_results.emplace_back(result.get());
On 2017/02/23 17:04:22, afakhry wrote:
> On 2017/02/23 01:38:14, xiyuan wrote:
> > This is faster than the vector ctor? Interesting.
> 
> Oh, this is just a unit test file so I didn't measure this. The change here is
> needed as I converted the ScopedVector to vector<unique_ptr<>>.

Acknowledged.

[afakhry, 2017-02-24 19:19:07]

The CQ bit was checked by afakhry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 19:19:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[afakhry, 2017-02-24 19:20:47]

https://codereview.chromium.org/2701123002/diff/100001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/search/app_search_provider.cc (right):

https://codereview.chromium.org/2701123002/diff/100001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/app_search_provider.cc:74: return
base::MakeUnique<TokenizedString>(name_);
On 2017/02/24 05:59:46, xiyuan (ooo) wrote:
> On 2017/02/23 17:04:22, afakhry wrote:
> > On 2017/02/23 01:38:14, xiyuan wrote:
> > > Can we cache the result instead of creating it everytime?
> > 
> > This is a bit tricky to decide. For our use cases here, UpdateResults()
might
> > need the tokenized string only if the query is not empty. 
> > 
> > UpdateResults() is most probably preceded by RefreshApps() which "recreates"
> all
> > the apps from scratch, so caching won't be very useful in this case. Also
> > caching would waste a few cycles on a conditional like:
> > 
> > if (cached)
> >   return cached;
> > 
> > return CalculateNew();
> > 
> > For those reasons I chose not to go for caching. WDYT?
> 
> App is not a SearchResult. It is the indexed data that AppSearchProvider uses
to
> match against a query.  RefreshApps() should only be called when app is
> installed/uninstalled etc.  I would think the most probable case is from
Start()
> call, which is called on every char user typed and would schedule an
> UpdateResults call. And I don't think we should index app name frequently on
> user types.

Oh, I see. In that case it makes more sense to cache it. Done.

[commit-bot: I haz the power, 2017-02-24 20:10:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 20:10:38]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-02-27 17:56:54]

lgtm

[afakhry, 2017-02-27 17:59:20]

The CQ bit was checked by afakhry@chromium.org

[commit-bot: I haz the power, 2017-02-27 18:00:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-27 18:34:45]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1488218360146630,
"parent_rev": "52feccd87c2102746f7119ab94f4f1aa7fe7a354", "commit_rev":
"7f63839ce573ecdf7628f68a6d7434bf53e282f9"}

[commit-bot: I haz the power, 2017-02-27 18:35:22]

Description was changed from

==========
AppList Performance Optimization 2

CL number 2 in a series of CLs to optimize the creation and showing of
Chrome OS App Launcher.

This CL modernizes the code, and removes a lot of wasted cycles.

CPU Duration savings in creating and showing the launcher for the first time
after login:
  On peach_pit:
    - Old: 39 ms.
    - New: 34 ms.
  On kevin:
    - Old: 46 ms.
    - New: 40 ms.

BUG=631516
==========

to

==========
AppList Performance Optimization 2

CL number 2 in a series of CLs to optimize the creation and showing of
Chrome OS App Launcher.

This CL modernizes the code, and removes a lot of wasted cycles.

CPU Duration savings in creating and showing the launcher for the first time
after login:
  On peach_pit:
    - Old: 39 ms.
    - New: 34 ms.
  On kevin:
    - Old: 46 ms.
    - New: 40 ms.

BUG=631516

Review-Url: https://codereview.chromium.org/2701123002
Cr-Commit-Position: refs/heads/master@{#453267}
Committed:
https://chromium.googlesource.com/chromium/src/+/7f63839ce573ecdf7628f68a6d74...
==========

[commit-bot: I haz the power, 2017-02-27 18:35:23]

Committed patchset #7 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/7f63839ce573ecdf7628f68a6d74...