Flush stability file after initialization

Flush stability file after initialization

Maximize the chance of persisting minimal information (version details)
in stability files. This is to defend against system instability issues
for which the system would not persist the in memory content.

BUG=620813
Review-Url: https://codereview.chromium.org/2720353002
Cr-Commit-Position: refs/heads/master@{#455133}
Committed: https://chromium.googlesource.com/chromium/src/+/e7ea29da6f1af5d6c75be744254558786100ef8f

[manzagop (departed), 2017-02-28 22:00:23]

manzagop@chromium.org changed reviewers:
+ siggi@chromium.org

[manzagop (departed), 2017-02-28 22:00:23]

Please have a look. I am thinking this should be unconditional.

[Sigurður Ásgeirsson, 2017-02-28 23:20:31]

This should be happening at early startup, which is a sensitive performance
path. Flushing a mapping to disk is a synchronous operation, which should
never be done lightly if it's happening on the main thread.


On Tue, Feb 28, 2017 at 5:00 PM <manzagop@chromium.org> wrote:

> Reviewers: Sigurður Ásgeirsson
> CL: https://codereview.chromium.org/2720353002/
>
> Message:
> Please have a look. I am thinking this should be unconditional.
>
> Description:
> Flush stability file after initialization
>
> Maximize the chance of persisting minimal information (version details)
> in stability files. This is to defend against system instability issues
> for which the system would not persist the in memory content.
>
> BUG=620813
>
> Affected files (+7, -0 lines):
> M chrome/browser/chrome_browser_field_trials_desktop.cc
>
>
> Index: chrome/browser/chrome_browser_field_trials_desktop.cc
> diff --git a/chrome/browser/chrome_browser_field_trials_desktop.cc
> b/chrome/browser/chrome_browser_field_trials_desktop.cc
> index
>
47715a06258dff1e92f36cdbb9c33c3b44687a00..e61c1fdb1e99607b5703e681eec2db9f281dd56e
> 100644
> --- a/chrome/browser/chrome_browser_field_trials_desktop.cc
> +++ b/chrome/browser/chrome_browser_field_trials_desktop.cc
> @@ -175,6 +175,13 @@ void SetupStabilityDebugging() {
>
> // Record information about chrome's module. We want this to be done early.
> RecordChromeModuleInfo(global_tracker);
> +
> + // Trigger a flush of the memory mapped file to maximize the chances of
> + // having a minimal amount of content in the stability file, even if
> + // the system crashes or loses power. Note: this does not flush the file
> + // metadata nor does it wait for the changes to be flushed to disk before
> + // returning.
> + ::FlushViewOfFile(global_tracker->allocator()->data(), 0U);
> }
> }
> #endif // defined(OS_WIN)
>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[manzagop (departed), 2017-03-01 22:40:21]

Would you be ok with running this change under a short term canary-only
experiment to measure the impact on report collection result and time?

[Sigurður Ásgeirsson, 2017-03-01 22:44:54]

Yeah, that'd be a super-fun experiment. Talk to Chris, who knows who now
cares about^W^Wmeasures performance, to see where you ought to be looking.

On Wed, Mar 1, 2017 at 5:40 PM <manzagop@chromium.org> wrote:

> Would you be ok with running this change under a short term canary-only
> experiment to measure the impact on report collection result and time?
>
> https://codereview.chromium.org/2720353002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[bcwhite, 2017-03-06 13:59:17]

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

[bcwhite, 2017-03-06 13:59:17]

https://codereview.chromium.org/2720353002/diff/1/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_field_trials_desktop.cc (right):

https://codereview.chromium.org/2720353002/diff/1/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_field_trials_desktop.cc:184:
::FlushViewOfFile(global_tracker->allocator()->data(), 0U);
How about making this a method off of GlobalActivityTracker so it can be
cross-platform and reusable.

Perhaps with a boolean "synchronous" flag that waits for the flush?

[manzagop (departed), 2017-03-06 16:46:26]

Thanks! Another look?

https://codereview.chromium.org/2720353002/diff/1/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_field_trials_desktop.cc (right):

https://codereview.chromium.org/2720353002/diff/1/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_field_trials_desktop.cc:184:
::FlushViewOfFile(global_tracker->allocator()->data(), 0U);
On 2017/03/06 13:59:17, bcwhite wrote:
> How about making this a method off of GlobalActivityTracker so it can be
> cross-platform and reusable.
> 
> Perhaps with a boolean "synchronous" flag that waits for the flush?

