Bug 209581 - MESSAGE_CHECK base macros should use UNLIKELY()
Summary: MESSAGE_CHECK base macros should use UNLIKELY()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-03-25 19:52 PDT by David Kilzer (:ddkilzer)
Modified: 2020-03-26 09:26 PDT (History)
4 users (show)

See Also:

Attachments
Patch v1 (2.30 KB, patch)
2020-03-25 20:01 PDT, David Kilzer (:ddkilzer) 		no flags Details | Formatted Diff | Diff
Patch for landing (2.56 KB, patch)
2020-03-26 08:46 PDT, David Kilzer (:ddkilzer) 		no flags Details | Formatted Diff | Diff
Patch for landing (2.49 KB, patch)
2020-03-26 09:02 PDT, David Kilzer (:ddkilzer) 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2020-03-25 19:52:38 PDT 
MESSAGE_CHECK base macros should use UNLIKELY().
Comment 1 Radar WebKit Bug Importer 2020-03-25 19:52:56 PDT 
<rdar://problem/60901307>
Comment 2 David Kilzer (:ddkilzer) 2020-03-25 20:01:12 PDT 
Created attachment 394572 [details]
Patch v1
Comment 3 youenn fablet 2020-03-26 05:26:08 PDT 
Comment on attachment 394572 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=394572&action=review

> Source/WebKit/Platform/IPC/Connection.h:83
>          ASSERT(assertion); \

Seems a bit odd to test assertion twice.
Comment 4 youenn fablet 2020-03-26 05:26:36 PDT 
Maybe we should put the ASSERT() before the if check.
Or add ASSERT_NOT_REACHED() instead.
Comment 5 David Kilzer (:ddkilzer) 2020-03-26 08:16:12 PDT 
(In reply to youenn fablet from comment #4)
> Maybe we should put the ASSERT() before the if check.
> Or add ASSERT_NOT_REACHED() instead.

I had the same concern when I first saw the macros (especially if the expression didn't evaluate the same way twice), but I ended up realizing the benefit was that the UI Process would crash on debug builds with the exact assertion text.

Maybe we could use ASSERT_NOT_REACHED_WITH_MESSAGE() and use the C-preprocessor trick of changing `assertion` into a literal text string?
Comment 6 David Kilzer (:ddkilzer) 2020-03-26 08:46:15 PDT 
Created attachment 394615 [details]
Patch for landing
Comment 7 David Kilzer (:ddkilzer) 2020-03-26 08:54:43 PDT 
(In reply to David Kilzer (:ddkilzer) from comment #5)
> (In reply to youenn fablet from comment #4)
> > Maybe we should put the ASSERT() before the if check.
> > Or add ASSERT_NOT_REACHED() instead.
> 
> I had the same concern when I first saw the macros (especially if the
> expression didn't evaluate the same way twice), but I ended up realizing the
> benefit was that the UI Process would crash on debug builds with the exact
> assertion text.
> 
> Maybe we could use ASSERT_NOT_REACHED_WITH_MESSAGE() and use the
> C-preprocessor trick of changing `assertion` into a literal text string?

Actually, putting the ASSERT() before the `if` check is more idiomatic.  I'll do that.
Comment 8 David Kilzer (:ddkilzer) 2020-03-26 09:02:26 PDT 
Created attachment 394617 [details]
Patch for landing
Comment 9 EWS 2020-03-26 09:26:11 PDT 
Committed r259047: <https://trac.webkit.org/changeset/259047>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394617 [details].