Android systrace: Optimize memory dumps.

Android systrace: Optimize memory dumps.

- Faster parsing of smaps
- Smaller dump size, process info mentioned once
- Skipping in-kernel workers
- Skipping GPU stats for non-app processes
- Selective full dumps

BUG=catapult:#3437

Review-Url: https://codereview.chromium.org/2946033002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/3b0c0e04db0bd422a420e3e9b45bbbaad1dd978b

[kraynov, 2017-06-21 14:23:35]

kraynov@chromium.org changed reviewers:
+ primiano@chromium.org

[kraynov, 2017-06-21 14:23:36]

PTAL

https://codereview.chromium.org/2946033002/diff/40001/systrace/atrace_helper/...
File systrace/atrace_helper/jni/libmemtrack_wrapper.h (left):

https://codereview.chromium.org/2946033002/diff/40001/systrace/atrace_helper/...
systrace/atrace_helper/jni/libmemtrack_wrapper.h:30: bool has_errors() const {
return proc_ != nullptr; };
It was mistake

[kraynov, 2017-06-21 14:27:41]

Example after running
atrace_helper -c 1 -m system_server -g -s

https://drive.google.com/open?id=0B-IDmAzWp11ibzZYODlFckZhZEU

[Primiano Tucci (use gerrit), 2017-06-22 07:55:52]

Looks overall good, but few remarks:

1) This patch is 600 LOC. The median of changes in chrome is 60 LOC. 75th
percentile is 160 LOC, 95th 535 LOC *.
This change is hence over the 95th percentile in terms of lines of code changed.
Please consider splitting this in more reviewable pieces.

2) Architecturally I am not sure I understand the difference between
atrace_process_dump.cc and ProcessDumpManager. Both of them seem to somehow
coordinate dumps. Can you pick one?

More below inline.

*
https://docs.google.com/spreadsheets/d/1NSoN1cu3ciIgecoTogJXtnAG7jdOV5WwSsuJ_...

https://codereview.chromium.org/2946033002/diff/40001/systrace/atrace_helper/...
File systrace/atrace_helper/jni/atrace_process_dump.cc (right):

https://codereview.chromium.org/2946033002/diff/40001/systrace/atrace_helper/...
systrace/atrace_helper/jni/atrace_process_dump.cc:72: int n_mmaps =
mem->mmaps_count();
size_t, here and below

https://codereview.chromium.org/2946033002/diff/40001/systrace/atrace_helper/...
File systrace/atrace_helper/jni/atrace_process_dump.h (right):

https://codereview.chromium.org/2946033002/diff/40001/systrace/atrace_helper/...
systrace/atrace_helper/jni/atrace_process_dump.h:17: class AtraceProcessDump {
some lines of comment explaining what this class does could help

https://codereview.chromium.org/2946033002/diff/40001/systrace/atrace_helper/...
systrace/atrace_helper/jni/atrace_process_dump.h:20: // comma-separated list -
only processes with named in the list.
i'd just say here: comma-separate list of process names

https://codereview.chromium.org/2946033002/diff/40001/systrace/atrace_helper/...
systrace/atrace_helper/jni/atrace_process_dump.h:27: void SetDumpFrequency(int
interval_ms) { dump_interval_ms_ = interval_ms; }
You call the setter Frequency but the argument is an interval :)
Maybe SetDumpRate, SetDumpPeriod, SetDumpInterval

https://codereview.chromium.org/2946033002/diff/40001/systrace/atrace_helper/...
File systrace/atrace_helper/jni/process_info.h (right):

https://codereview.chromium.org/2946033002/diff/40001/systrace/atrace_helper/...
systrace/atrace_helper/jni/process_info.h:20: std::string name;
isn't this going to create all sort of (slower) heap allocations an movements?
Or are you using this class in a copiable/movable structure?

https://codereview.chromium.org/2946033002/diff/40001/systrace/atrace_helper/...
systrace/atrace_helper/jni/process_info.h:74: void SetFullDumpPredicate(const
DumpPredicate& predicate);
honestly I don't see the need of passing a predicate just to do the filtering
outside. Why the filtering isn't here?

[kraynov, 2017-06-22 09:19:45]

Updated :)

https://codereview.chromium.org/2946033002/diff/40001/systrace/atrace_helper/...
File systrace/atrace_helper/jni/atrace_process_dump.cc (right):

https://codereview.chromium.org/2946033002/diff/40001/systrace/atrace_helper/...
systrace/atrace_helper/jni/atrace_process_dump.cc:72: int n_mmaps =
mem->mmaps_count();
On 2017/06/22 07:55:51, Primiano Tucci wrote:
> size_t, here and below

Done.

