Implement FlushSystemCacheForDirectory for Chrome OS

Implement FlushSystemCacheForDirectory for Chrome OS

BUG=429708
TEST=tools/perf/run_benchmark --browser=cros-chrome --remote=${host} startup.cold.blank_page

Committed: https://crrev.com/405e29d407f9be7c102acd7262e0aa25ea202f43
Cr-Commit-Position: refs/heads/master@{#308528}

[hashimoto, 2014-12-15 12:03:35]

Patchset #1 (id:1) has been deleted

[hashimoto, 2014-12-15 12:03:46]

hashimoto@chromium.org changed reviewers:
+ achuith@chromium.org

[achuithb, 2014-12-15 12:05:32]

achuith@chromium.org changed reviewers:
+ nednguyen@google.com

[achuithb, 2014-12-15 12:05:33]

Ned is probably more suitable to review this.

[nednguyen, 2014-12-15 16:11:25]

https://codereview.chromium.org/798483004/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/cros_platform_backend.py (right):

https://codereview.chromium.org/798483004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/cros_platform_backend.py:114:
'/usr/local/telemetry/src/src/out/Release/clear_system_cache')
This seems fragile to me. Is having telemetry source installed in /usr/local is
a requirement for telemetry test to run on an ChromeOs instance remotely?

https://codereview.chromium.org/798483004/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/desktop_platform_backend.py
(right):

https://codereview.chromium.org/798483004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/desktop_platform_backend.py:25:
subprocess.check_call([flush_command, '--recurse', directory])
Is this change related to what you are doing or is this a refactoring? If this
is merely a refactoring change, please make this change in a different patch for
the ease of reviewing.

[hashimoto, 2014-12-15 16:47:36]

https://codereview.chromium.org/798483004/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/cros_platform_backend.py (right):

