Support persistent system profiles.

Support persistent system profiles.

This allows the entire system profile to be saved into one or more
persistent memory segments for use after the process exits.

The initial version saves only the snapshot known during metrics
reporting but later versions will build upon this to keep the profile
up-to-date in real time.

BUG=695880
Review-Url: https://codereview.chromium.org/2907543003
Cr-Commit-Position: refs/heads/master@{#475534}
Committed: https://chromium.googlesource.com/chromium/src/+/4cde5b7b932ff3ae33107778f592fc21021a63c1

[bcwhite, 2017-05-26 00:38:45]

bcwhite@chromium.org changed reviewers:
+ asvitkine@chromium.org

[bcwhite, 2017-05-26 00:38:46]

Here's a rough first draft.  I'll go through and clean it
up, add comments, etc. but wanted to see if you had any
architecture comments.

[bcwhite, 2017-05-26 16:13:28]

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

[commit-bot: I haz the power, 2017-05-26 16:13:52]

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-05-26 17:45:06]

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

[commit-bot: I haz the power, 2017-05-26 17:45:07]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Alexei Svitkine (slow), 2017-05-26 18:01:53]

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
File components/metrics/persistent_system_profile.cc (right):

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
components/metrics/persistent_system_profile.cc:37: size_t CalcRecordSize(size_t
data_amount) {
Nit: Calculate

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
components/metrics/persistent_system_profile.cc:83: size_t size = record.size();
Nit: remaining_size

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
components/metrics/persistent_system_profile.cc:104:
allocator_->GetAllocSize(tail_reference_) & ~(sizeof(RecordHeader) - 1);
This is very cryptic.

You get the size as size_t.

Then you take sizeof(RecordHeader)-1, which should be 31 (i.e. the 0b11111) and
essentially clearing those bottom 5 bits from the size... I don't really follow
why this is needed...

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
components/metrics/persistent_system_profile.cc:106: amount = std::min(amount,
alloc_size - end_offset_ - sizeof(header));
Nit: size_to_write instead of amount

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
components/metrics/persistent_system_profile.cc:119: memcpy(block + offset +
sizeof(header), data, amount);
I feel like this code would be clearer with another helper function.

At a high-level, what this is doing is:

while (remaining_size > 0 && MaybeAdvanceSegment()) { 
  data = WriteData(data, &remaining_size);
}

Can you refactor the code so it's a bit more like that? i.e. split a helper to
do the actual copying of data from the logic of making sure we have a segment to
write to.

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
components/metrics/persistent_system_profile.cc:165: if (header.as_parts.type ==
kUnusedSpace) {
I don't see where kUnusedSpace is written? Is it meant to be implicit - if so
that should be documented in the enum.

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
components/metrics/persistent_system_profile.cc:251: }
Nit: Use EraseIf() from base/stl_util.h?

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
components/metrics/persistent_system_profile.cc:287: return true;
Return the result of system_profile->ParseFromString()? In case there's data
corruption for example.

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
File components/metrics/persistent_system_profile.h (right):

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
components/metrics/persistent_system_profile.h:14: 
Nit: No need for inner newlines for forward declarations.

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
components/metrics/persistent_system_profile.h:21: // Manage a copy of the
system profile inside persistent memory segments.
Nit: Manages

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
components/metrics/persistent_system_profile.h:40: const
base::PersistentMemoryAllocator* memory_allocator);
If the param can't be null, pass by const ref instead of pointer.

Also, swap the param order, since system_profile is an out param.

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
components/metrics/persistent_system_profile.h:48: kPadding,
This seems unused?

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
components/metrics/persistent_system_profile.h:62: bool Matches(const
base::PersistentMemoryAllocator* allocator) const;
Nit: Add a comment.

