Add a metric to Chrome to measure fragmentation of chrome.dll at startup.BUG=98033TEST=None

chrome/browser/fragmentation_checker_win.cc

+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/fragmentation_checker_win.h"
+
+#include <windows.h>
+#include <winioctl.h>
+
+#include "base/at_exit.h"
+#include "base/basictypes.h"
+#include "base/command_line.h"
+#include "base/file_util.h"
+#include "base/logging.h"
+#include "base/metrics/histogram.h"
+#include "base/path_service.h"
+
+namespace {
+
+size_t kMaxBufferSize = 1 << 20;

[grt (UTC plus 2), 2011/09/30 18:44:14]

const size_t...

[robertshield, 2011/09/30 19:53:00]

Done.

+
+struct FileExtentData {
+  int number_of_extents;
+  int64 virtual_cluster_span;
+};
+
+size_t ComputeRetrievalPointersBufferSize(int number_of_extents) {
+  return sizeof(RETRIEVAL_POINTERS_BUFFER) +
+        (number_of_extents - 1) * (sizeof(LARGE_INTEGER) * 2);

[grt (UTC plus 2), 2011/09/30 18:44:14]

indentation

[grt (UTC plus 2), 2011/09/30 18:44:14]

is it possible to replace sizeof(LARGE_INTEGER) * 2 with something like
RETRIEVAL_POINTERS_BUFFER::Extents?  or something silly like that?

[robertshield, 2011/09/30 19:53:00]

Done.

[robertshield, 2011/09/30 19:53:00]

Done.

+}
+
+bool DumpFileExtents(const FilePath& file_path,
+                     FileExtentData* file_extent_data) {
+  DCHECK(file_extent_data);
+  if (!file_util::PathExists(file_path)) {

[grt (UTC plus 2), 2011/09/30 18:44:14]

why do this extra check?  CreatePlatformFile should fail just fine if the file
doesn't exist.  if you want to log that, check for PLATFORM_FILE_ERROR_NOT_FOUND
when CreatePlatformFile fails.

[robertshield, 2011/09/30 19:53:00]

Done.

+    LOG(ERROR) << "File not found.";
+    return false;
+  }
+
+  bool success = false;
+
+  base::PlatformFileError error_code = base::PLATFORM_FILE_ERROR_FAILED;
+  bool created = false;
+  base::PlatformFile file_handle = CreatePlatformFile(
+      file_path,
+      base::PLATFORM_FILE_OPEN | base::PLATFORM_FILE_READ,
+      &created,
+      &error_code);
+  if (error_code == base::PLATFORM_FILE_OK && !created) {
+    STARTING_VCN_INPUT_BUFFER starting_vcn_input_buffer = {0};
+
+    // Compute an output size capable of holding 16 extents at first. This may
+    // fail when the number of extents exceeds 16, in which case we make
+    // a bigger buffer up to kMaxBufferSize.
+    int number_of_extents = 16;
+    std::vector<BYTE> retrieval_pointers_buffer;

[grt (UTC plus 2), 2011/09/30 18:44:14]

uint8 rather than win-specific BYTE?

[robertshield, 2011/09/30 19:53:00]

Done, although this file is (and can only be) Windows-only.

+
+    size_t output_size = ComputeRetrievalPointersBufferSize(number_of_extents);

[grt (UTC plus 2), 2011/09/30 18:44:14]

do this computation before line 56, then change line 56 to construct the buffer
with the proper size:
std::vector<uint8> retrieval_pointers_buffer(output_size);

[robertshield, 2011/09/30 19:53:00]

Done.

+    retrieval_pointers_buffer.resize(output_size);
+
+    DWORD bytes_returned = 0;
+
+    bool result = false;
+    do {
+      SetLastError(ERROR_SUCCESS);

[grt (UTC plus 2), 2011/09/30 18:44:14]

this is extremely odd.  please document why this is needed.

[robertshield, 2011/09/30 19:53:00]

rejiggered as discussed.

+      result = DeviceIoControl(

[grt (UTC plus 2), 2011/09/30 18:44:14]

MSDN says that the call fails (returns 0) if the buffer is too small, yet that's
not how you're using it.  have you found that MSDN is lying?  if so, please
document why you're not following the conventions described in MSDN.

[robertshield, 2011/09/30 19:53:00]

I found documentation suggesting using the GLE code rather than the return
value, but have updated this to be more conventional.

+          file_handle,
+          FSCTL_GET_RETRIEVAL_POINTERS,
+          reinterpret_cast<void*>(&starting_vcn_input_buffer),
+          sizeof(starting_vcn_input_buffer),
+          reinterpret_cast<void*>(&retrieval_pointers_buffer[0]),
+          retrieval_pointers_buffer.size(),
+          &bytes_returned,
+          NULL) == TRUE;

[grt (UTC plus 2), 2011/09/30 18:44:14]

== TRUE -> != 0
MSDN says it returns zero/non-zero, not FALSE/TRUE.  even if that weren't the
case, it's safer to compare to FALSE/0 than the converse.

[robertshield, 2011/09/30 19:53:00]

My version of WinBase.h says it returns FALSE/TRUE and I trust that more. 
Changed to != FALSE.

+
+      DWORD last_error = GetLastError();

[grt (UTC plus 2), 2011/09/30 18:44:14]

this is extremely odd.  according to MSDN, you would only need to call GLE if
DeviceIoControl returned 0, yet here you call it without checking the return
value of DeviceIoControl.  please document why this is needed.

[robertshield, 2011/09/30 19:53:00]

Changed as discussed.

+      if (last_error == ERROR_MORE_DATA) {
+        // Grow the extents we can handle
+        number_of_extents *= 2;
+        output_size = ComputeRetrievalPointersBufferSize(number_of_extents);
+
+        if (output_size > kMaxBufferSize) {
+          LOG(ERROR) << "FSCTL_GET_RETRIEVAL_POINTERS output buffer exceeded "
+                        "maximum size.";
+          result = false;
+          break;
+        }
+
+        retrieval_pointers_buffer.clear();
+        retrieval_pointers_buffer.resize(output_size);

[grt (UTC plus 2), 2011/09/30 18:44:14]

replace these two lines with a call to .assign(output_size, 0)

[robertshield, 2011/09/30 19:53:00]

Done.

+      } else {
+        if (!result) {
+          LOG(ERROR) << "FSCTL_GET_RETRIEVAL_POINTERS failed. gle = "

[grt (UTC plus 2), 2011/09/30 18:44:14]

PLOG(ERROR) << "FSCTL_GET_RETRIEVAL_POINTERS failed.";

[robertshield, 2011/09/30 19:53:00]

Done.

+                     << last_error;
+        }
+        break;
+      }
+    } while (true);
+
+    if (result) {
+      RETRIEVAL_POINTERS_BUFFER* retrieval_pointers =
+          reinterpret_cast<RETRIEVAL_POINTERS_BUFFER*>(
+              &retrieval_pointers_buffer[0]);
+      DWORD extent_count = retrieval_pointers->ExtentCount;
+      LARGE_INTEGER extent_cluster_range = {0};
+      if (extent_count > 0) {
+        extent_cluster_range.QuadPart =
+            retrieval_pointers->Extents[extent_count - 1].NextVcn.QuadPart;
+        extent_cluster_range.QuadPart -=
+            retrieval_pointers->StartingVcn.QuadPart;
+      }
+
+      file_extent_data->number_of_extents = extent_count;

[grt (UTC plus 2), 2011/09/30 18:44:14]

this does (in essence) a uint32 -> int conversion.  please make it safe.

[robertshield, 2011/09/30 19:53:00]

Done.

+      file_extent_data->virtual_cluster_span = extent_cluster_range.QuadPart;
+      success = true;
+    } else {
+      LOG(ERROR) << "Failed to retrieve extents.";
+    }
+  }
+
+  return success;
+}
+
+}  // namespace
+
+namespace fragmentation_checker {
+
+void RecordFragmentationMetricForCurrentModule() {
+  FilePath module_path;

[grt (UTC plus 2), 2011/09/30 18:44:14]

#include "base/file_path.h" for this

[robertshield, 2011/09/30 19:53:00]

Done.

+  if (PathService::Get(base::FILE_MODULE, &module_path)) {
+    FileExtentData file_extent_data = {0};
+    if (DumpFileExtents(module_path, &file_extent_data)) {
+      UMA_HISTOGRAM_COUNTS("Fragmentation.ModuleExtents",
+                           file_extent_data.number_of_extents);
+      UMA_HISTOGRAM_COUNTS("Fragmentation.ModuleSpan",
+                           file_extent_data.virtual_cluster_span);

[grt (UTC plus 2), 2011/09/30 18:44:14]

1) this macro sets the max sample to 1000000, which is a lot less than what
virtual_cluster_span can potentially hold.  perhaps you should use CUSTOM_COUNTS
here with Histogram::kSampleType_MAX?  Maybe you want more buckets in that case?
2) you're passing an int64 into something that takes an int.  i think you need
to compress down to an int somehow.

[robertshield, 2011/09/30 19:53:00]

As discussed, while reading the documentation again I realized that the
virtual_cluster_span metric isn't actually useful. I'd need to map that to
logical addresses on disk which requires querying the volume for information,
which requires a volume handle which requires admin. So I'll drop that and leave
the number_of_extents as the only metric being reported for now.

+    }
+  } else {
+    NOTREACHED() << "Could not get path to current module.";
+  }
+}
+
+}  // namespace fragmentation_checker