Modernize Differential edges to subclass PhabricatorEdgeType. Largely based on D11045.
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Commits
- Restricted Diffusion Commit
rP7cab903943f5: Migrate Differential revision edges to use modern `EdgeType` subclasses
From previous experience, these changes are fairly trivial and safe. I poked around a little to make sure things looked reasonably okay.
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
One inline about cycle detection.
src/applications/differential/edge/DifferentialReviewerForRevisionEdgeType.php | ||
---|---|---|
17–19 | We should probably remove this -- it's not normally possible to introduce a cycle for this edge type (or most types of edges), and we pay a not-totally-trivial performance cost to do cycle detect on every write if it's enabled. The "Task depends on task" edge from the original reference diff was a rare case where cycle detection is of particular importance, since it's easy to introduce a cycle from the UI with that edge type. |
src/applications/differential/edge/DifferentialReviewerForRevisionEdgeType.php | ||
---|---|---|
17–19 | Remove this for all edges or just for DifferentialReviewerForRevisionEdgeType? Also, is DifferentialReviewer correct? I was also considering PeopleReviewer or something. |
The only edges which need cycle detection are:
- DifferentialRevisionDependsOnRevisionEdgeType (in this diff)
- ManiphestTaskDependsOnTaskEdgeType (in the reference diff)
- TYPE_TEST_NO_CYCLE (not converted yet)
Other edge types shouldn't activate cycle detection.
Also, is DifferentialReviewer correct? I was also considering PeopleReviewer or something.
Seems fine to me. Reviewers may also be projects, so PeopleReviewer would be less-good, I think.
src/applications/differential/edge/DifferentialRevisionDependsOnRevisionEdgeType.php | ||
---|---|---|
16–18 | That is: keep this one, ditch the others. |
If we need cycle detection for DifferentialRevisionDependsOnRevisionEdgeType then don't we also need it for DifferentialRevisionDependedOnByRevisionEdgeType?
As long as one of the two halves has cycle detection, cycles will be prevented. Putting cycle detection on both means we do twice as much work in cycle detection.
Cycle detection isn't normally particularly expensive, but it can require us to load a big part of the graph in some cases (i.e., we must load every connected edge) and we hold a global lock on the edge type while executing it, so doing twice as much of it means the lock is held twice as long.