https://codereview.chromium.org/2946033002/diff/40001/systrace/atrace_helper/...
File systrace/atrace_helper/jni/atrace_process_dump.h (right):

https://codereview.chromium.org/2946033002/diff/40001/systrace/atrace_helper/...
systrace/atrace_helper/jni/atrace_process_dump.h:17: class AtraceProcessDump {
On 2017/06/22 07:55:51, Primiano Tucci wrote:
> some lines of comment explaining what this class does could help

Done.

https://codereview.chromium.org/2946033002/diff/40001/systrace/atrace_helper/...
systrace/atrace_helper/jni/atrace_process_dump.h:20: // comma-separated list -
only processes with named in the list.
On 2017/06/22 07:55:51, Primiano Tucci wrote:
> i'd just say here: comma-separate list of process names

Done.

https://codereview.chromium.org/2946033002/diff/40001/systrace/atrace_helper/...
systrace/atrace_helper/jni/atrace_process_dump.h:27: void SetDumpFrequency(int
interval_ms) { dump_interval_ms_ = interval_ms; }
On 2017/06/22 07:55:51, Primiano Tucci wrote:
> You call the setter Frequency but the argument is an interval :)
> Maybe SetDumpRate, SetDumpPeriod, SetDumpInterval 

Done.

https://codereview.chromium.org/2946033002/diff/40001/systrace/atrace_helper/...
File systrace/atrace_helper/jni/process_info.h (right):

https://codereview.chromium.org/2946033002/diff/40001/systrace/atrace_helper/...
systrace/atrace_helper/jni/process_info.h:20: std::string name;
On 2017/06/22 07:55:52, Primiano Tucci wrote:
> isn't this going to create all sort of (slower) heap allocations an movements?
> Or are you using this class in a copiable/movable structure?

It takes O(n_processes) to allocate all the strings which is quite small and
roughly constant overhead for ~hundreds of processes with ~tens of threads. In
case of ProcessInfo name field is using to query std::set which would be
inconvenient with char*. For consistency and simplicity I'd prefer to keep
std::string in these structures.

https://codereview.chromium.org/2946033002/diff/40001/systrace/atrace_helper/...
systrace/atrace_helper/jni/process_info.h:74: void SetFullDumpPredicate(const
DumpPredicate& predicate);
On 2017/06/22 07:55:51, Primiano Tucci wrote:
> honestly I don't see the need of passing a predicate just to do the filtering
> outside. Why the filtering isn't here?

The logic which process gets which dump is the scope of program itself, not a
module. However predicate for graphics dump is really overkill here because it's
quite fast to obtain and very simple rule for filtering. Have removed this
predicate.

[Primiano Tucci (use gerrit), 2017-06-23 08:50:19]

On 2017/06/22 09:19:45, kraynov wrote:
> Updated :)
This seems still a 600 lines patch. can you please split it?

[kraynov, 2017-06-23 11:44:42]

Unfortunately this change has incurred some refactoring and seems to be quite
atomic. Also -146 lines in main.cc basically reallocated to
atrace_process_dump.cc.
If it's too hard to review this CL I can try to split but in doubt that two
rough-splitted CLs would be better.

[Primiano Tucci (use gerrit), 2017-06-23 13:15:38]

On 2017/06/23 11:44:42, kraynov wrote:
> Unfortunately this change has incurred some refactoring and seems to be quite
> atomic. Also -146 lines in main.cc basically reallocated to
> atrace_process_dump.cc.
> If it's too hard to review this CL I can try to split but in doubt that two
> rough-splitted CLs would be better.

Not a problem but this will have to wait then. Reviewing a 600loc patch takes me
roughly 1h and I don't have a contiguous hour today. Alternatively if you can't
wait you can always find somebody else willing to review this

[Primiano Tucci (use gerrit), 2017-06-26 10:07:20]

It seems there are still unresolved questions from my previous review,
specifically:

- atrace_process_dump.cc vs ProcessDumpManager
- the predicate stuff.

I stopped reviewing the other minor stuff once I reached that point and realized
we have unresolved design questions still open.

https://codereview.chromium.org/2946033002/diff/40001/systrace/atrace_helper/...
File systrace/atrace_helper/jni/process_info.h (right):

https://codereview.chromium.org/2946033002/diff/40001/systrace/atrace_helper/...
systrace/atrace_helper/jni/process_info.h:74: void SetFullDumpPredicate(const
DumpPredicate& predicate);
On 2017/06/22 09:19:45, kraynov wrote:
> On 2017/06/22 07:55:51, Primiano Tucci wrote:
> > honestly I don't see the need of passing a predicate just to do the
filtering
> > outside. Why the filtering isn't here?
> 
> The logic which process gets which dump is the scope of program itself, not a
> module. However predicate for graphics dump is really overkill here because
it's
> quite fast to obtain and very simple rule for filtering. Have removed this
> predicate.