Alternatively, how about just having an accessor for allocator() and then the
caller could do the comparison themselves?

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
components/metrics/persistent_system_profile.h:66: void Reset();
Given you always call Reset() before Write(), how about combining the two? (i.e.
Reset() being an implementation detail of Write().

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
components/metrics/persistent_system_profile.h:86: mutable uint32_t end_offset_;
     // End of data in block.
This field is not clear to me.

Why is it an offset, and not something like filled_size_?

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
components/metrics/persistent_system_profile.h:95:
THREAD_CHECKER(thread_checker_);
Nit: Make this the first member.

[bcwhite, 2017-05-29 17:59:39]

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

[commit-bot: I haz the power, 2017-05-29 17:59:55]

Dry run: CQ is trying da patch.

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

[bcwhite, 2017-05-29 18:32:27]

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
File components/metrics/persistent_system_profile.cc (right):

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
components/metrics/persistent_system_profile.cc:37: size_t CalcRecordSize(size_t
data_amount) {
On 2017/05/26 18:01:52, Alexei Svitkine (slow) wrote:
> Nit: Calculate

Done.

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
components/metrics/persistent_system_profile.cc:83: size_t size = record.size();
On 2017/05/26 18:01:52, Alexei Svitkine (slow) wrote:
> Nit: remaining_size

Done.

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
components/metrics/persistent_system_profile.cc:104:
allocator_->GetAllocSize(tail_reference_) & ~(sizeof(RecordHeader) - 1);
On 2017/05/26 18:01:52, Alexei Svitkine (slow) wrote:
> This is very cryptic.
> 
> You get the size as size_t.
> 
> Then you take sizeof(RecordHeader)-1, which should be 31 (i.e. the 0b11111)
and
> essentially clearing those bottom 5 bits from the size... I don't really
follow
> why this is needed...
> 

The tilde (~) before it flips the bits so it's clearing the bottom bits.  This
is the typical way of stripping off any extra bytes so that the value is rounded
down to a multiple of sizeof(RecordHeader).  It basically guarantees that there
will be space for another header if there is any space at all.

It could be removed since the allocator will always meet that requirement. 
Perhaps it would be better just to document it instead.  Done.

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
components/metrics/persistent_system_profile.cc:106: amount = std::min(amount,
alloc_size - end_offset_ - sizeof(header));
On 2017/05/26 18:01:52, Alexei Svitkine (slow) wrote:
> Nit: size_to_write instead of amount

Done.

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
components/metrics/persistent_system_profile.cc:119: memcpy(block + offset +
sizeof(header), data, amount);
On 2017/05/26 18:01:52, Alexei Svitkine (slow) wrote:
> I feel like this code would be clearer with another helper function.
> 
> At a high-level, what this is doing is:
> 
> while (remaining_size > 0 && MaybeAdvanceSegment()) { 
>   data = WriteData(data, &remaining_size);
> }
> 
> Can you refactor the code so it's a bit more like that? i.e. split a helper to
> do the actual copying of data from the logic of making sure we have a segment
to
> write to.

Done.

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
components/metrics/persistent_system_profile.cc:165: if (header.as_parts.type ==
kUnusedSpace) {
On 2017/05/26 18:01:52, Alexei Svitkine (slow) wrote:
> I don't see where kUnusedSpace is written? Is it meant to be implicit - if so
> that should be documented in the enum.

Done.

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
components/metrics/persistent_system_profile.cc:251: }
On 2017/05/26 18:01:52, Alexei Svitkine (slow) wrote:
> Nit: Use EraseIf() from base/stl_util.h?

Done.

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
components/metrics/persistent_system_profile.cc:287: return true;
On 2017/05/26 18:01:52, Alexei Svitkine (slow) wrote:
> Return the result of system_profile->ParseFromString()? In case there's data
> corruption for example.

Done.

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
File components/metrics/persistent_system_profile.h (right):

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
components/metrics/persistent_system_profile.h:14: 
On 2017/05/26 18:01:53, Alexei Svitkine (slow) wrote:
> Nit: No need for inner newlines for forward declarations.

Done.

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
components/metrics/persistent_system_profile.h:21: // Manage a copy of the
system profile inside persistent memory segments.
On 2017/05/26 18:01:53, Alexei Svitkine (slow) wrote:
> Nit: Manages

Done.

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
components/metrics/persistent_system_profile.h:40: const
base::PersistentMemoryAllocator* memory_allocator);
On 2017/05/26 18:01:53, Alexei Svitkine (slow) wrote:
> If the param can't be null, pass by const ref instead of pointer.
> 
> Also, swap the param order, since system_profile is an out param.

For historical reasons, the persistent allocator is always passed as a pointer.

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
components/metrics/persistent_system_profile.h:48: kPadding,
On 2017/05/26 18:01:53, Alexei Svitkine (slow) wrote:
> This seems unused?

I was expecting to need it when more record types get implemented should there
be a little space at the end of a segment but I think it's now unnecessary. 
Removed.

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
components/metrics/persistent_system_profile.h:62: bool Matches(const
base::PersistentMemoryAllocator* allocator) const;
On 2017/05/26 18:01:53, Alexei Svitkine (slow) wrote:
> Nit: Add a comment.
> 
> Alternatively, how about just having an accessor for allocator() and then the
> caller could do the comparison themselves?

Done.

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
components/metrics/persistent_system_profile.h:66: void Reset();
On 2017/05/26 18:01:53, Alexei Svitkine (slow) wrote:
> Given you always call Reset() before Write(), how about combining the two?
(i.e.
> Reset() being an implementation detail of Write().

Once there are more record types than just the system profile, that will no
longer be the case.

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
components/metrics/persistent_system_profile.h:86: mutable uint32_t end_offset_;
     // End of data in block.
On 2017/05/26 18:01:53, Alexei Svitkine (slow) wrote:
> This field is not clear to me.
> 
> Why is it an offset, and not something like filled_size_?

It follows the STL begin/end naming.  It's not strictly the "filled" size
because the reading process uses the same offset to mark its place.

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
components/metrics/persistent_system_profile.h:95:
THREAD_CHECKER(thread_checker_);
On 2017/05/26 18:01:53, Alexei Svitkine (slow) wrote:
> Nit: Make this the first member.

Typically its the last member.

[commit-bot: I haz the power, 2017-05-29 19:20:55]

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

[commit-bot: I haz the power, 2017-05-29 19:20:56]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2017-05-29 19:48:32]

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
File components/metrics/persistent_system_profile.h (right):

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
components/metrics/persistent_system_profile.h:40: const
base::PersistentMemoryAllocator* memory_allocator);
On 2017/05/29 18:32:26, bcwhite wrote:
> On 2017/05/26 18:01:53, Alexei Svitkine (slow) wrote:
> > If the param can't be null, pass by const ref instead of pointer.
> > 
> > Also, swap the param order, since system_profile is an out param.
> 
> For historical reasons, the persistent allocator is always passed as a
pointer.

