70620 2011-10-21 10:48:09 -0700 [chromium] Route Console::time and Console::timeEnd to trace_event 2011-10-25 02:53:43 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 70618 1 nduca nduca dglazkov jamesr pfeldman webkit.review.bot yurys oldest_to_newest 488482 0 nduca 2011-10-21 10:48:09 -0700 [chromium] Route Console::time and Console::timeEnd to trace_event 488485 1 111986 nduca 2011-10-21 10:49:07 -0700 Created attachment 111986 Patch 488486 2 nduca 2011-10-21 10:49:56 -0700 James -- do I need other reviewers? 488488 3 111986 webkit.review.bot 2011-10-21 10:52:23 -0700 Comment on attachment 111986 Patch Attachment 111986 did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10179962 488554 4 jamesr 2011-10-21 12:19:43 -0700 Pavel or Yury should take a look. Could you explain how the tracing works (or point them at the relevant docs), Nat? 488575 5 111986 pfeldman 2011-10-21 12:51:50 -0700 Comment on attachment 111986 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111986&action=review What is the story behind the change? We tend to do it the other way (push information into the Web Inspector front-end through the WebCore). r- for not compiling in any case. > Source/WebCore/page/Console.cpp:311 > +#if PLATFORM(CHROMIUM) I really don't like these. But I don't see a better way of handling it here. > Source/WebCore/page/Console.cpp:312 > + if (PlatformSupport::isTraceEventEnabledForCategory("webcore.console")) I don't see this method on ToT. See compile failures on ews. 488580 6 nduca 2011-10-21 12:58:00 -0700 Yeah, I agree with the #ifs. If I find time, I'd like to get some platform abstraction of tracing concept landed. In the near term, would this be OK? Its okay if no, but it does help us chase interactions between JS and chrome innards. The isTraceEventEnabled is b70618, marked as a dep on this bug. That needs some tweaks to land too. :) 488612 7 nduca 2011-10-21 13:31:55 -0700 (In reply to comment #5) > What is the story behind the change? We tend to do it the other way (push information into the Web Inspector front-end through the WebCore). r- for not compiling in any case. Oh, sorry I missed this comment! This isn't intended for use by web developers. I don't think web devlopers reading about:tracing to understand performance is the *right* thing for our platform. trace_event ties to chrome's about:tracing, which is an implementer-facing tool. For example, us folks working on GPU acceleration now ask developers to submit about:trace dumps with their graphics-related bug reports. Whereas inspector information is tailored for human consumption, the about:trace contains extremely detailed implementation level information about what Chrome was doing to lead to bad perf. This has been really useful for us in speeding chrome perf up and we're constantly adding instrumentation to shipping Chrome builds to make these types of perf dumps more informative. One problem we have with these traces is that we'll see some heavy MessageLoop task in Chrome [e.g. some sync message call] but not have a clue what the JS part of the app was trying to do. Was it their renderer code? Or was it some other piece of third_party advertising code the page just happens to be using? This patch allows us or our customers to sprinkle time/timeEnd calls throughout JS. The dumps they send us will help us see the relationship between "their code" and our code. The reason I hooked console.time and timeEnd rather than adding a platform-specific or test-specific macro is because our traces often come from people outside running Canary. Were this to require a custom build of Chrome, it'd limit the utility of the hooking. Hope that makes sense? I'm totally fine if you think this is a bad thing to land... it was a simple change, so I figured I'd try. :) 489520 8 pfeldman 2011-10-24 12:14:15 -0700 > Hope that makes sense? I'm totally fine if you think this is a bad thing to land... it was a simple change, so I figured I'd try. :) I think it is Ok to land it as is once you get the API it uses in place. In return, it would be great if you could experiment with the data and push developer-centric scenarios back into the inspector. I am sure there is enough we could expose to the end-user without frightening him. 489532 9 nduca 2011-10-24 12:31:10 -0700 (In reply to comment #8) > In return, it would be great if you could experiment with the data and push developer-centric scenarios back into the inspector. I am sure there is enough we could expose to the end-user without frightening him. Definitely! That's why I was poking you the other day about how to an experimental rendering tab for inspector. Its not clear to me what we should tell the end-dev yet, but we definitely need to make something human-consumable available. :) 489689 10 112259 nduca 2011-10-24 15:12:35 -0700 Created attachment 112259 Patch 489696 11 112259 webkit.review.bot 2011-10-24 15:21:04 -0700 Comment on attachment 112259 Patch Attachment 112259 did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10200988 489718 12 112266 nduca 2011-10-24 15:49:12 -0700 Created attachment 112266 Should build 489919 13 nduca 2011-10-25 01:45:59 -0700 Ok, it builds. Pavel, can you do the final review? 489942 14 nduca 2011-10-25 02:53:43 -0700 Committed r98331: <http://trac.webkit.org/changeset/98331> 111986 2011-10-21 10:49:07 -0700 2011-10-24 15:12:32 -0700 Patch bug-70620-20111021104906.patch text/plain 1937 nduca U3VidmVyc2lvbiBSZXZpc2lvbjogOTgwNzcKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCAyZWEyZmM5YTJjMDMzODQx MDRkMDhlZWY5MTQzNDU4NWUzZTNiNDFhLi41NmZkMzc2ZGE5YjE1YmU0MjUyODZmMWM5MGNhNzIw NDlmNzdmMTZiIDEwMDc1NQotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTQgQEAKKzIwMTEtMTAtMjEgIE5hdCBE dWNhICA8bmR1Y2FAY2hyb21pdW0ub3JnPgorCisgICAgICAgIFtjaHJvbWl1bV0gUm91dGUgQ29u c29sZTo6dGltZSBhbmQgQ29uc29sZTo6dGltZUVuZCB0byB0cmFjZV9ldmVudAorICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NzA2MjAKKworICAgICAgICBS ZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIHBhZ2UvQ29uc29sZS5jcHA6 CisgICAgICAgIChXZWJDb3JlOjpDb25zb2xlOjp0aW1lKToKKyAgICAgICAgKFdlYkNvcmU6OkNv bnNvbGU6OnRpbWVFbmQpOgorCiAyMDExLTEwLTIwICBQZXRlciBSeWJpbiAgPHBldGVyLnJ5Ymlu QGdtYWlsLmNvbT4KIAogICAgICAgICBXZWIgSW5zcGVjdG9yOiByZWltcGxlbWVudCBwcm90b2Nv bCBiYWNrZW5kL2Zyb250ZW5kIHNvdXJjZSBnZW5lcmF0b3IKZGlmZiAtLWdpdCBhL1NvdXJjZS9X ZWJDb3JlL3BhZ2UvQ29uc29sZS5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9wYWdlL0NvbnNvbGUuY3Bw CmluZGV4IDc0ZGZlNzRhYjI0Mjk0YmUzYjVlN2RjYjdlZmE4ZmNiZTM0MjkxYmIuLjM4YWY2OTE3 YTZkOWU1ZTc3MmM4M2E5ZTEzYjNkY2EyMWE0ODhjY2QgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJD b3JlL3BhZ2UvQ29uc29sZS5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvcGFnZS9Db25zb2xlLmNw cApAQCAtNDksNiArNDksMTAgQEAKICNpbmNsdWRlIDx3dGYvVW51c2VkUGFyYW0uaD4KICNpbmNs dWRlIDx3dGYvdGV4dC9DU3RyaW5nLmg+CiAKKyNpZiBQTEFURk9STShDSFJPTUlVTSkKKyNpbmNs dWRlICJUcmFjZUV2ZW50LmgiCisjZW5kaWYKKwogbmFtZXNwYWNlIFdlYkNvcmUgewogCiBDb25z b2xlOjpDb25zb2xlKEZyYW1lKiBmcmFtZSkKQEAgLTMwNCwxMCArMzA4LDE4IEBAIHZvaWQgQ29u c29sZTo6cHJvZmlsZUVuZChjb25zdCBTdHJpbmcmIHRpdGxlLCBTY3JpcHRTdGF0ZSogc3RhdGUs IFBhc3NSZWZQdHI8U2NyCiB2b2lkIENvbnNvbGU6OnRpbWUoY29uc3QgU3RyaW5nJiB0aXRsZSkK IHsKICAgICBJbnNwZWN0b3JJbnN0cnVtZW50YXRpb246OnN0YXJ0Q29uc29sZVRpbWluZyhwYWdl KCksIHRpdGxlKTsKKyNpZiBQTEFURk9STShDSFJPTUlVTSkKKyAgICBpZiAoUGxhdGZvcm1TdXBw b3J0Ojppc1RyYWNlRXZlbnRFbmFibGVkRm9yQ2F0ZWdvcnkoIndlYmNvcmUuY29uc29sZSIpKQor ICAgICAgICBQbGF0Zm9ybVN1cHBvcnQ6OnRyYWNlRXZlbnRCZWdpbih0aXRsZS51dGY4KCkuZGF0 YSgpLCAwLCAwKTsKKyNlbmRpZgogfQogCiB2b2lkIENvbnNvbGU6OnRpbWVFbmQoY29uc3QgU3Ry aW5nJiB0aXRsZSwgUGFzc1JlZlB0cjxTY3JpcHRBcmd1bWVudHM+LCBQYXNzUmVmUHRyPFNjcmlw dENhbGxTdGFjaz4gY2FsbFN0YWNrKQogeworI2lmIFBMQVRGT1JNKENIUk9NSVVNKQorICAgIGlm IChQbGF0Zm9ybVN1cHBvcnQ6OmlzVHJhY2VFdmVudEVuYWJsZWRGb3JDYXRlZ29yeSgid2ViY29y ZS5jb25zb2xlIikpCisgICAgICAgIFBsYXRmb3JtU3VwcG9ydDo6dHJhY2VFdmVudEVuZCh0aXRs ZS51dGY4KCkuZGF0YSgpLCAwLCAwKTsKKyNlbmRpZgogICAgIEluc3BlY3Rvckluc3RydW1lbnRh dGlvbjo6c3RvcENvbnNvbGVUaW1pbmcocGFnZSgpLCB0aXRsZSwgY2FsbFN0YWNrKTsKIH0KIAo= 112259 2011-10-24 15:12:35 -0700 2011-10-24 15:49:09 -0700 Patch bug-70620-20111024151234.patch text/plain 1928 nduca U3VidmVyc2lvbiBSZXZpc2lvbjogOTgyODUKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCAyMzZjZmY0YTMxZDg3NDdj NDAzN2NlZTc5ZTY1MzM4YjAyZjE0MDViLi5jMDg3NDhhZjVkNjFjOTMzYmMxZTZkNmRiNzg4ODIx NmQ2ZGJlOTAwIDEwMDc1NQotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDUgKzEsMTYgQEAKIDIwMTEtMTAtMjEgIE5hdCBE dWNhICA8bmR1Y2FAY2hyb21pdW0ub3JnPgogCisgICAgICAgIFtjaHJvbWl1bV0gUm91dGUgQ29u c29sZTo6dGltZSBhbmQgQ29uc29sZTo6dGltZUVuZCB0byB0cmFjZV9ldmVudAorICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NzA2MjAKKworICAgICAgICBS ZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIHBhZ2UvQ29uc29sZS5jcHA6 CisgICAgICAgIChXZWJDb3JlOjpDb25zb2xlOjp0aW1lKToKKyAgICAgICAgKFdlYkNvcmU6OkNv bnNvbGU6OnRpbWVFbmQpOgorCisyMDExLTEwLTIxICBOYXQgRHVjYSAgPG5kdWNhQGNocm9taXVt Lm9yZz4KKwogICAgICAgICBbY2hyb21pdW1dIEFkZCBpc1RyYWNlRXZlbnRFbmFibGVkIHRvIFBs YXRmb3JtU3VwcG9ydC4KICAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcu Y2dpP2lkPTcwNjE4CiAKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BhZ2UvQ29uc29sZS5j cHAgYi9Tb3VyY2UvV2ViQ29yZS9wYWdlL0NvbnNvbGUuY3BwCmluZGV4IDc0ZGZlNzRhYjI0Mjk0 YmUzYjVlN2RjYjdlZmE4ZmNiZTM0MjkxYmIuLjdjZjRlZDg2NTlkMDkwY2RkMzg2NzFkZWMwNGYy MTFmOTFlOTRiYTQgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3BhZ2UvQ29uc29sZS5jcHAK KysrIGIvU291cmNlL1dlYkNvcmUvcGFnZS9Db25zb2xlLmNwcApAQCAtNDksNiArNDksMTAgQEAK ICNpbmNsdWRlIDx3dGYvVW51c2VkUGFyYW0uaD4KICNpbmNsdWRlIDx3dGYvdGV4dC9DU3RyaW5n Lmg+CiAKKyNpZiBQTEFURk9STShDSFJPTUlVTSkKKyNpbmNsdWRlICJUcmFjZUV2ZW50LmgiCisj ZW5kaWYKKwogbmFtZXNwYWNlIFdlYkNvcmUgewogCiBDb25zb2xlOjpDb25zb2xlKEZyYW1lKiBm cmFtZSkKQEAgLTMwNCwxMCArMzA4LDE4IEBAIHZvaWQgQ29uc29sZTo6cHJvZmlsZUVuZChjb25z dCBTdHJpbmcmIHRpdGxlLCBTY3JpcHRTdGF0ZSogc3RhdGUsIFBhc3NSZWZQdHI8U2NyCiB2b2lk IENvbnNvbGU6OnRpbWUoY29uc3QgU3RyaW5nJiB0aXRsZSkKIHsKICAgICBJbnNwZWN0b3JJbnN0 cnVtZW50YXRpb246OnN0YXJ0Q29uc29sZVRpbWluZyhwYWdlKCksIHRpdGxlKTsKKyNpZiBQTEFU Rk9STShDSFJPTUlVTSkKKyAgICBpZiAoUGxhdGZvcm1TdXBwb3J0Ojppc1RyYWNlRXZlbnRFbmFi bGVkKCkpCisgICAgICAgIFBsYXRmb3JtU3VwcG9ydDo6dHJhY2VFdmVudEJlZ2luKHRpdGxlLnV0 ZjgoKS5kYXRhKCksIDAsIDApOworI2VuZGlmCiB9CiAKIHZvaWQgQ29uc29sZTo6dGltZUVuZChj b25zdCBTdHJpbmcmIHRpdGxlLCBQYXNzUmVmUHRyPFNjcmlwdEFyZ3VtZW50cz4sIFBhc3NSZWZQ dHI8U2NyaXB0Q2FsbFN0YWNrPiBjYWxsU3RhY2spCiB7CisjaWYgUExBVEZPUk0oQ0hST01JVU0p CisgICAgaWYgKFBsYXRmb3JtU3VwcG9ydDo6aXNUcmFjZUV2ZW50RW5hYmxlZEZvckNhdGVnb3J5 KCkpCisgICAgICAgIFBsYXRmb3JtU3VwcG9ydDo6dHJhY2VFdmVudEVuZCh0aXRsZS51dGY4KCku ZGF0YSgpLCAwLCAwKTsKKyNlbmRpZgogICAgIEluc3BlY3Rvckluc3RydW1lbnRhdGlvbjo6c3Rv cENvbnNvbGVUaW1pbmcocGFnZSgpLCB0aXRsZSwgY2FsbFN0YWNrKTsKIH0KIAo= 112266 2011-10-24 15:49:12 -0700 2011-10-25 02:20:37 -0700 Should build bug-70620-20111024154911.patch text/plain 1917 nduca U3VidmVyc2lvbiBSZXZpc2lvbjogOTgyODUKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCAyMzZjZmY0YTMxZDg3NDdj NDAzN2NlZTc5ZTY1MzM4YjAyZjE0MDViLi5jMDg3NDhhZjVkNjFjOTMzYmMxZTZkNmRiNzg4ODIx NmQ2ZGJlOTAwIDEwMDc1NQotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDUgKzEsMTYgQEAKIDIwMTEtMTAtMjEgIE5hdCBE dWNhICA8bmR1Y2FAY2hyb21pdW0ub3JnPgogCisgICAgICAgIFtjaHJvbWl1bV0gUm91dGUgQ29u c29sZTo6dGltZSBhbmQgQ29uc29sZTo6dGltZUVuZCB0byB0cmFjZV9ldmVudAorICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NzA2MjAKKworICAgICAgICBS ZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIHBhZ2UvQ29uc29sZS5jcHA6 CisgICAgICAgIChXZWJDb3JlOjpDb25zb2xlOjp0aW1lKToKKyAgICAgICAgKFdlYkNvcmU6OkNv bnNvbGU6OnRpbWVFbmQpOgorCisyMDExLTEwLTIxICBOYXQgRHVjYSAgPG5kdWNhQGNocm9taXVt Lm9yZz4KKwogICAgICAgICBbY2hyb21pdW1dIEFkZCBpc1RyYWNlRXZlbnRFbmFibGVkIHRvIFBs YXRmb3JtU3VwcG9ydC4KICAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcu Y2dpP2lkPTcwNjE4CiAKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BhZ2UvQ29uc29sZS5j cHAgYi9Tb3VyY2UvV2ViQ29yZS9wYWdlL0NvbnNvbGUuY3BwCmluZGV4IDc0ZGZlNzRhYjI0Mjk0 YmUzYjVlN2RjYjdlZmE4ZmNiZTM0MjkxYmIuLjM3YmU1ZGRlOTQyYTZmZjBiM2Q3OWFlN2VkMzA5 NzJjYWY1M2JlYjMgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3BhZ2UvQ29uc29sZS5jcHAK KysrIGIvU291cmNlL1dlYkNvcmUvcGFnZS9Db25zb2xlLmNwcApAQCAtNDksNiArNDksMTAgQEAK ICNpbmNsdWRlIDx3dGYvVW51c2VkUGFyYW0uaD4KICNpbmNsdWRlIDx3dGYvdGV4dC9DU3RyaW5n Lmg+CiAKKyNpZiBQTEFURk9STShDSFJPTUlVTSkKKyNpbmNsdWRlICJUcmFjZUV2ZW50LmgiCisj ZW5kaWYKKwogbmFtZXNwYWNlIFdlYkNvcmUgewogCiBDb25zb2xlOjpDb25zb2xlKEZyYW1lKiBm cmFtZSkKQEAgLTMwNCwxMCArMzA4LDE4IEBAIHZvaWQgQ29uc29sZTo6cHJvZmlsZUVuZChjb25z dCBTdHJpbmcmIHRpdGxlLCBTY3JpcHRTdGF0ZSogc3RhdGUsIFBhc3NSZWZQdHI8U2NyCiB2b2lk IENvbnNvbGU6OnRpbWUoY29uc3QgU3RyaW5nJiB0aXRsZSkKIHsKICAgICBJbnNwZWN0b3JJbnN0 cnVtZW50YXRpb246OnN0YXJ0Q29uc29sZVRpbWluZyhwYWdlKCksIHRpdGxlKTsKKyNpZiBQTEFU Rk9STShDSFJPTUlVTSkKKyAgICBpZiAoUGxhdGZvcm1TdXBwb3J0Ojppc1RyYWNlRXZlbnRFbmFi bGVkKCkpCisgICAgICAgIFBsYXRmb3JtU3VwcG9ydDo6dHJhY2VFdmVudEJlZ2luKHRpdGxlLnV0 ZjgoKS5kYXRhKCksIDAsIDApOworI2VuZGlmCiB9CiAKIHZvaWQgQ29uc29sZTo6dGltZUVuZChj b25zdCBTdHJpbmcmIHRpdGxlLCBQYXNzUmVmUHRyPFNjcmlwdEFyZ3VtZW50cz4sIFBhc3NSZWZQ dHI8U2NyaXB0Q2FsbFN0YWNrPiBjYWxsU3RhY2spCiB7CisjaWYgUExBVEZPUk0oQ0hST01JVU0p CisgICAgaWYgKFBsYXRmb3JtU3VwcG9ydDo6aXNUcmFjZUV2ZW50RW5hYmxlZCgpKQorICAgICAg ICBQbGF0Zm9ybVN1cHBvcnQ6OnRyYWNlRXZlbnRFbmQodGl0bGUudXRmOCgpLmRhdGEoKSwgMCwg MCk7CisjZW5kaWYKICAgICBJbnNwZWN0b3JJbnN0cnVtZW50YXRpb246OnN0b3BDb25zb2xlVGlt aW5nKHBhZ2UoKSwgdGl0bGUsIGNhbGxTdGFjayk7CiB9CiAK