the dump predicate seems to be still here, but I thought you sad it was removed

https://codereview.chromium.org/2946033002/diff/60001/systrace/atrace_helper/...
File systrace/atrace_helper/jni/atrace_process_dump.cc (right):

https://codereview.chromium.org/2946033002/diff/60001/systrace/atrace_helper/...
systrace/atrace_helper/jni/atrace_process_dump.cc:17: self_pid_ = (int)
getpid();
static_cast<int>(getpid()). c-style cast are not allowed in our code style:

https://google.github.io/styleguide/cppguide.html#Casting

https://codereview.chromium.org/2946033002/diff/60001/systrace/atrace_helper/...
systrace/atrace_helper/jni/atrace_process_dump.cc:38: for (auto pit =
processes->begin(); pit != processes->end();) {
I thought this was a typo (pit -> pid) but then realized that you meant iterator
here.
Eitehr just |it|, or |proc_iter|, or |proc_it|.

https://codereview.chromium.org/2946033002/diff/60001/systrace/atrace_helper/...
systrace/atrace_helper/jni/atrace_process_dump.cc:145:
std::unique_ptr<ProcessDumpManager> mgr(new ProcessDumpManager());
Copy/pasting to what I said in the previous review (I didn't get any reply):
Architecturally I am not sure I understand the difference between
atrace_process_dump.cc and ProcessDumpManager. Both of them seem to somehow
coordinate dumps. Can you pick one?

https://codereview.chromium.org/2946033002/diff/60001/systrace/atrace_helper/...
File systrace/atrace_helper/jni/atrace_process_dump.h (right):

https://codereview.chromium.org/2946033002/diff/60001/systrace/atrace_helper/...
systrace/atrace_helper/jni/atrace_process_dump.h:33: void Stop() { CloseTimer();
}
why this is inlined?
coding style says that only setters and trivial methods can be inlined, and in
that case they should be named lowercase(). This one calls an actual function,
so it looks like it should have its own definition in the .cc ifle

https://codereview.chromium.org/2946033002/diff/60001/systrace/atrace_helper/...
systrace/atrace_helper/jni/atrace_process_dump.h:54: bool full_dump_all_ =
false;
it feels this should really be an enum: with 3 values {kOnlyWhitelistedProcs,
kAllProcesses, kAllJavaApps} or something similar

[kraynov, 2017-06-28 14:08:32]

PTAL

[Primiano Tucci (use gerrit), 2017-06-29 15:21:34]

lgtm

[kraynov, 2017-06-29 15:27:18]

Description was changed from

==========
Android systrace: Optimize memory dumps.

- Faster parsing of smaps
- Smaller dump size, process info mentioned once
- Skipping in-kernel workers
- Skipping GPU stats for non-app processes
- Selective full dumps

BUG=catapult:#3482
==========

to

==========
Android systrace: Optimize memory dumps.

- Faster parsing of smaps
- Smaller dump size, process info mentioned once
- Skipping in-kernel workers
- Skipping GPU stats for non-app processes
- Selective full dumps

BUG=catapult:#3437
==========

[kraynov, 2017-06-29 15:28:28]

kraynov@chromium.org changed reviewers:
+ ccraik@google.com, jreck@google.com

[kraynov, 2017-06-29 15:28:29]

+jreck
+ccraik

[John Reck, 2017-06-30 21:11:51]

lgtm

[kraynov, 2017-07-01 21:19:23]

The CQ bit was checked by kraynov@chromium.org

[kraynov, 2017-07-01 21:19:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org
Link to the patchset: https://codereview.chromium.org/2946033002/#ps120001
(title: "tiny fix")

[commit-bot: I haz the power, 2017-07-01 21:19:28]

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-07-01 21:44:20]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1498943963995740,
"parent_rev": "39accaa53587b91392345046b0ec5bb52f3c6c33", "commit_rev":
"3b0c0e04db0bd422a420e3e9b45bbbaad1dd978b"}

[commit-bot: I haz the power, 2017-07-01 21:44:23]

Description was changed from

==========
Android systrace: Optimize memory dumps.

- Faster parsing of smaps
- Smaller dump size, process info mentioned once
- Skipping in-kernel workers
- Skipping GPU stats for non-app processes
- Selective full dumps

BUG=catapult:#3437
==========

to

==========
Android systrace: Optimize memory dumps.

- Faster parsing of smaps
- Smaller dump size, process info mentioned once
- Skipping in-kernel workers
- Skipping GPU stats for non-app processes
- Selective full dumps

BUG=catapult:#3437

Review-Url: https://codereview.chromium.org/2946033002
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2017-07-01 21:44:24]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...