I don't see why it needs to be that way. The style guide says to pass things by
const ref unless there's a case they can be null. So please do that for new
code. If you have a ptr in the caller, you can just put a * before it to pass
here.

Also, please change the order of the params as well, since system_profile param
will be changed.

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
components/metrics/persistent_system_profile.h:95:
THREAD_CHECKER(thread_checker_);
On 2017/05/29 18:32:26, bcwhite wrote:
> On 2017/05/26 18:01:53, Alexei Svitkine (slow) wrote:
> > Nit: Make this the first member.
> 
> Typically its the last member.

That's not been my experience back with ThreadChecker (before the macro was
added). I don't thinking making it the last member makes sense because a) it's
not required and b) WeakPtrFactory needs to be the last member. So for classes
using both of these, it's not possible for both of them to be last member. So it
seems better to have it be the first member.

https://codereview.chromium.org/2907543003/diff/40001/components/metrics/metr...
File components/metrics/metrics_log.cc (right):

https://codereview.chromium.org/2907543003/diff/40001/components/metrics/metr...
components/metrics/metrics_log.cc:307: *system_profile);
This causes the system profile to be converted from proto to string two times -
once in your new code and once as part of EnvironmentRecorder.

Let's not duplicate this work - please refactor this code so that we only do
this once. For example, pass a ptr to the persistent sp to the
EnvironmentRecorder object and have it call SetSystemProfileData() on it.

https://codereview.chromium.org/2907543003/diff/40001/components/metrics/pers...
File components/metrics/persistent_system_profile.cc (right):

https://codereview.chromium.org/2907543003/diff/40001/components/metrics/pers...
components/metrics/persistent_system_profile.cc:71:
base::subtle::NoBarrier_Store(block, 0);
Can you use the RecordHeader type here? This way it makes it clear what it's
doing and removes an assumption from this function about size of RecordHeader.

[bcwhite, 2017-05-29 20:50:56]

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

[commit-bot: I haz the power, 2017-05-29 20:51:11]

Dry run: CQ is trying da patch.

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

[bcwhite, 2017-05-29 20:56:35]

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
File components/metrics/persistent_system_profile.h (right):

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
components/metrics/persistent_system_profile.h:40: const
base::PersistentMemoryAllocator* memory_allocator);
On 2017/05/29 19:48:32, Alexei Svitkine (slow) wrote:
> On 2017/05/29 18:32:26, bcwhite wrote:
> > On 2017/05/26 18:01:53, Alexei Svitkine (slow) wrote:
> > > If the param can't be null, pass by const ref instead of pointer.
> > > 
> > > Also, swap the param order, since system_profile is an out param.
> > 
> > For historical reasons, the persistent allocator is always passed as a
> pointer.
> 
> I don't see why it needs to be that way. The style guide says to pass things
by
> const ref unless there's a case they can be null. So please do that for new
> code. If you have a ptr in the caller, you can just put a * before it to pass
> here.
> 
> Also, please change the order of the params as well, since system_profile
param
> will be changed.

Done.

