Skip to content

Commit

Permalink
Add some words of caution to File::Flush().
Browse files Browse the repository at this point in the history
Document latency percentiles.

BUG=473337

Review-Url: https://codereview.chromium.org/2171833002
Cr-Commit-Position: refs/heads/master@{#407305}
  • Loading branch information
thiemonagel authored and Commit bot committed Jul 22, 2016
1 parent dcd4167 commit 301e81e
Show file tree
Hide file tree
Showing 5 changed files with 18 additions and 14 deletions.
8 changes: 0 additions & 8 deletions base/files/file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -138,12 +138,4 @@ std::string File::ErrorToString(Error error) {
return "";
}

bool File::Flush() {
ElapsedTimer timer;
SCOPED_FILE_TRACE("Flush");
bool return_value = DoFlush();
UMA_HISTOGRAM_TIMES("PlatformFile.FlushTime", timer.Elapsed());
return return_value;
}

} // namespace base
14 changes: 10 additions & 4 deletions base/files/file.h
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,16 @@ class BASE_EXPORT File {

// Instructs the filesystem to flush the file to disk. (POSIX: fsync, Windows:
// FlushFileBuffers).
// Calling Flush() does not guarantee file integrity and thus is not a valid
// substitute for file integrity checks and recovery codepaths for malformed
// files. It can also be *really* slow, so avoid blocking on Flush(),
// especially please don't block shutdown on Flush().
// Latency percentiles of Flush() across all platforms as of July 2016:
// 50 % > 5 ms
// 10 % > 58 ms
// 1 % > 357 ms
// 0.1 % > 1.8 seconds
// 0.01 % > 7.6 seconds
bool Flush();

// Updates the file times.
Expand Down Expand Up @@ -310,10 +320,6 @@ class BASE_EXPORT File {
// traversal ('..') components.
void DoInitialize(const FilePath& path, uint32_t flags);

// TODO(tnagel): Reintegrate into Flush() once histogram isn't needed anymore,
// cf. issue 473337.
bool DoFlush();

void SetPlatformFile(PlatformFile file);

#if defined(OS_WIN)
Expand Down
3 changes: 2 additions & 1 deletion base/files/file_posix.cc
Original file line number Diff line number Diff line change
Expand Up @@ -511,9 +511,10 @@ void File::DoInitialize(const FilePath& path, uint32_t flags) {
}
#endif // !defined(OS_NACL)

bool File::DoFlush() {
bool File::Flush() {
ThreadRestrictions::AssertIOAllowed();
DCHECK(IsValid());
SCOPED_FILE_TRACE("Flush");

#if defined(OS_NACL)
NOTIMPLEMENTED(); // NaCl doesn't implement fsync.
Expand Down
3 changes: 2 additions & 1 deletion base/files/file_win.cc
Original file line number Diff line number Diff line change
Expand Up @@ -396,9 +396,10 @@ void File::DoInitialize(const FilePath& path, uint32_t flags) {
}
}

bool File::DoFlush() {
bool File::Flush() {
ThreadRestrictions::AssertIOAllowed();
DCHECK(IsValid());
SCOPED_FILE_TRACE("Flush");
return ::FlushFileBuffers(file_.Get()) != FALSE;
}

Expand Down
4 changes: 4 additions & 0 deletions tools/metrics/histograms/histograms.xml
Original file line number Diff line number Diff line change
Expand Up @@ -40975,6 +40975,10 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
</histogram>

<histogram name="PlatformFile.FlushTime" units="ms">
<obsolete>
Deprecated as of 2016-07 because the histogram's purpose of adding colour to
the description of File::Flush() has been fulfilled.
</obsolete>
<owner>tnagel@chromium.org</owner>
<summary>The time it takes to run File::Flush().</summary>
</histogram>
Expand Down

0 comments on commit 301e81e

Please sign in to comment.