Bug 76885

Created attachment 123667 [details] Patch

Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.

Comment on attachment 123667 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123667&action=review > Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h:246 > + virtual const unsigned char* getCategoryEnabled(const char* categoryName) { return 0; } > + virtual int addTraceEvent( these are the public WK APIs that are being exported (and that anyone implementing support for the WebKit API will need to implement), so they need to be documented here.

Created attachment 123687 [details] Patch

Comment on attachment 123687 [details] Patch Is the use of the internal namespace buying us anything? I'd rather we just set up the names so that they're not going to obviously clash. We talked about this before though so not a blocker from my point of view. How are you going to switch over the old instrumentation points to this one? Does this even compile?

Comment on attachment 123687 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123687&action=review > Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h:250 > + virtual const unsigned char* getCategoryEnabled(const char* categoryName) { return 0; } sorry, i can't help but nit-pick the name of this function. it confuses me everytime I read it. perhaps better named: "const bool* getTraceCategoryEnabledFlag(const char* categoryName)" - what's the point of returning the address of an unsigned char over a bool? - it seems like it should have the word "Trace" in it somewhere for namespace reasons. - a "Flag" suffix might make the function name a little clearer--dunno. Everytime I read "getCategoryEnabled" I think it is going to tell me right now if the category is enabled, but in reality this is a query for the address of a flag that I can later dereference to see if the category is enabled. another idea, maybe too crazy, would be to implement a small class to wrap the flag: class WebTraceCategory { public: ... bool isEnabled() const { *m_flag != 0; } private: const unsigned char* m_flag; }; Then getCategoryEnabled could be morphed into "WebTraceCategory getTraceCategory(const char* categoryName)" > Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h:274 > + virtual int addTraceEvent( had to you considered creating a struct for all these params? WebTraceEvent? if you went with my suggestion of WebTraceCategory, then you could also just make this be a virtual method on WebTraceCategory since you already having to pass the "categoryEnabled" flag to this function. also, if you went with WebTraceCategory::addEvent, then you'd be able to document the addEvent function not in WebKitPlatformSupport.h. perhaps some downsides to introducing classes like these: - don't have it on the chromium side - don't have it on the webcore side - would be awkward to have it only in the webkit API and not elsewhere hmmm...

(In reply to comment #5) > (From update of attachment 123687 [details]) > Is the use of the internal namespace buying us anything? Yeah, I think so. It buys us less WebCore namespace clutter with things that should never be used outside of this header. This follows the original design in this header where the ScopedTracer class was in the internal namespace. >I'd rather we just set up the names so that they're not going to obviously clash. How about we mangle the names a bit more but still keep them in internal to avoid cluttering the WebCore namespace? > How are you going to switch over the old instrumentation points to this one? Does this even compile? Yes, it does compile and run. Notice the old TRACE_EVENT macro that has been changed to use the new style. However, a followup patch needs to clean up all instances of TRACE_EVENT so that we're not wasting arguments on 0 and 'this', etc. We could also optionally switch to variadic TRACE_EVENT at that point.

(In reply to comment #6) > (From update of attachment 123687 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=123687&action=review > > > Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h:250 > > + virtual const unsigned char* getCategoryEnabled(const char* categoryName) { return 0; } > > sorry, i can't help but nit-pick the name of this function. it confuses me everytime > I read it. Me too, it was left over API naming that I forgot to fix up -- I'll fix it now. > > perhaps better named: "const bool* getTraceCategoryEnabledFlag(const char* categoryName)" > - what's the point of returning the address of an unsigned char over a bool? Some discussion of this took place on the chromium-side CL, but essentially C-compatibility in the unlikely event that we want to trace from a C third-party library. (tl;dr There were also unsubstantiated claims that this pointer could point to garbage in some cases, which would mean unsigned char != 0 is more spec compliant than bool == true, but that's being really pedantic. Also it should never be garbage thanks to cache coherency as discussed in the chromium CL http://codereview.chromium.org/9155024/.) > - it seems like it should have the word "Trace" in it somewhere for namespace reasons. > - a "Flag" suffix might make the function name a little clearer--dunno. Sounds good, will go with getTraceCategoryEnabledFlag. > > Everytime I read "getCategoryEnabled" I think it is going to tell me right now if the category > is enabled, but in reality this is a query for the address of a flag that I can later dereference > to see if the category is enabled. > > another idea, maybe too crazy, would be to implement a small class to wrap the flag: > > class WebTraceCategory { > public: > ... > bool isEnabled() const { *m_flag != 0; } > private: > const unsigned char* m_flag; > }; > > Then getCategoryEnabled could be morphed into "WebTraceCategory getTraceCategory(const char* categoryName)" > > > Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h:274 > > + virtual int addTraceEvent( > > had to you considered creating a struct for all these params? One of the design constraints was to avoid structs altogether in this API so that we didn't end up with structs defined in multiple libraries that could potentially diverge. Since the third party libraries can't include headers from chromium/base and base can't include from a third party, this seemed to be the best solution. Another design constraint is performance, so if we went with a struct and then had to copy from each third party variant to the base struct it would be a bit slower. > > WebTraceEvent? > > if you went with my suggestion of WebTraceCategory, then you could also > just make this be a virtual method on WebTraceCategory since you already > having to pass the "categoryEnabled" flag to this function. > > also, if you went with WebTraceCategory::addEvent, then you'd be able to > document the addEvent function not in WebKitPlatformSupport.h. > > perhaps some downsides to introducing classes like these: > > - don't have it on the chromium side > - don't have it on the webcore side > - would be awkward to have it only in the webkit API and not elsewhere > > hmmm... Yeah, there were some hairy design constraints. It's also nice to avoid the virtual call to get the enabled state for optimal performance when tracing is disabled (where it currently only needs to fetch the unsigned char value).

Created attachment 123825 [details] Patch

(In reply to comment #9) > Created an attachment (id=123825) [details] > Patch I've updated the API wording and moved from the internal namespace to WebCore::TraceEvent to give the tracing symbols a home of their own. PTAL.

(In reply to comment #10) > (In reply to comment #9) > > Created an attachment (id=123825) [details] [details] > > Patch > > I've updated the API wording and moved from the internal namespace to WebCore::TraceEvent to give the tracing symbols a home of their own. PTAL. Note this won't compile until the chromium-side CL has landed: https://chromiumcodereview.appspot.com/9234007/

(In reply to comment #8) > > perhaps better named: "const bool* getTraceCategoryEnabledFlag(const char* categoryName)" > > - what's the point of returning the address of an unsigned char over a bool? > > Some discussion of this took place on the chromium-side CL, but essentially C-compatibility in the unlikely event that we want to trace from a C third-party library. (tl;dr There were also unsubstantiated claims that this pointer could point to garbage in some cases, which would mean unsigned char != 0 is more spec compliant than bool == true, but that's being really pedantic. Also it should never be garbage thanks to cache coherency as discussed in the chromium CL http://codereview.chromium.org/9155024/.) It doesn't seem like we should worry about C-compat, especially in the context of WebKit. My proposal really was to just wrap the char pointer with a class. The generated code would be no different than what you get by using the char pointer directly. I was only proposing some syntactic sugar to make the API cleaner :) > One of the design constraints was to avoid structs altogether in this API so that we didn't end up with structs defined in multiple libraries that could potentially diverge. Since the third party libraries can't include headers from chromium/base and base can't include from a third party, this seemed to be the best solution. Another design constraint is performance, so if we went with a struct and then had to copy from each third party variant to the base struct it would be a bit slower. Multiple definitions of the same struct is definitely annoying to maintain. I wouldn't be concerned about the cost of copying structs. Passing a struct by value is the same as passing a set of parameters each by value. You should get nearly the same code generated. > Yeah, there were some hairy design constraints. It's also nice to avoid the virtual call to get the enabled state for optimal performance when tracing is disabled (where it currently only needs to fetch the unsigned char value). I wasn't proposing an additional virtual calls. My proposal was to move the addTraceEvent virtual call from WebKitPlatformSupport to WebTraceCategory.

(In reply to comment #12) > (In reply to comment #8) > > > perhaps better named: "const bool* getTraceCategoryEnabledFlag(const char* categoryName)" > > > - what's the point of returning the address of an unsigned char over a bool? > > > > Some discussion of this took place on the chromium-side CL, but essentially C-compatibility in the unlikely event that we want to trace from a C third-party library. (tl;dr There were also unsubstantiated claims that this pointer could point to garbage in some cases, which would mean unsigned char != 0 is more spec compliant than bool == true, but that's being really pedantic. Also it should never be garbage thanks to cache coherency as discussed in the chromium CL http://codereview.chromium.org/9155024/.) > > It doesn't seem like we should worry about C-compat, especially in the context of WebKit. My proposal really was to just wrap the char pointer with a class. The generated code would be no different than what you get by using the char pointer directly. I was only proposing some syntactic sugar to make the API cleaner :) I see. I guess this API has become a bit monstrous in appearance because it's never called by user code. It's only called by the macros in TraceEvent.h. The main goal for the macros is to keep them somewhat in sync with chromium's versions in trace_event.h so that the behavior stays the same. > > > > One of the design constraints was to avoid structs altogether in this API so that we didn't end up with structs defined in multiple libraries that could potentially diverge. Since the third party libraries can't include headers from chromium/base and base can't include from a third party, this seemed to be the best solution. Another design constraint is performance, so if we went with a struct and then had to copy from each third party variant to the base struct it would be a bit slower. > > Multiple definitions of the same struct is definitely annoying to maintain. I wouldn't be concerned about the cost of copying structs. Passing a struct by value is the same as passing a set of parameters each by value. You should get nearly the same code generated. Good point. I was hoping the compiler could optimize those copies when they are passed through multiple leaf-function calls with the same parameters by sharing the stack frame. > > > > Yeah, there were some hairy design constraints. It's also nice to avoid the virtual call to get the enabled state for optimal performance when tracing is disabled (where it currently only needs to fetch the unsigned char value). > > I wasn't proposing an additional virtual calls. My proposal was to move the addTraceEvent virtual call from WebKitPlatformSupport to WebTraceCategory. I think I understand now. Is the goal to simplify the API of WebKitPlatformSupport or to provide syntactic sugar for the code that calls WebKitPlatformSupport? (The latter would be limited to the TraceEvent macros since they are the only callers.)

(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #8) > > > > perhaps better named: "const bool* getTraceCategoryEnabledFlag(const char* categoryName)" > > > > - what's the point of returning the address of an unsigned char over a bool? > > > > > > Some discussion of this took place on the chromium-side CL, but essentially C-compatibility in the unlikely event that we want to trace from a C third-party library. (tl;dr There were also unsubstantiated claims that this pointer could point to garbage in some cases, which would mean unsigned char != 0 is more spec compliant than bool == true, but that's being really pedantic. Also it should never be garbage thanks to cache coherency as discussed in the chromium CL http://codereview.chromium.org/9155024/.) > > > > It doesn't seem like we should worry about C-compat, especially in the context of WebKit. My proposal really was to just wrap the char pointer with a class. The generated code would be no different than what you get by using the char pointer directly. I was only proposing some syntactic sugar to make the API cleaner :) > > I see. I guess this API has become a bit monstrous in appearance because it's never called by user code. It's only called by the macros in TraceEvent.h. The main goal for the macros is to keep them somewhat in sync with chromium's versions in trace_event.h so that the behavior stays the same. Yeah... > > > One of the design constraints was to avoid structs altogether in this API so that we didn't end up with structs defined in multiple libraries that could potentially diverge. Since the third party libraries can't include headers from chromium/base and base can't include from a third party, this seemed to be the best solution. Another design constraint is performance, so if we went with a struct and then had to copy from each third party variant to the base struct it would be a bit slower. > > > > Multiple definitions of the same struct is definitely annoying to maintain. I wouldn't be concerned about the cost of copying structs. Passing a struct by value is the same as passing a set of parameters each by value. You should get nearly the same code generated. > > Good point. I was hoping the compiler could optimize those copies when they are passed through multiple leaf-function calls with the same parameters by sharing the stack frame. that optimization should apply regardless of how you structure the parameters passed on the stack, right? pass by struct or pass by N variables... same thing, no? > > > Yeah, there were some hairy design constraints. It's also nice to avoid the virtual call to get the enabled state for optimal performance when tracing is disabled (where it currently only needs to fetch the unsigned char value). > > > > I wasn't proposing an additional virtual calls. My proposal was to move the addTraceEvent virtual call from WebKitPlatformSupport to WebTraceCategory. > > I think I understand now. Is the goal to simplify the API of WebKitPlatformSupport or to provide syntactic sugar for the code that calls WebKitPlatformSupport? (The latter would be limited to the TraceEvent macros since they are the only callers. I am mostly motivated by wanting to compartmentalize, but also when I see a function with a zillion mysterious parameters, it makes me feel a bit sad :) That said, I don't think you need to necessarily take my suggestions. I just wanted to share in case it spurned some thoughts about how to improve things. It is fine to proceed as-is so that folks can start benefiting from tracing soon.

(In reply to comment #14) > > Good point. I was hoping the compiler could optimize those copies when they are passed through multiple leaf-function calls with the same parameters by sharing the stack frame. > > that optimization should apply regardless of how you structure the parameters passed on the stack, right? pass by struct or pass by N variables... same thing, no? I get it now. The structs at each level would be different types, and while we might attempt to keep them in-sync, if they ever drifted, then it would neutralize any chance of such a "pass through" optimization. Got it. OK, so you've convinced that we should have a function with a zillion parameters :)

Comment on attachment 123825 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123825&action=review > Source/WebCore/platform/chromium/PlatformSupport.h:424 > + const unsigned char* category_enabled, nit: category_enabled_flag > Source/WebCore/platform/chromium/TraceEvent.h:570 > +const int kZeroNumArgs = 0; nit: webkit style for constants is still to use variable naming style: zeroNumArgs, etc. > Source/WebCore/platform/chromium/TraceEvent.h:571 > +const int kNoThreshholdBeginId = -1; nit: "hh" -> "h" ?? > Source/WebCore/platform/chromium/TraceEvent.h:791 > + void Initialize(const unsigned char* categoryEnabled, nit: webkit style has function names starting with a lowercase letter.

Created attachment 124007 [details] Patch

Comment on attachment 123825 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123825&action=review One more thing to do -- remove the old unused tracing API (traceEventBegin, etc). >> Source/WebCore/platform/chromium/PlatformSupport.h:424 >> + const unsigned char* category_enabled, > > nit: category_enabled_flag Done. >> Source/WebCore/platform/chromium/TraceEvent.h:570 >> +const int kZeroNumArgs = 0; > > nit: webkit style for constants is still to use variable naming style: zeroNumArgs, etc. Done. >> Source/WebCore/platform/chromium/TraceEvent.h:571 >> +const int kNoThreshholdBeginId = -1; > > nit: "hh" -> "h" ?? Done. >> Source/WebCore/platform/chromium/TraceEvent.h:791 >> + void Initialize(const unsigned char* categoryEnabled, > > nit: webkit style has function names starting with a lowercase letter. Done.

Created attachment 124021 [details] Patch

Comment on attachment 124021 [details] Patch Attachment 124021 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11191070 New failing tests: animations/animation-controller-drt-api.html animations/3d/transform-origin-vs-functions.html accessibility/aria-checkbox-checked.html http/tests/appcache/access-via-redirect.php http/tests/appcache/auth.html animations/animation-css-rule-types.html animations/3d/matrix-transform-type-animation.html animations/3d/state-at-end-event-transform.html http/tests/appcache/credential-url.html http/tests/appcache/crash-when-navigating-away-then-back.html accessibility/anonymous-render-block-in-continuation-causes-crash.html accessibility/adjacent-continuations-cause-assertion-failure.html animations/additive-transform-animations.html animations/3d/change-transform-in-end-event.html animations/3d/replace-filling-transform.html http/tests/appcache/cyrillic-uri.html accessibility/anchor-linked-anonymous-block-crash.html animations/animation-direction-normal.html accessibility/aria-checkbox-sends-notification.html animations/animation-add-events-in-handler.html

Created attachment 124025 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

(In reply to comment #20) > (From update of attachment 124021 [details]) > Attachment 124021 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/11191070 > > New failing tests: > animations/animation-controller-drt-api.html > animations/3d/transform-origin-vs-functions.html > accessibility/aria-checkbox-checked.html > http/tests/appcache/access-via-redirect.php > http/tests/appcache/auth.html > animations/animation-css-rule-types.html > animations/3d/matrix-transform-type-animation.html > animations/3d/state-at-end-event-transform.html > http/tests/appcache/credential-url.html > http/tests/appcache/crash-when-navigating-away-then-back.html > accessibility/anonymous-render-block-in-continuation-causes-crash.html > accessibility/adjacent-continuations-cause-assertion-failure.html > animations/additive-transform-animations.html > animations/3d/change-transform-in-end-event.html > animations/3d/replace-filling-transform.html > http/tests/appcache/cyrillic-uri.html > accessibility/anchor-linked-anonymous-block-crash.html > animations/animation-direction-normal.html > accessibility/aria-checkbox-sends-notification.html > animations/animation-add-events-in-handler.html Looks like every test that hits a TRACE_EVENT crashes

(In reply to comment #22) > (In reply to comment #20) > > (From update of attachment 124021 [details] [details]) > > Attachment 124021 [details] [details] did not pass chromium-ews (chromium-xvfb): > > Output: http://queues.webkit.org/results/11191070 > > > > New failing tests: > > animations/animation-controller-drt-api.html > > animations/3d/transform-origin-vs-functions.html > > accessibility/aria-checkbox-checked.html > > http/tests/appcache/access-via-redirect.php > > http/tests/appcache/auth.html > > animations/animation-css-rule-types.html > > animations/3d/matrix-transform-type-animation.html > > animations/3d/state-at-end-event-transform.html > > http/tests/appcache/credential-url.html > > http/tests/appcache/crash-when-navigating-away-then-back.html > > accessibility/anonymous-render-block-in-continuation-causes-crash.html > > accessibility/adjacent-continuations-cause-assertion-failure.html > > animations/additive-transform-animations.html > > animations/3d/change-transform-in-end-event.html > > animations/3d/replace-filling-transform.html > > http/tests/appcache/cyrillic-uri.html > > accessibility/anchor-linked-anonymous-block-crash.html > > animations/animation-direction-normal.html > > accessibility/aria-checkbox-sends-notification.html > > animations/animation-add-events-in-handler.html > > Looks like every test that hits a TRACE_EVENT crashes The chromium rev needs to be rolled past 118999 before this will run.

Created attachment 124170 [details] Patch

Created attachment 124221 [details] Patch

Comment on attachment 124221 [details] Patch I removed the manglePointer API, because it's simpler and cleaner to do that through the flags parameter of addTraceEvent. Everything else is the same -- should be ready to commit.

Created attachment 124224 [details] Patch

Comment on attachment 124224 [details] Patch Voiding unused parameters to be pedantic. Sorry for the patch spam!

jamesr, darin -- final R / CQ?

Comment on attachment 124224 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124224&action=review API is up to Darin. Left some comments. > Source/WebCore/bindings/v8/V8Proxy.cpp:342 > + TRACE_EVENT_BEGIN1("v8", "v8.compile", "url", source.url().string().ascii()); will the argument only be evaluated when tracing is active? source.url().string().ascii() is going to do at least one copy of the string data. even if it's only active when tracing is enabled this seems pretty heavyweight to do on every evaluate > Source/WebCore/page/Console.cpp:302 > + TRACE_EVENT_COPY_BEGIN0("webkit", title.utf8().data()); title.utf8() is going to do a conversion and string copy. i guess it was before, though

Comment on attachment 124224 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124224&action=review >> Source/WebCore/bindings/v8/V8Proxy.cpp:342 >> + TRACE_EVENT_BEGIN1("v8", "v8.compile", "url", source.url().string().ascii()); > > will the argument only be evaluated when tracing is active? source.url().string().ascii() is going to do at least one copy of the string data. even if it's only active when tracing is enabled this seems pretty heavyweight to do on every evaluate It's only evaluated when tracing is active, but even so I'll remove it since this is a hot code path. >> Source/WebCore/page/Console.cpp:302 >> + TRACE_EVENT_COPY_BEGIN0("webkit", title.utf8().data()); > > title.utf8() is going to do a conversion and string copy. i guess it was before, though Yeah, same behavior as before where this arg is only evaluated when tracing is enabled.

Created attachment 124353 [details] Patch

Created attachment 124599 [details] Patch

(In reply to comment #33) > Created an attachment (id=124599) [details] > Patch The latest patch adds more detail to the documentation of the new tracing API in WebKitPlatformSupport.h so that embedders know exactly what to expect.

Comment on attachment 124599 [details] Patch Clearing flags on attachment: 124599 Committed r106382: <http://trac.webkit.org/changeset/106382>

All reviewed patches have been landed. Closing bug.

Reopen after revert to fix crash in chromium unit_tests.

Created attachment 125429 [details] Patch

Comment on attachment 125429 [details] Patch I hadn't realized there were chromium tests that ran with the default implementation of WebKitPlatformSupport. The latest patch makes the tracing macros run in a disabled state when the default API in WebKitPlatformSupport is used.

Comment on attachment 124599 [details] Patch recommitting now that chromium tests have been fixed.

Comment on attachment 124599 [details] Patch Rejecting attachment 124599 [details] from review queue. jbates@google.com does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your reviewer rights.

Comment on attachment 124599 [details] Patch Rejecting attachment 124599 [details] from commit-queue. jbates@google.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.

Created attachment 126338 [details] Patch

Comment on attachment 126338 [details] Patch Clearing flags on attachment: 126338 Committed r107291: <http://trac.webkit.org/changeset/107291>

All reviewed patches have been landed. Closing bug.