Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1069473003:
Detect seek penalties on Mac. (Closed)
DescriptionDetect seek penalties on Mac.
R=asvitkine@chromium.org
BUG=463209
Committed: https://crrev.com/5a1bb0b8a37b744aef72dcd4a4e5df8cde7c360e
Cr-Commit-Position: refs/heads/master@{#324519}
Patch Set 1 #
Total comments: 20
Patch Set 2 : groby@ review #Patch Set 3 : kIOPropertyMediumTypeSolidStateKey #Patch Set 4 : whoops #Patch Set 5 : XCode 10.6 SDK #
Total comments: 4
Patch Set 6 : asvitkine@ review #Messages
Total messages: 25 (8 generated)
so some confusing bits to me: - lots of file-related things are deprecated as of 10.8 - many new things replace ^ these deprecated things in 10.8+ do we need to write a < 10.8 version that doesn't use disk arbitration? I sure hope not... https://codereview.chromium.org/1069473003/diff/1/chrome/browser/metrics/driv... File chrome/browser/metrics/drive_metrics_provider_mac.mm (right): https://codereview.chromium.org/1069473003/diff/1/chrome/browser/metrics/driv... chrome/browser/metrics/drive_metrics_provider_mac.mm:9: #import <Foundation/NSURL.h> full disclosure: i little idea when to #include vs #import
https://codereview.chromium.org/1069473003/diff/1/chrome/browser/metrics/driv... File chrome/browser/metrics/drive_metrics_provider_mac.mm (right): https://codereview.chromium.org/1069473003/diff/1/chrome/browser/metrics/driv... chrome/browser/metrics/drive_metrics_provider_mac.mm:34: DADiskCreateFromVolumePath(nullptr, session, (CFURLRef)volume)); i don't know our stance on toll-free bridging... or how we generally do it https://codereview.chromium.org/1069473003/diff/1/chrome/browser/metrics/driv... chrome/browser/metrics/drive_metrics_provider_mac.mm:44: kCFAllocatorDefault, also i wasn't sure when to pass kCFAllocatorDefault vs nullptr, so i just did what the Chromium-specific uses of these functions did
groby@chromium.org changed reviewers: + groby@chromium.org
https://codereview.chromium.org/1069473003/diff/1/chrome/browser/metrics/driv... File chrome/browser/metrics/drive_metrics_provider_mac.mm (right): https://codereview.chromium.org/1069473003/diff/1/chrome/browser/metrics/driv... chrome/browser/metrics/drive_metrics_provider_mac.mm:9: #import <Foundation/NSURL.h> On 2015/04/08 00:15:54, Dan Beam wrote: > full disclosure: i little idea when to #include vs #import You guessed well :) (If it's ObjC stuff, it's import, otherwise #include. I.e. square brackets == import ;) https://codereview.chromium.org/1069473003/diff/1/chrome/browser/metrics/driv... chrome/browser/metrics/drive_metrics_provider_mac.mm:9: #import <Foundation/NSURL.h> Instead of picking on NSUrl, just use #import <Cocoa/Cocoa.h> That's the default Chromium pattern. https://codereview.chromium.org/1069473003/diff/1/chrome/browser/metrics/driv... chrome/browser/metrics/drive_metrics_provider_mac.mm:22: if (!url) Can't return nil https://codereview.chromium.org/1069473003/diff/1/chrome/browser/metrics/driv... chrome/browser/metrics/drive_metrics_provider_mac.mm:25: NSURL* volume; Cocoa usually spells out the type, so volumeUrl https://codereview.chromium.org/1069473003/diff/1/chrome/browser/metrics/driv... chrome/browser/metrics/drive_metrics_provider_mac.mm:29: base::ScopedCFTypeRef<DASessionRef> session(DASessionCreate(nullptr)); Please don't nullptr - kCFAllocatorDefault (It's the same, but more readable) https://codereview.chromium.org/1069473003/diff/1/chrome/browser/metrics/driv... chrome/browser/metrics/drive_metrics_provider_mac.mm:34: DADiskCreateFromVolumePath(nullptr, session, (CFURLRef)volume)); On 2015/04/08 00:17:35, Dan Beam wrote: > i don't know our stance on toll-free bridging... or how we generally do it base::mac::NSToCFCast(volumeUrl) https://codereview.chromium.org/1069473003/diff/1/chrome/browser/metrics/driv... chrome/browser/metrics/drive_metrics_provider_mac.mm:34: DADiskCreateFromVolumePath(nullptr, session, (CFURLRef)volume)); nullptr is kCFAllocatorDefault https://codereview.chromium.org/1069473003/diff/1/chrome/browser/metrics/driv... chrome/browser/metrics/drive_metrics_provider_mac.mm:44: kCFAllocatorDefault, On 2015/04/08 00:17:35, Dan Beam wrote: > also i wasn't sure when to pass kCFAllocatorDefault vs nullptr, so i just did > what the Chromium-specific uses of these functions did Heh - see above :) https://codereview.chromium.org/1069473003/diff/1/chrome/browser/metrics/driv... chrome/browser/metrics/drive_metrics_provider_mac.mm:54: return [@"Solid State" isEqualToString:(NSString*)type]; Eeeeeeew. base::mac::CFToNSCast, please. :)
https://codereview.chromium.org/1069473003/diff/1/chrome/browser/metrics/driv... File chrome/browser/metrics/drive_metrics_provider_mac.mm (right): https://codereview.chromium.org/1069473003/diff/1/chrome/browser/metrics/driv... chrome/browser/metrics/drive_metrics_provider_mac.mm:9: #import <Foundation/NSURL.h> On 2015/04/08 00:41:02, groby wrote: > On 2015/04/08 00:15:54, Dan Beam wrote: > > full disclosure: i little idea when to #include vs #import > > You guessed well :) (If it's ObjC stuff, it's import, otherwise #include. I.e. > square brackets == import ;) > that was actually exactly my reasoning, lol https://codereview.chromium.org/1069473003/diff/1/chrome/browser/metrics/driv... chrome/browser/metrics/drive_metrics_provider_mac.mm:9: #import <Foundation/NSURL.h> On 2015/04/08 00:41:02, groby wrote: > Instead of picking on NSUrl, just use > #import <Cocoa/Cocoa.h> > > That's the default Chromium pattern. Done. https://codereview.chromium.org/1069473003/diff/1/chrome/browser/metrics/driv... chrome/browser/metrics/drive_metrics_provider_mac.mm:22: if (!url) On 2015/04/08 00:41:02, groby wrote: > Can't return nil Done. https://codereview.chromium.org/1069473003/diff/1/chrome/browser/metrics/driv... chrome/browser/metrics/drive_metrics_provider_mac.mm:25: NSURL* volume; On 2015/04/08 00:41:02, groby wrote: > Cocoa usually spells out the type, so volumeUrl Done. https://codereview.chromium.org/1069473003/diff/1/chrome/browser/metrics/driv... chrome/browser/metrics/drive_metrics_provider_mac.mm:29: base::ScopedCFTypeRef<DASessionRef> session(DASessionCreate(nullptr)); On 2015/04/08 00:41:02, groby wrote: > Please don't nullptr - kCFAllocatorDefault (It's the same, but more readable) Done. https://codereview.chromium.org/1069473003/diff/1/chrome/browser/metrics/driv... chrome/browser/metrics/drive_metrics_provider_mac.mm:34: DADiskCreateFromVolumePath(nullptr, session, (CFURLRef)volume)); On 2015/04/08 00:41:02, groby wrote: > On 2015/04/08 00:17:35, Dan Beam wrote: > > i don't know our stance on toll-free bridging... or how we generally do it > > base::mac::NSToCFCast(volumeUrl) Done. https://codereview.chromium.org/1069473003/diff/1/chrome/browser/metrics/driv... chrome/browser/metrics/drive_metrics_provider_mac.mm:34: DADiskCreateFromVolumePath(nullptr, session, (CFURLRef)volume)); On 2015/04/08 00:41:02, groby wrote: > nullptr is kCFAllocatorDefault Done. https://codereview.chromium.org/1069473003/diff/1/chrome/browser/metrics/driv... chrome/browser/metrics/drive_metrics_provider_mac.mm:54: return [@"Solid State" isEqualToString:(NSString*)type]; On 2015/04/08 00:41:02, groby wrote: > Eeeeeeew. base::mac::CFToNSCast, please. :) Done.
if anybody wants to try this locally, apply this issue as well as: https://gist.githubusercontent.com/anonymous/05164e84eac888460ddb/raw/9bb8bfd... and look at "HasSeekPenalty" and related metrics in about:histograms (after they've been logged, usually takes ~20s)
The CQ bit was checked by dbeam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1069473003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
dbeam@chromium.org changed reviewers: - groby@chromium.org
ping asvitkine@
The CQ bit was checked by dbeam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1069473003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/1069473003/diff/80001/chrome/browser/metrics/... File chrome/browser/metrics/drive_metrics_provider_mac.mm (right): https://codereview.chromium.org/1069473003/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/drive_metrics_provider_mac.mm:28: if (bsd_name.empty() || bsd_name == "null") According to devname() make page, it can return NULL on error. Please check that explicitly on the return value, rather than passing it to the std::string constructor and possibly checking if it converts to the string "null" (?). https://codereview.chromium.org/1069473003/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/drive_metrics_provider_mac.mm:31: bsd_name.insert(0, "/dev/"); Nit: I suggest just initializing bsd_name to "/dev/" and then using append() on the value returned from devname() (after checking for NULL).
https://codereview.chromium.org/1069473003/diff/80001/chrome/browser/metrics/... File chrome/browser/metrics/drive_metrics_provider_mac.mm (right): https://codereview.chromium.org/1069473003/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/drive_metrics_provider_mac.mm:28: if (bsd_name.empty() || bsd_name == "null") On 2015/04/09 20:21:12, Alexei Svitkine wrote: > According to devname() make page, it can return NULL on error. Please check that > explicitly on the return value, rather than passing it to the std::string > constructor and possibly checking if it converts to the string "null" (?). ah, printf("%s", null) confused me. fixed. https://codereview.chromium.org/1069473003/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/drive_metrics_provider_mac.mm:31: bsd_name.insert(0, "/dev/"); On 2015/04/09 20:21:12, Alexei Svitkine wrote: > Nit: I suggest just initializing bsd_name to "/dev/" and then using append() on > the value returned from devname() (after checking for NULL). Done.
lgtm
The CQ bit was checked by dbeam@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1069473003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/5a1bb0b8a37b744aef72dcd4a4e5df8cde7c360e Cr-Commit-Position: refs/heads/master@{#324519}
Message was sent while issue was closed.
groby@chromium.org changed reviewers: + groby@chromium.org
Message was sent while issue was closed.
FWIW, finally ran it on my spinning rust machine, and HasSeekPenalty is successful and detects properly. |
