It would be nice to show perf bots on the WebKit dashboard alongside builders and testers.
I'll be working on this.
Created attachment 237383 [details] Implementation to display performance bots on the dashboard
Comment on attachment 237383 [details] Implementation to display performance bots on the dashboard View in context: https://bugs.webkit.org/attachment.cgi?id=237383&action=review Overall this looks fine, although it looks like adding the concept of perf bots doesn't quite fit into the assumptions about debug and release builds (based on some of the if statements added). I don't think that should stop us from landing this change, though. r=me, but I'd prefer it if someone more familiar with the code looked at it, too. (I CCed some folks.) > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/WebKitBuildbot.js:54 > + "EFL Linux 64-bit Release WK2": {platform: Dashboard.Platform.LinuxEFL, tester: true, testCategory: Buildbot.TestCategory.WebKit2}, > + "EFL Linux 64-bit Release WK2 (Perf)": {platform: Dashboard.Platform.LinuxEFL, tester: true, testCategory: Buildbot.TestCategory.Performance} These seem unrelated to the patch (although I suppose it's okay to add them now while you're in the neighborhood).
Comment on attachment 237383 [details] Implementation to display performance bots on the dashboard View in context: https://bugs.webkit.org/attachment.cgi?id=237383&action=review Dana and myself checked whether this new column fits on a 13" screen, and it doesn't quite. Fortunately, the layout can be squeezed very very slightly to make it fit, but we can't have any more columns (which is a bummer, as there are also leaks bots and other misc ones). > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Buildbot.js:62 > + Performance: "perf" We avoid abbreviations in WebKit code base. Please change all "perf"s into complete words. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotTesterQueueView.js:128 > + if (queue.perfType) { > + label = queue.perfType; > + } There are no braces around single line blocks in WebKit coding style. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotTesterQueueView.js:146 > + if (this.perfQueues) { > + appendBuild.call(this, this.perfQueues); > + } Ditto. Also, this check is always true anyway, as [] converts to true. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:56 > var buildType = queue.debug ? "debug" : "release"; > + buildType = queue.perfType ? "perfType" : buildType; I'd probably use a nested conditional here - it's unnecessarily confusing to initialize a variable and immediately override it. Not a big deal though. >> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/WebKitBuildbot.js:54 >> + "EFL Linux 64-bit Release WK2 (Perf)": {platform: Dashboard.Platform.LinuxEFL, tester: true, testCategory: Buildbot.TestCategory.Performance} > > These seem unrelated to the patch (although I suppose it's okay to add them now while you're in the neighborhood). OK to land, but I'd still suggest splitting this into a separate patch.
Actually, I have some more substantive comments. 1. The result bubbles are yellow, because neither BuildbotIteration nor BuildbotTesterQueueView know perfbot's productive step name, so they fall into "something was wrong, but it was not a failure" mode. 2. In addition to buildbot page, there should be a link to actual results on perf dashboard. Dana and myself looked into where to put them, and it seems like a popover would be best. As we are talking about a popover, it might be best to have a custom performance queue view, and not just reuse BuildbotTesterQueueView.
Created attachment 237528 [details] Implementation to display performance bots on the dashboard
Uploaded a patch to take care of Alexey's concerns. When Ryosuke gets back, we should discuss the possibility of adding more information to the popover, as it only displays a link to perf.webkit.org for the time being.
Created attachment 237532 [details] *Actual* implementation *Really* updated the patch this time. I am not good at bugzilla.
Created attachment 237533 [details] Update ChangeLog to account for new BuildbotPerformanceQueueView class.
Comment on attachment 237532 [details] *Actual* implementation View in context: https://bugs.webkit.org/attachment.cgi?id=237532&action=review > Tools/ChangeLog:15 > + Support new 'performance' and 'perfType' keys. Looks like the ChangeLog is stale. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:238 > + function collectPerfTestResults(data, stepName) { s/Perf/Performance/g > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:253 > + if (!testStep.results || !testStep.results[0]) { Should this be if (!testStep.results || testStep.results[0] === BuildbotIteration.SUCCESS) ? > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:263 > + if (!testResults.failureCount) { > + testResults.errorOccurred = true; > + } We don't use braces around single line blocks. This code looks super confusing at first (why? no failures means that an error occurred?). Can you explain this situation in a comment? If you do, it won't be single line any more, so you should keep the braces :) > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:318 > + var webkitPerformanceTestResults = collectTestResults.call(this, data, "perf-test"); collectPerfTestResults or collectTestResults? If it's the latter, it needs a comment explaining why, or someone will "fix" it for consistency. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotPerformanceQueueView.js:28 > + BuildbotQueueView.call(this, [], queues); We should probable relieve BuildbotQueueView of knowing the distinction between debug and release queues (not in this patch). All subclasses add their own logic on top of it, sometimes fighting with BuildbotQueueView for control. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotPerformanceQueueView.js:44 > + function appendPerfQueueStatus(queue) s/Perf/Performance/g > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotPerformanceQueueView.js:91 > + releaseLabel.textContent = queue.perfType ? queue.perfType : label; label should probably be named "defaultLabel" or something. Not seeing how perfType is used, I'm not sure about the intended design. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotPerformanceQueueView.js:119 > + this.addLinkToRow(row, "dasboard-link", "Performance Dashboard", queue.buildbot.perf_dashboard_url); Typo: dasboard. This style is not defined in CSS. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:44 > + this.perfType = info.perfType || null; s/perf/performance/g But perfType doesn't appear to be set anywhere. Is this just dead code, or did you intend to set it? > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/WebKitBuildbot.js:54 > + "GTK Linux 64-bit Release (Perf)": {platform: Dashboard.Platform.LinuxGTK, debug: false, tester: true, testCategory: Buildbot.TestCategory.Performance}, > + "EFL Linux 64-bit Release WK2": {platform: Dashboard.Platform.LinuxEFL, tester: true, testCategory: Buildbot.TestCategory.WebKit2}, > + "EFL Linux 64-bit Release WK2 (Perf)": {platform: Dashboard.Platform.LinuxEFL, performance: true} I couldn't understand the difference between a performance bot like the GTK one, and a testCategory: Buildbot.TestCategory.Performance bot like the EFL one. How does one go about figuring this out? > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/WebKitBuildbot.js:65 > + perf_dashboard_url: "https://perf.webkit.org", Coding style nit: should be performanceDashboardURL, not perf_dashboard_url. It would be nice to make this a getter function for consistency with other URL getters.
(In reply to comment #10) > (From update of attachment 237532 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=237532&action=review > > > Tools/ChangeLog:15 > > + Support new 'performance' and 'perfType' keys. > > Looks like the ChangeLog is stale. > > > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:238 > > + function collectPerfTestResults(data, stepName) { > > s/Perf/Performance/g > OK > > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:253 > > + if (!testStep.results || !testStep.results[0]) { > > Should this be > > if (!testStep.results || testStep.results[0] === BuildbotIteration.SUCCESS) > > ? > No, we're actually testing for failure of any kind, not success (failure is non-zero). > > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:263 > > + if (!testResults.failureCount) { > > + testResults.errorOccurred = true; > > + } > > We don't use braces around single line blocks. > > This code looks super confusing at first (why? no failures means that an error occurred?). Can you explain this situation in a comment? If you do, it won't be single line any more, so you should keep the braces :) > OK. I was following the style of some similar code. The reason is that we should have already either hit a success *or* failure case, and if we haven't something is wrong, so we set 'errorOccurred' instead of failure > > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:318 > > + var webkitPerformanceTestResults = collectTestResults.call(this, data, "perf-test"); > > collectPerfTestResults or collectTestResults? If it's the latter, it needs a comment explaining why, or someone will "fix" it for consistency. > Good point. It's correct, although strange, so I'll add a comment. collectPerfTestResults() is for compatibility with the internal buildbots. > > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotPerformanceQueueView.js:28 > > + BuildbotQueueView.call(this, [], queues); > > We should probable relieve BuildbotQueueView of knowing the distinction between debug and release queues (not in this patch). > > All subclasses add their own logic on top of it, sometimes fighting with BuildbotQueueView for control. > > > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotPerformanceQueueView.js:44 > > + function appendPerfQueueStatus(queue) > > s/Perf/Performance/g > OK > > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotPerformanceQueueView.js:91 > > + releaseLabel.textContent = queue.perfType ? queue.perfType : label; > > label should probably be named "defaultLabel" or something. Not seeing how perfType is used, I'm not sure about the intended design. > > > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotPerformanceQueueView.js:119 > > + this.addLinkToRow(row, "dasboard-link", "Performance Dashboard", queue.buildbot.perf_dashboard_url); > > Typo: dasboard. > Good catch. > This style is not defined in CSS. > Nor are many of the other arbitrary classes added to the procedurally generated HTML, I am simply following the same style. > > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:44 > > + this.perfType = info.perfType || null; > > s/perf/performance/g > > But perfType doesn't appear to be set anywhere. Is this just dead code, or did you intend to set it? > It is not set on the OpenSource dashboard, as we have only one type of performance bot on build.webkit. It is used for the internal configuration to change the heading (which normally says 'Release') > > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/WebKitBuildbot.js:54 > > + "GTK Linux 64-bit Release (Perf)": {platform: Dashboard.Platform.LinuxGTK, debug: false, tester: true, testCategory: Buildbot.TestCategory.Performance}, > > + "EFL Linux 64-bit Release WK2": {platform: Dashboard.Platform.LinuxEFL, tester: true, testCategory: Buildbot.TestCategory.WebKit2}, > > + "EFL Linux 64-bit Release WK2 (Perf)": {platform: Dashboard.Platform.LinuxEFL, performance: true} > > I couldn't understand the difference between a performance bot like the GTK one, and a testCategory: Buildbot.TestCategory.Performance bot like the EFL one. > > How does one go about figuring this out? > That's actually a mistake on my part, good catch! They should both look like the EFL entry. > > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/WebKitBuildbot.js:65 > > + perf_dashboard_url: "https://perf.webkit.org", > > Coding style nit: should be performanceDashboardURL, not perf_dashboard_url. > > It would be nice to make this a getter function for consistency with other URL getters. OK. Thanks for then review Alexey, I'll have a new patch addressing these concerns up shortly.
(In reply to comment #11) > (In reply to comment #10) > > > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:253 > > > + if (!testStep.results || !testStep.results[0]) { > > > > Should this be > > > > if (!testStep.results || testStep.results[0] === BuildbotIteration.SUCCESS) > > > > ? > > > > No, we're actually testing for failure of any kind, not success (failure is non-zero). > Actually, I misread the context. I am testing for success, however I was following the same convention used earlier in the file. I can change it in both places if you want.
Created attachment 237540 [details] Updated patch with Alexey's suggestions.
Comment on attachment 237540 [details] Updated patch with Alexey's suggestions. View in context: https://bugs.webkit.org/attachment.cgi?id=237540&action=review > I can change it in both places if you want. Yes, I think that comparing to BuildbotIteration.SUCCESS reads better. The existing place probably pre-dates adding these named constants. > Tools/ChangeLog:18 > + Support new 'performance' and 'perfType' keys. Stale ChangeLog, perfType is no more. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:259 > + testResults.failureCount = (testStep.results[0]) ? 1 : 0; I think that this is not quite how the other result objects work. Failure count is for steps that actually have failure counts (like layout tests). Besides, what are the possible values for testStep.results[0] here? We know that it's not 0, because that case is handled above. How else can testStep.results[0] evaluate to false? This needs a comment if that can actually happen. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:315 > + // This is actually correct, we're calling collectTestResults vs. collectPerformanceTestResults because of > + // differences in the internal buildbot. "This is actually correct" is superfluous, you can just say "We are calling collectTestResults and not collectPerformanceTestResults because of differences in the internal buildbot." A little more detail about what the differences are could be appropriate (cf. how we explain revision number formats for internal bots with two source trees). > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotPerformanceQueueView.js:91 > + releaseLabel.textContent = queue.performanceType ? queue.performanceType : label; This still seems a little confusing. What is "performance type", that doesn't parse. Perhaps performanceTestName would be more descriptive? > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotPerformanceQueueView.js:116 > + content.className = "ews-popover"; Didn't notice this at first - since it's not a EWS popover, please don't use its class names. Duplicating style rules is OK. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/WebKitBuildbot.js:66 > + performanceDashboardURL: function() I now think that I gave the wrong advice here. I was thinking of a getter function, but we have regular functions when there is an argument, and simple attributes like baseURL when there is none.
Committed in r173211.