I guess GlobalActivityTracker would have a flush that calls to the allocator's
flush that only has an effect if it's a FilePersistentMemoryAllocator? And the
actual flushing would be in base::MemoryMappedFile?

If you agree, maybe we can start with this and in parallel, have another CL to
add flushing to base::MemoryMappedFile?

[manzagop (departed), 2017-03-06 16:47:09]

manzagop@chromium.org changed reviewers:
+ rkaplow@chromium.org

[manzagop (departed), 2017-03-06 16:47:09]

Hi Rob,

Could you have an OWNERS' look at:
    chrome\browser\chrome_browser_field_trials_desktop.cc
    tools\metrics\histograms\histograms.xml

Thanks!

Pierre

[Sigurður Ásgeirsson, 2017-03-06 17:45:28]

lgtm

[rkaplow, 2017-03-07 05:35:06]

lgtm

https://codereview.chromium.org/2720353002/diff/20001/components/browser_watc...
File components/browser_watcher/features.cc (right):

https://codereview.chromium.org/2720353002/diff/20001/components/browser_watc...
components/browser_watcher/features.cc:12: const base::Feature
kStabilityDebuggingFlushFeature{
wouldn't mind one-liners explaining what these features will drigger

[manzagop (departed), 2017-03-07 15:24:21]

Thanks! Going ahead with submission.

https://codereview.chromium.org/2720353002/diff/20001/components/browser_watc...
File components/browser_watcher/features.cc (right):

https://codereview.chromium.org/2720353002/diff/20001/components/browser_watc...
components/browser_watcher/features.cc:12: const base::Feature
kStabilityDebuggingFlushFeature{
On 2017/03/07 05:35:06, rkaplow (slow) wrote:
> wouldn't mind one-liners explaining what these features will drigger

Done.

[manzagop (departed), 2017-03-07 15:24:27]

The CQ bit was checked by manzagop@chromium.org

[manzagop (departed), 2017-03-07 15:24:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
rkaplow@chromium.org, siggi@chromium.org
Link to the patchset: https://codereview.chromium.org/2720353002/#ps60001
(title: "Address rkaplow comments")

[commit-bot: I haz the power, 2017-03-07 15:24:58]

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-03-07 18:09:48]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488900267917310,
"parent_rev": "83621ef58f633413145bd7093f16d56eb2c88103", "commit_rev":
"e7ea29da6f1af5d6c75be744254558786100ef8f"}

[commit-bot: I haz the power, 2017-03-07 18:10:46]

Description was changed from

==========
Flush stability file after initialization

Maximize the chance of persisting minimal information (version details)
in stability files. This is to defend against system instability issues
for which the system would not persist the in memory content.

BUG=620813
==========

to

==========
Flush stability file after initialization

Maximize the chance of persisting minimal information (version details)
in stability files. This is to defend against system instability issues
for which the system would not persist the in memory content.

BUG=620813

Review-Url: https://codereview.chromium.org/2720353002
Cr-Commit-Position: refs/heads/master@{#455133}
Committed:
https://chromium.googlesource.com/chromium/src/+/e7ea29da6f1af5d6c75be7442545...
==========

[commit-bot: I haz the power, 2017-03-07 18:10:47]

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

[bcwhite, 2017-03-07 18:37:26]

https://codereview.chromium.org/2720353002/diff/1/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_field_trials_desktop.cc (right):

https://codereview.chromium.org/2720353002/diff/1/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_field_trials_desktop.cc:184:
::FlushViewOfFile(global_tracker->allocator()->data(), 0U);
On 2017/03/06 16:46:26, manzagop wrote:
> On 2017/03/06 13:59:17, bcwhite wrote:
> > How about making this a method off of GlobalActivityTracker so it can be
> > cross-platform and reusable.
> > 
> > Perhaps with a boolean "synchronous" flag that waits for the flush?
> 
> I guess GlobalActivityTracker would have a flush that calls to the allocator's
> flush that only has an effect if it's a FilePersistentMemoryAllocator? And the
> actual flushing would be in base::MemoryMappedFile?
> 
> If you agree, maybe we can start with this and in parallel, have another CL to
> add flushing to base::MemoryMappedFile?

Yeah...  It would have to be a virtual method on the PMA.  Best to do that in a
different CL.  I'll look into it.