Page MenuHomePhorge

Remove unused code in ManiphestReportController::renderBurn()
AcceptedPublic

Authored by aklapper on Mar 4 2025, 11:03.
Tags
None
Referenced Files
F3711901: D25902.1745662991.diff
Fri, Apr 25, 10:23
F3702685: D25902.1745628320.diff
Fri, Apr 25, 00:45
F3605051: D25902.1745251476.diff
Sun, Apr 20, 16:04
F3399922: D25902.1744552176.diff
Sat, Apr 12, 13:49
F3388357: D25902.1744446481.diff
Fri, Apr 11, 08:28
F3388355: D25902.1744446464.diff
Fri, Apr 11, 08:27
F3388353: D25902.1744446453.diff
Fri, Apr 11, 08:27
F3368218: D25902.1744206695.diff
Tue, Apr 8, 13:51

Details

Summary

ManiphestReportController (which renders global /maniphest/report/burn/) wastes CPU cycles. Thus remove useless code.

It constructs table rows never to be shown: The code calculates rows and adds them via $table = new AphrontTableView($rows), then defines $panel = new PHUIObjectBoxView() and does $panel->setTable($table) but a bit later $panel = id(new PhabricatorProjectBurndownChartEngine()) overwrites that table and all its data.
This re-definition of $panel was introduced in rP5c1b91ab457db9f3db10d8cc5e07831512645ebb.

Its code creates a PhabricatorProjectBurndownChartEngine anyway, similar to PhabricatorProjectReportsController. So none of those expensive database queries are needed at all. Variables like list($burn_x, $burn_y) = $this->buildSeries($data); are never read since rPf8ebc71b8f217ed156f416ddb4cd028dcaa28174.

Bug: T16005

Test Plan
  1. Make sure /applications/view/PhabricatorFactApplication/ is installed
  2. Go to /maniphest/report/burn/ for global view, go to some URL like /maniphest/report/burn/?project=PHID-PROJ-1234567890abcdef for per-project view
  3. See that the "Burnup Rate" chart is rendered in the same way before and after applying this patch
  4. Optionally, run ./bin/cache purge --all (but seems not relevant here) and reload
  5. Optionally, go to /project/reports/1/ and as expected the burndown chart is the same as in step 2

Diff Detail

Repository
rP Phorge
Branch
T16005ChartPerform (branched from master)
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 1762
Build 1762: arc lint + arc unit

Event Timeline

aklapper requested review of this revision.Mar 4 2025, 11:03
aklapper retitled this revision from Remove unused code in ManiphestReportController:renderBurn() to Remove unused code in ManiphestReportController::renderBurn().Mar 4 2025, 11:06

It seems that there is even more code to rip out here

Rip out more unused ancient code like unneeded expensive database queries

aklapper edited the test plan for this revision. (Show Details)
aklapper edited the test plan for this revision. (Show Details)

I've tested this again in my production on gitpull.it (lol) and I cannot see whatsoever change in the resulting project report (and I'm sure that the page I'm testing is the one affected since I've introduced some logs in-code). So, approving 👍

I was a bit curious of the performance benefit. This is a first check on the report page itself. Note that I only checked the server-side page generation at /maniphest/report/?project=<phid>, NOT the internal ajax calls.

The benefit seems visible even without considering Ajax. Lower is better:

A/B Test of /maniphest/report/?project=<phid> (with logged-in GET cURL call)

D25902-ab-check.png (481×855 px, 30 KB)

May God and Greta Thunberg perdon our souls for what happened before this patch.

This revision is now accepted and ready to land.Fri, May 2, 13:31