https://codereview.chromium.org/798483004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/cros_platform_backend.py:114:
'/usr/local/telemetry/src/src/out/Release/clear_system_cache')
On 2014/12/15 16:11:25, nednguyen wrote:
> This seems fragile to me. Is having telemetry source installed in /usr/local
is
> a requirement for telemetry test to run on an ChromeOs instance remotely?
Yes, we require test images to run telemetry tests
(http://www.chromium.org/developers/telemetry/run_locally#TOC-Chrome-OS),
and emerge installs clear_system_cache to this path for test images.
(https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/ma...)

Though, I agree this path looks weird.
Maybe we should submit a Chromium OS-side change to make the path saner.

https://codereview.chromium.org/798483004/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/desktop_platform_backend.py
(right):

https://codereview.chromium.org/798483004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/desktop_platform_backend.py:25:
subprocess.check_call([flush_command, '--recurse', directory])
On 2014/12/15 16:11:25, nednguyen wrote:
> Is this change related to what you are doing or is this a refactoring? If this
> is merely a refactoring change, please make this change in a different patch
for
> the ease of reviewing.

I'm removing this |ignoring| parameter because I don't want to implement unused
|ignoring| handling code for Chrome OS.

Would it be better to keep |ignoring| as is and put "if ignoring: raise
NotImplemented()" in Chrome OS's FlushSystemCacheForDirecotry() implementation
instead?

[nednguyen, 2014-12-15 16:58:10]

https://codereview.chromium.org/798483004/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/cros_platform_backend.py (right):

https://codereview.chromium.org/798483004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/cros_platform_backend.py:114:
'/usr/local/telemetry/src/src/out/Release/clear_system_cache')
On 2014/12/15 16:47:36, hashimoto wrote:
> On 2014/12/15 16:11:25, nednguyen wrote:
> > This seems fragile to me. Is having telemetry source installed in /usr/local
> is
> > a requirement for telemetry test to run on an ChromeOs instance remotely?
> Yes, we require test images to run telemetry tests
> (http://www.chromium.org/developers/telemetry/run_locally#TOC-Chrome-OS),
> and emerge installs clear_system_cache to this path for test images.
>
(https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/ma...)
> 
> Though, I agree this path looks weird.
> Maybe we should submit a Chromium OS-side change to make the path saner.

Ok.

https://codereview.chromium.org/798483004/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/desktop_platform_backend.py
(right):

https://codereview.chromium.org/798483004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/desktop_platform_backend.py:25:
subprocess.check_call([flush_command, '--recurse', directory])
On 2014/12/15 16:47:36, hashimoto wrote:
> On 2014/12/15 16:11:25, nednguyen wrote:
> > Is this change related to what you are doing or is this a refactoring? If
this
> > is merely a refactoring change, please make this change in a different patch
> for
> > the ease of reviewing.
> 
> I'm removing this |ignoring| parameter because I don't want to implement
unused
> |ignoring| handling code for Chrome OS.
> 
> Would it be better to keep |ignoring| as is and put "if ignoring: raise
> NotImplemented()" in Chrome OS's FlushSystemCacheForDirecotry() implementation
> instead?

Yes, please do that instead. I suspect that the ignoring logic may still be used
somewhere.

[hashimoto, 2014-12-15 17:14:24]

Uploaded a new patch set.
PTAL

https://codereview.chromium.org/798483004/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/desktop_platform_backend.py
(right):

https://codereview.chromium.org/798483004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/desktop_platform_backend.py:25:
subprocess.check_call([flush_command, '--recurse', directory])
On 2014/12/15 16:58:10, nednguyen wrote:
> On 2014/12/15 16:47:36, hashimoto wrote:
> > On 2014/12/15 16:11:25, nednguyen wrote:
> > > Is this change related to what you are doing or is this a refactoring? If
> this
> > > is merely a refactoring change, please make this change in a different
patch
> > for
> > > the ease of reviewing.
> > 
> > I'm removing this |ignoring| parameter because I don't want to implement
> unused
> > |ignoring| handling code for Chrome OS.
> > 
> > Would it be better to keep |ignoring| as is and put "if ignoring: raise
> > NotImplemented()" in Chrome OS's FlushSystemCacheForDirecotry()
implementation
> > instead?
> 
> Yes, please do that instead. I suspect that the ignoring logic may still be
used
> somewhere.

Done.

[nednguyen, 2014-12-15 17:21:35]

LGTM

https://codereview.chromium.org/798483004/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/cros_platform_backend.py (right):

https://codereview.chromium.org/798483004/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/cros_platform_backend.py:114: raise
NotImplementedError()
NotImplementedError('Ignoring option is not support when running telemetry test
on cros remotely')

[hashimoto, 2014-12-15 17:31:52]

Patchset #3 (id:60001) has been deleted

[hashimoto, 2014-12-15 17:33:11]

https://codereview.chromium.org/798483004/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/cros_platform_backend.py (right):

https://codereview.chromium.org/798483004/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/cros_platform_backend.py:114: raise
NotImplementedError()
On 2014/12/15 17:21:34, nednguyen wrote:
> NotImplementedError('Ignoring option is not support when running telemetry
test
> on cros remotely')

Done.

[hashimoto, 2014-12-15 17:33:15]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-12-15 17:33:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/798483004/80001

[nednguyen, 2014-12-15 17:58:03]

The CQ bit was unchecked by nednguyen@google.com

[nednguyen, 2014-12-15 18:01:14]

nednguyen@google.com changed reviewers:
+ simonhatch@google.com, tonyg@chromium.org

[nednguyen, 2014-12-15 18:01:15]

I am truly sorry for the back and forth. I think the best things to do here to
make the API consistent across platform.

+Tony, Simon: do you recall why do we need to ignoring param in
FlushSystemCacheForDirectory? Code search indicates that this param is not used
anywhere. If not, can Hashimoto remove support for that param?

[tonyg, 2014-12-15 18:05:33]

On 2014/12/15 18:01:15, nednguyen wrote:
> I am truly sorry for the back and forth. I think the best things to do here to
> make the API consistent across platform.
> 
> +Tony, Simon: do you recall why do we need to ignoring param in
> FlushSystemCacheForDirectory? Code search indicates that this param is not
used
> anywhere. If not, can Hashimoto remove support for that param?

Agreed. Removing the param sgtm (although I didn't bother to do the archaeology
to figure out how things got into this state).

[nednguyen, 2014-12-15 18:09:06]

On 2014/12/15 18:05:33, tonyg wrote:
> On 2014/12/15 18:01:15, nednguyen wrote:
> > I am truly sorry for the back and forth. I think the best things to do here
to
> > make the API consistent across platform.
> > 
> > +Tony, Simon: do you recall why do we need to ignoring param in
> > FlushSystemCacheForDirectory? Code search indicates that this param is not
> used
> > anywhere. If not, can Hashimoto remove support for that param?
> 
> Agreed. Removing the param sgtm (although I didn't bother to do the
archaeology
> to figure out how things got into this state).

Hi Hashi, it seems like your first patch is the better candidate to land. Sorry
again for this back n forth! :(

[hashimoto, 2014-12-16 03:12:33]

On 2014/12/15 18:09:06, nednguyen wrote:
> On 2014/12/15 18:05:33, tonyg wrote:
> > On 2014/12/15 18:01:15, nednguyen wrote:
> > > I am truly sorry for the back and forth. I think the best things to do
here
> to
> > > make the API consistent across platform.
> > > 
> > > +Tony, Simon: do you recall why do we need to ignoring param in
> > > FlushSystemCacheForDirectory? Code search indicates that this param is not
> > used
> > > anywhere. If not, can Hashimoto remove support for that param?
> > 
> > Agreed. Removing the param sgtm (although I didn't bother to do the
> archaeology
> > to figure out how things got into this state).
> 
> Hi Hashi, it seems like your first patch is the better candidate to land.
Sorry
> again for this back n forth! :(

Uploaded the first patch again.
PTAL

[nednguyen, 2014-12-16 03:13:38]

The CQ bit was checked by nednguyen@google.com

[nednguyen, 2014-12-16 03:13:39]

lgtm

[commit-bot: I haz the power, 2014-12-16 03:14:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/798483004/100001

[commit-bot: I haz the power, 2014-12-16 04:21:38]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2014-12-16 05:08:11]

Patchset 4 (id:??) landed as
https://crrev.com/405e29d407f9be7c102acd7262e0aa25ea202f43
Cr-Commit-Position: refs/heads/master@{#308528}