Bug 179576 - Avoid composing the message + recipient name for crash logs until a failure actually occurs
Summary: Avoid composing the message + recipient name for crash logs until a failure a...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-11-10 23:33 PST by Tim Horton
Modified: 2017-11-15 09:34 PST (History)
6 users (show)

See Also:


Attachments
Patch (1.98 KB, patch)
2017-11-10 23:34 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (3.98 KB, patch)
2017-11-11 00:09 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (3.96 KB, patch)
2017-11-11 00:10 PST, Tim Horton
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2017-11-10 23:33:51 PST
Use StringBuilder instead of NSString to build the full message name for crash logs
Comment 1 Tim Horton 2017-11-10 23:34:13 PST
Created attachment 326681 [details]
Patch
Comment 2 Tim Horton 2017-11-10 23:34:54 PST
Comment on attachment 326681 [details]
Patch

Not totally sure what the trailing colon is about, but I kept it.
Comment 3 mitz 2017-11-10 23:39:54 PST
Comment on attachment 326681 [details]
Patch

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

> Source/WebKit/Platform/IPC/mac/ConnectionMac.mm:291
> +    message->setMessageName(messageNameBuilder.toString().createCFString().get());

I wonder why MachMessage’s messageName needs to be a CFString.
Comment 4 Tim Horton 2017-11-10 23:45:42 PST
It looks like it doesn't, and seems likely that will make this even faster.
Comment 5 Tim Horton 2017-11-10 23:52:40 PST
That makes it a 90% reduction instead. But I think we can do yet better, and only compose the string in the failure case, instead of always.
Comment 6 Tim Horton 2017-11-11 00:09:28 PST
Created attachment 326682 [details]
Patch
Comment 7 Tim Horton 2017-11-11 00:10:46 PST
Created attachment 326683 [details]
Patch
Comment 8 mitz 2017-11-11 00:13:11 PST
Comment on attachment 326683 [details]
Patch

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

> Source/WebKit/Platform/IPC/mac/ConnectionMac.mm:250
> +        WebKit::setCrashReportApplicationSpecificInformation((CFStringRef)[NSString stringWithFormat:@"Unhandled error code %x, message '%s::%s'", kr, message->messageReceiverName().data(), message->messageName().data()]);

The :: makes more sense, but I wonder if the change in format would disturb some long-running data aggregation. Probably not!
Comment 9 Tim Horton 2017-11-11 01:04:01 PST
We'll see! I've only see this used for one-off investigations, though.
Comment 10 WebKit Commit Bot 2017-11-11 01:24:47 PST
Comment on attachment 326683 [details]
Patch

Clearing flags on attachment: 326683

Committed r224728: <https://trac.webkit.org/changeset/224728>
Comment 11 WebKit Commit Bot 2017-11-11 01:24:49 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Darin Adler 2017-11-12 17:06:30 PST
Comment on attachment 326683 [details]
Patch

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

> Source/WebKit/Platform/IPC/mac/MachMessage.h:47
> +    void setMessageReceiverName(const CString& messageReceiverName) { m_messageReceiverName = messageReceiverName; }

CString&& and WTFMove would give us slightly more efficient code here.

> Source/WebKit/Platform/IPC/mac/MachMessage.h:50
> +    void setMessageName(const CString& messageName) { m_messageName = messageName; }

Ditto.
Comment 13 Tim Horton 2017-11-13 14:49:39 PST
(In reply to Darin Adler from comment #12)
> Comment on attachment 326683 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326683&action=review
> 
> > Source/WebKit/Platform/IPC/mac/MachMessage.h:47
> > +    void setMessageReceiverName(const CString& messageReceiverName) { m_messageReceiverName = messageReceiverName; }
> 
> CString&& and WTFMove would give us slightly more efficient code here.
> 
> > Source/WebKit/Platform/IPC/mac/MachMessage.h:50
> > +    void setMessageName(const CString& messageName) { m_messageName = messageName; }
> 
> Ditto.

Did that in https://trac.webkit.org/changeset/224782/webkit
Comment 14 Radar WebKit Bug Importer 2017-11-15 09:34:42 PST
<rdar://problem/35561994>