Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1927003004:
Mac: Introduce "StaticCast" variants of base::mac::FooCastStrict, which do not typecheck in Release (Closed)
DescriptionMac: Introduce "StaticCast" variants of base::mac::FooCastStrict, which do not typecheck in Release
The FooCastStrict methods in foundation_util.h DCHECK on a failed cast.
In Release the DCHECK is skipped, but the type is still checked (and nil
is returned on failure). This adds a runtime cost and hides potential
Release-only bugs that may be getting absorbed by the runtime.
Provide CFStaticCast and ObjCStaticCast which, in Release, just do the
cast and are always inlineable.
Context: this started with an investigation of a performance bottleneck
in RenderTextMac which flagged calls to NSClassFromString(@"NSFont") as
being surprisingly expensive.
BUG=605131
Patch Set 1 #Patch Set 2 : Remove typechecking from all the FooCastStrict variants #Patch Set 3 : Make CFCastStrict inlineable #Patch Set 4 : rebase #
Total comments: 2
Patch Set 5 : Split out NSClassFromString stuff #
Total comments: 3
Patch Set 6 : Add StaticCast variants #Messages
Total messages: 16 (6 generated)
Description was changed from ========== Do not use NSClassFromString for CFCast in Release for CTFont BUG=605131 ========== to ========== Mac: Remove typechecking in Release for the FooCastStrict variants in foundation_util.h The FooCastStrict methods in foundation_util.h DCHECK on a failed cast. In Release the DCHECK is skipped, but the type is still checked (and nil is returned on failure). This adds a runtime cost and hides potential Release-only bugs that may be getting absorbed by the runtime. Instead, in Release, just do the cast and ensure it is inlineable. BUG=605131 ==========
tapted@chromium.org changed reviewers: + thakis@chromium.org
Description was changed from ========== Mac: Remove typechecking in Release for the FooCastStrict variants in foundation_util.h The FooCastStrict methods in foundation_util.h DCHECK on a failed cast. In Release the DCHECK is skipped, but the type is still checked (and nil is returned on failure). This adds a runtime cost and hides potential Release-only bugs that may be getting absorbed by the runtime. Instead, in Release, just do the cast and ensure it is inlineable. BUG=605131 ========== to ========== Mac: Remove typechecking in Release for the FooCastStrict variants in foundation_util.h The FooCastStrict methods in foundation_util.h DCHECK on a failed cast. In Release the DCHECK is skipped, but the type is still checked (and nil is returned on failure). This adds a runtime cost and hides potential Release-only bugs that may be getting absorbed by the runtime. Instead, in Release, just do the cast and ensure it is inlineable. Context: this started with an investigation of a performance bottleneck in RenderTextMac with flagged calls to NSClassFromString(@"NSFont") as being surprisingly expensive. BUG=605131 ==========
Description was changed from ========== Mac: Remove typechecking in Release for the FooCastStrict variants in foundation_util.h The FooCastStrict methods in foundation_util.h DCHECK on a failed cast. In Release the DCHECK is skipped, but the type is still checked (and nil is returned on failure). This adds a runtime cost and hides potential Release-only bugs that may be getting absorbed by the runtime. Instead, in Release, just do the cast and ensure it is inlineable. Context: this started with an investigation of a performance bottleneck in RenderTextMac with flagged calls to NSClassFromString(@"NSFont") as being surprisingly expensive. BUG=605131 ========== to ========== Mac: Remove typechecking in Release for the FooCastStrict variants in foundation_util.h The FooCastStrict methods in foundation_util.h DCHECK on a failed cast. In Release the DCHECK is skipped, but the type is still checked (and nil is returned on failure). This adds a runtime cost and hides potential Release-only bugs that may be getting absorbed by the runtime. Instead, in Release, just do the cast and ensure it is inlineable. Context: this started with an investigation of a performance bottleneck in RenderTextMac which flagged calls to NSClassFromString(@"NSFont") as being surprisingly expensive. BUG=605131 ==========
Hi Nico - WDYT? Note there is a behaviour change in Release -- now returns non-nil on a failed cast -- that could throw some new stuff at the crash server, but I think it will ultimately lead to more robust code. Also note this CL evolved a bit while exploring a solution - I've left the earlier stuff in for context and to link it back to the bug, but I'm happy to split this up. Thanks!
In general I think this is cool, but a) Mento wrote this, so he should probably look at this b) CFCastStrict is now less strict than CFCast (which still does the typechecking), which seems a bit weird (?) https://codereview.chromium.org/1927003004/diff/60001/base/mac/foundation_uti... File base/mac/foundation_util.mm (right): https://codereview.chromium.org/1927003004/diff/60001/base/mac/foundation_uti... base/mac/foundation_util.mm:323: [ns_val isKindOfClass:[NSFont class]])); Splitting these NSClassFromString("foo") -> [foo class] changes out makes sense to me (you can tbr that against me, this looks good)
tapted@chromium.org changed reviewers: + mark@chromium.org
+mento - WDYT? On 2016/04/29 02:50:57, Nico wrote: > In general I think this is cool, but > a) Mento wrote this, so he should probably look at this > b) CFCastStrict is now less strict than CFCast (which still does the > typechecking), which seems a bit weird (?) How about renaming CFCast to CFCastChecked? or.. even CFCastDynamic, since its behaviour is a lot like dynamic_cast<>. It's also likely that in that process, a lot of places calling CFCast can just be changed to CFCastStrict, since if a caller isn't checking whether CFCast returns null, there's not really any reason to prefer it over CFCastStrict. Another option (with a lot more churn): * CFCastStrict -> CFStaticCast * CFCast -> CFDynamicCast https://codereview.chromium.org/1927003004/diff/60001/base/mac/foundation_uti... File base/mac/foundation_util.mm (right): https://codereview.chromium.org/1927003004/diff/60001/base/mac/foundation_uti... base/mac/foundation_util.mm:323: [ns_val isKindOfClass:[NSFont class]])); On 2016/04/29 02:50:57, Nico wrote: > Splitting these NSClassFromString("foo") -> [foo class] changes out makes sense > to me (you can tbr that against me, this looks good) Done.
mark: ping? WDYT..
https://codereview.chromium.org/1927003004/diff/80001/base/mac/foundation_util.h File base/mac/foundation_util.h (right): https://codereview.chromium.org/1927003004/diff/80001/base/mac/foundation_uti... base/mac/foundation_util.h:279: return (T)(cf_val); So now in release mode, the “strict” variants will NOT return nullptr/nil even if the type doesn’t match. That seems kind of bad, considering how these are often used to protect us against type-confusion bugs.
https://codereview.chromium.org/1927003004/diff/80001/base/mac/foundation_util.h File base/mac/foundation_util.h (right): https://codereview.chromium.org/1927003004/diff/80001/base/mac/foundation_uti... base/mac/foundation_util.h:279: return (T)(cf_val); On 2016/05/05 13:26:31, Mark Mentovai wrote: > So now in release mode, the “strict” variants will NOT return nullptr/nil even > if the type doesn’t match. That seems kind of bad, considering how these are > often used to protect us against type-confusion bugs. currently they may protect against a _crash_ from a type confusion bug, but not necessarily against other kinds of bugs. I guess this is more true for the ObjC variant than the CF one, but there returning null/nil isn't really any good if the caller doesn't check for null. In Release. I would rather have an actual crash like `uncaught unrecognized selector sent to instance` than have calls on the `nil` absorbed by the runtime. Also since these already DCHECK, they kinda go against the spirit of the regime imposed at https://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC... i.e. "you should not handle DCHECK() failures, even if failure would result in a crash" But if changing existing callers is too scary, how would you feel about adding another variant of these called "CFStaticCast" and "ObjCStaticCast", with this implementation? Then things with good test coverage can opt for the "Static" versions and not suffer the runtime penalty of the type checking.
Description was changed from ========== Mac: Remove typechecking in Release for the FooCastStrict variants in foundation_util.h The FooCastStrict methods in foundation_util.h DCHECK on a failed cast. In Release the DCHECK is skipped, but the type is still checked (and nil is returned on failure). This adds a runtime cost and hides potential Release-only bugs that may be getting absorbed by the runtime. Instead, in Release, just do the cast and ensure it is inlineable. Context: this started with an investigation of a performance bottleneck in RenderTextMac which flagged calls to NSClassFromString(@"NSFont") as being surprisingly expensive. BUG=605131 ========== to ========== Mac: Introduce "StaticCast" variants of base::mac::FooCastStrict, which do not typecheck in Release The FooCastStrict methods in foundation_util.h DCHECK on a failed cast. In Release the DCHECK is skipped, but the type is still checked (and nil is returned on failure). This adds a runtime cost and hides potential Release-only bugs that may be getting absorbed by the runtime. Provide CFStaticCast and ObjCStaticCast which, in Release, just do the cast and are always inlineable. Context: this started with an investigation of a performance bottleneck in RenderTextMac which flagged calls to NSClassFromString(@"NSFont") as being surprisingly expensive. BUG=605131 ==========
On 2016/05/06 01:57:09, tapted wrote: > how would you feel about adding > another variant of these called "CFStaticCast" and "ObjCStaticCast", with this > implementation? Then things with good test coverage can opt for the "Static" > versions and not suffer the runtime penalty of the type checking. Added StaticCast variants in Patch Set 6 so you can try them on :) (I would still advocate phasing out the "strict" variant in favour of the "static" variant though).
https://codereview.chromium.org/1927003004/diff/80001/base/mac/foundation_util.h File base/mac/foundation_util.h (right): https://codereview.chromium.org/1927003004/diff/80001/base/mac/foundation_uti... base/mac/foundation_util.h:279: return (T)(cf_val); tapted wrote: > currently they may protect against a _crash_ from a type confusion bug, but not > necessarily against other kinds of bugs. The big problem with type confusion is that you can’t nail down the behavior at all when you treat a pointer to one type as a pointer to another. It’s totally dependent on the data. Sometimes it’s a crash, but sometimes it’s some other unexpected garbage. Sometimes it gets nasty, and in the worst cases, there are security implications. Returning nullptr or nil when the type doesn’t match reduces all of this to a non-issue. Even if the calling code doesn’t handle nullptr or nil, it’s not going to result in unexpected garbage. You might get a crash, but it’s a clean crash with no security implications and it’ll be obvious in the post-mortem that the pointer was 0. In most instances in Objective-C, it’ll just turn into a no-op. So no, you won’t see “unrecognized selector,” but you also weren’t guaranteed that anyway, because the selector you’re calling may have been implemented in both classes. And this is where type confusion gets really confusing. You enter an unexpected codepath that seems to do the right thing for a while, and then when things hit the fan, you’re in too deep to figure out where you actually went off the rails. But in reality, when you’re slurping stuff out of a dictionary (which is probably where this is used most commonly), you probably EITHER want to check that the key was actually in the dictionary (and you’d achieve that with a nil check) OR you’d just let messages to nil turn into no-ops (which the cast as-is gives you). > I guess this is more true for the ObjC > variant than the CF one, but there returning null/nil isn't really any good if > the caller doesn't check for null. In Release. I would rather have an actual > crash like `uncaught unrecognized selector sent to instance` than have calls on > the `nil` absorbed by the runtime. > > Also since these already DCHECK, they kinda go against the spirit of the regime > imposed at > https://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC... > i.e. "you should not handle DCHECK() failures, even if failure would result in > a crash" > > But if changing existing callers is too scary, how would you feel about adding > another variant of these called "CFStaticCast" and "ObjCStaticCast", with this > implementation? Then things with good test coverage can opt for the "Static" > versions and not suffer the runtime penalty of the type checking. Where are the hot paths that suffer from a performance problem? Most things that come through here are not hot code, or they probably wouldn’t be using CF or Objective-C to begin with.
Sorry for the radio silence - I had a WIP response ages ago but it got eaten by a chrome restart during bug triage to repro a bug :p The gist was: I'd really like to get rid of all the current places that do static_cast<C[FTG]FooRef> around Chrome's codebase (also the places that use C-style casts!), and give them a drop-in replacement that will typecheck in debug but be unchanged in Release. I did a bunch of git-greps and there are about equal numbers using base::mac::FooCast as there are using direct casts. Another angle: for security-sensitive code, we usually say code should CHECK rather than DCHECK. Part of my rationale on the ObjC side was that we would get some crashes via unrecognized selectors that might be currently masking subtle bugs getting no-op'd by nil. If we keep the cost of typechecking anyway, would it be better to crash with a CHECK? -- maybe via a method call so we keep some inlining benefit. But I also wanted to get some numbers to quantify the actual performance overheads of the typechecking. (I started down this path because the checking for some types was using NSClassFromString, and that got flagged in a performance sample, but it's been resolved already). I kept telling myself I'd find some spare time to gather some nice numbers on this and report back. But that hasn't happened, so I wanted to give an update. I'll shelve this for now to take it out of codereview queues. But let me know if you think changing the DCHECK to a CHECK is worth pursuing.
Message was sent while issue was closed.
I was a little surprised that the “strict” cast used DCHECK instead of CHECK. That’s not how I remembered it, but it’s how it’s always been. I’d support giving the “strict” variant fatal-in-release semantics. |