https://codereview.chromium.org/2907543003/diff/20001/components/metrics/pers...
components/metrics/persistent_system_profile.h:95:
THREAD_CHECKER(thread_checker_);
On 2017/05/29 19:48:32, Alexei Svitkine (slow) wrote:
> On 2017/05/29 18:32:26, bcwhite wrote:
> > On 2017/05/26 18:01:53, Alexei Svitkine (slow) wrote:
> > > Nit: Make this the first member.
> > 
> > Typically its the last member.
> 
> That's not been my experience back with ThreadChecker (before the macro was
> added). I don't thinking making it the last member makes sense because a) it's
> not required and b) WeakPtrFactory needs to be the last member. So for classes
> using both of these, it's not possible for both of them to be last member. So
it
> seems better to have it be the first member.

I checked a 1/2 dozen files before replying and all but one were at the end. 
The other was somewhere in the middle.

And I'm pretty sure I had a review comment once specifically telling me to put
it at the end.

https://codereview.chromium.org/2907543003/diff/40001/components/metrics/metr...
File components/metrics/metrics_log.cc (right):

https://codereview.chromium.org/2907543003/diff/40001/components/metrics/metr...
components/metrics/metrics_log.cc:307: *system_profile);
On 2017/05/29 19:48:32, Alexei Svitkine (slow) wrote:
> This causes the system profile to be converted from proto to string two times
-
> once in your new code and once as part of EnvironmentRecorder.
> 
> Let's not duplicate this work - please refactor this code so that we only do
> this once. For example, pass a ptr to the persistent sp to the
> EnvironmentRecorder object and have it call SetSystemProfileData() on it.

Turns out that SerializeAndRecordEnvironment returns the serialized proto.

Done.

https://codereview.chromium.org/2907543003/diff/40001/components/metrics/pers...
File components/metrics/persistent_system_profile.cc (right):

https://codereview.chromium.org/2907543003/diff/40001/components/metrics/pers...
components/metrics/persistent_system_profile.cc:71:
base::subtle::NoBarrier_Store(block, 0);
On 2017/05/29 19:48:32, Alexei Svitkine (slow) wrote:
> Can you use the RecordHeader type here? This way it makes it clear what it's
> doing and removes an assumption from this function about size of RecordHeader.

Done.

[commit-bot: I haz the power, 2017-05-29 22:15:12]

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

[commit-bot: I haz the power, 2017-05-29 22:15:13]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2017-05-29 22:27:24]

lgtm

https://codereview.chromium.org/2907543003/diff/60001/components/metrics/pers...
File components/metrics/persistent_system_profile.cc (right):

https://codereview.chromium.org/2907543003/diff/60001/components/metrics/pers...
components/metrics/persistent_system_profile.cc:268: if (allocators_.empty() ||
serialized_profile.empty())
If serialized_profile is empty, I think this should Reset().

[bcwhite, 2017-05-30 01:19:12]

https://codereview.chromium.org/2907543003/diff/60001/components/metrics/pers...
File components/metrics/persistent_system_profile.cc (right):

https://codereview.chromium.org/2907543003/diff/60001/components/metrics/pers...
components/metrics/persistent_system_profile.cc:268: if (allocators_.empty() ||
serialized_profile.empty())
On 2017/05/29 22:27:24, Alexei Svitkine (slow) wrote:
> If serialized_profile is empty, I think this should Reset().

The string can be empty if there is a serialization error.  I figured it would
be better to keep what was held previously as that should have the last valid
serialization plus all the collected modifications (that will appear in future
CLs).

[bcwhite, 2017-05-30 15:22:43]

The CQ bit was checked by bcwhite@chromium.org

[commit-bot: I haz the power, 2017-05-30 15:23:05]

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-05-30 15:26:47]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496157763083300,
"parent_rev": "9653e0f340d0f88313b233fb69b288398249f823", "commit_rev":
"4cde5b7b932ff3ae33107778f592fc21021a63c1"}

[commit-bot: I haz the power, 2017-05-30 15:26:53]

Description was changed from

==========
Support persistent system profiles.

This allows the entire system profile to be saved into one or more
persistent memory segments for use after the process exits.

The initial version saves only the snapshot known during metrics
reporting but later versions will build upon this to keep the profile
up-to-date in real time.

BUG=695880
==========

to

==========
Support persistent system profiles.

This allows the entire system profile to be saved into one or more
persistent memory segments for use after the process exits.

The initial version saves only the snapshot known during metrics
reporting but later versions will build upon this to keep the profile
up-to-date in real time.

BUG=695880

Review-Url: https://codereview.chromium.org/2907543003
Cr-Commit-Position: refs/heads/master@{#475534}
Committed:
https://chromium.googlesource.com/chromium/src/+/4cde5b7b932ff3ae33107778f592...
==========

[commit-bot: I haz the power, 2017-05-30 15:26:54]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/4cde5b7b932ff3ae33107778f592...