Add a WebKitMessageRecorder DTrace provider, exposing IPC details to DTrace
Created attachment 244984 [details] preliminary
Need to sort out the ProcessType layering violation before this can go up for review.
Sam thinks that we should use bundleID instead of process type enums, and keep around the remote bundleID on the Connection when it's launched, and have a mapping in the script that processes the output. Dan thinks that the DTrace script that traces the messages should be included in this patch.
(In reply to comment #3) > Sam thinks that we should use bundleID instead of process type enums, and > keep around the remote bundleID on the Connection when it's launched, and > have a mapping in the script that processes the output. We're not going to do this, just move the enum to IPC::.
Created attachment 245074 [details] Patch
Comment on attachment 245074 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245074&action=review I would like to see the generated output as well before finishing the review. > Source/WebKit2/Platform/IPC/MessageRecorder.cpp:35 > +using namespace IPC; You should instead use the normal namespace IPC { }. > Source/WebKit2/Platform/IPC/MessageRecorder.cpp:49 > + record.sourceProcessType = (uint64_t)connection->client()->localProcessType(); > + record.destinationProcessType = (uint64_t)connection->client()->remoteProcessType(); static_cast. > Source/WebKit2/Platform/IPC/MessageRecorder.cpp:77 > + record.sourceProcessType = (uint64_t)connection->client()->remoteProcessType(); > + record.destinationProcessType = (uint64_t)connection->client()->localProcessType(); static_cast. > Source/WebKit2/Platform/IPC/MessageRecorder.h:45 > + MallocPtr<char> messageReceiverName; > + MallocPtr<char> messageName; Maybe this should use std::unique_ptr<char[]>, though I don't know if either have guarantees that that they have the same memory layout as a char*.
Created attachment 245474 [details] derived header
Created attachment 245528 [details] Patch
Comment on attachment 245528 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245528&action=review > Source/WebKit2/Platform/IPC/MessageRecorder.cpp:42 > +std::unique_ptr<MessageRecorder::MessageProcessingToken> MessageRecorder::recordOutgoingMessage(Connection* connection, MessageEncoder* encoder) Can this take a Connection& and a MessageDecoder& ? > Source/WebKit2/Platform/IPC/MessageRecorder.cpp:70 > +void MessageRecorder::recordIncomingMessage(Connection* connection, MessageDecoder* decoder) Can this take a Connection& and a MessageDecoder& ?
Created attachment 245583 [details] fixing sam's comments
http://trac.webkit.org/changeset/179326
Comment on attachment 245583 [details] fixing sam's comments View in context: https://bugs.webkit.org/attachment.cgi?id=245583&action=review > Source/WebKit2/Platform/IPC/ArgumentCoders.h:32 > +#include <uuid/uuid.h> This header doesn’t seem to be present on EFL or GTL, so the code needs to be conditional to allow them to continue to build.
(In reply to comment #12) > Comment on attachment 245583 [details] > fixing sam's comments > > View in context: > https://bugs.webkit.org/attachment.cgi?id=245583&action=review > > > Source/WebKit2/Platform/IPC/ArgumentCoders.h:32 > > +#include <uuid/uuid.h> > > This header doesn’t seem to be present on EFL or GTL, so the code needs to > be conditional to allow them to continue to build. Added uuid-dev as package dependency list: http://trac.webkit.org/changeset/179337
Comment on attachment 245583 [details] fixing sam's comments View in context: https://bugs.webkit.org/attachment.cgi?id=245583&action=review > Source/WebKit2/Platform/IPC/MessageRecorder.cpp:32 > +#include "MessageRecorderProbes.h" Where is it come from? I can't see this file in the tree checked in and can't find any generator responsible for generating it.
Comment on attachment 245583 [details] fixing sam's comments View in context: https://bugs.webkit.org/attachment.cgi?id=245583&action=review >> Source/WebKit2/Platform/IPC/MessageRecorder.cpp:32 >> +#include "MessageRecorderProbes.h" > > Where is it come from? I can't see this file in the tree checked > in and can't find any generator responsible for generating it. I believe it is generated from the "MessageRecorder.d" file by Xcode.
You can see the dtrace invocation in the build log: /usr/sbin/dtrace -h -s Platform/IPC/MessageRecorderProbes.d -o ${DERIVED_SOURCES_DIR}/MessageRecorderProbes.h
It seems there is no dtrace on Linux, I'll try add ifdef guards to disable this new feature on non Cocoa platforms, I filed a new bug report: bug141027
(In reply to comment #17) > It seems there is no dtrace on Linux, I'll try add ifdef guards to disable > this new feature on non Cocoa platforms, I filed a new bug report: bug141027 For the record: To my knowledge, there are at least two or three ports of DTrace for Linux. One of them done by Oracle itself (the owners of the copyright for DTrace) Is not shipped within the Linux kernel (and never will be) because of incompatibility between the GPL (Linux) and the CDDL (DTrace) Licenses. However, is possible to ship it as a DKMS kernel module that is built on the user machine at install time. That avoids the legal issues. Also it seems that Oracle ships it on their distribution. Some links: https://events.linuxfoundation.org/images/stories/pdf/lfcs2012_zannoni_hees.pdf https://blogs.oracle.com/linux/entry/announcement_dtrace_for_oracle_linux https://github.com/dtrace4linux/linux http://www.crisp.demon.co.uk/tools.html
(In reply to comment #17) > It seems there is no dtrace on Linux, I'll try add ifdef guards to disable > this new feature on non Cocoa platforms, I filed a new bug report: bug141027 I would use ENABLE(DTRACE) or something like that, instead of PLATFORM(COCOA), since we might support DTrace eventually. In any case, I think we want to support it optionally, so we need the #ifdefs guards even if we support DTrace
(In reply to comment #19) > (In reply to comment #17) > > It seems there is no dtrace on Linux, I'll try add ifdef guards to disable > > this new feature on non Cocoa platforms, I filed a new bug report: bug141027 > > I would use ENABLE(DTRACE) or something like that, instead of > PLATFORM(COCOA), since we might support DTrace eventually. In any case, I > think we want to support it optionally, so we need the #ifdefs guards even > if we support DTrace Also, other platforms where WebKitGTK+ runs, like FreeBSD, include support for DTrace out of the box... so I think that ENABLE(DTRACE) is ok. And maybe default ENABLE(DTRACE) to true on the platforms that this support comes out of the box: To my knowledge -> MacOS, FreeBSD, NetBSD and Solaris.
(In reply to comment #20) > (In reply to comment #19) > > (In reply to comment #17) > > > It seems there is no dtrace on Linux, I'll try add ifdef guards to disable > > > this new feature on non Cocoa platforms, I filed a new bug report: bug141027 > > > > I would use ENABLE(DTRACE) or something like that, instead of > > PLATFORM(COCOA), since we might support DTrace eventually. In any case, I > > think we want to support it optionally, so we need the #ifdefs guards even > > if we support DTrace > > Also, other platforms where WebKitGTK+ runs, like FreeBSD, include support > for DTrace out of the box... so I think that ENABLE(DTRACE) is ok. > > And maybe default ENABLE(DTRACE) to true on the platforms that this support > comes out of the box: To my knowledge -> MacOS, FreeBSD, NetBSD and Solaris. I wonder if it would be desirable to disable the message recorder in production builds even for platforms supporting DTrace. ENABLE(DTRACE) would work for that case as well.
(In reply to comment #21) > (In reply to comment #20) > > I wonder if it would be desirable to disable the message recorder in > production builds even for platforms supporting DTrace. ENABLE(DTRACE) would > work for that case as well. The plan is that it should be sufficiently low overhead that you don't have to do this, so you can trace production builds. But, sure, I think ENABLE(DTRACE) is probably a good plan all around (for https://bugs.webkit.org/show_bug.cgi?id=141027, not here). > This header doesn’t seem to be present on EFL or GTL, so the code needs to be conditional to allow them to continue to build. I checked to make sure that one was available for the other ports (though didn't know how to fix the build), but I totally forgot about DTrace :|
(In reply to comment #22) > (In reply to comment #21) > The plan is that it should be sufficiently low overhead that you don't have > to do this, so you can trace production builds. > > But, sure, I think ENABLE(DTRACE) is probably a good plan all around (for > https://bugs.webkit.org/show_bug.cgi?id=141027, not here). I think ENABLE(DTRACE) is needed on FreeBSD because dtrace -h needs dtraceall.ko to be loaded to work. Users may want to disable dtrace when they don't have root privileges to load kernel modules. > > > This header doesn’t seem to be present on EFL or GTL, so the code needs to be conditional to allow them to continue to build. > > I checked to make sure that one was available for the other ports (though > didn't know how to fix the build), but I totally forgot about DTrace :|