Page MenuHomePhabricator

Migrate Differential revision edges to use modern `EdgeType` subclasses
ClosedPublic

Authored by joshuaspence on Dec 30 2014, 11:30 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 26, 6:41 PM
Unknown Object (File)
Tue, Mar 26, 6:34 PM
Unknown Object (File)
Tue, Mar 26, 6:30 PM
Unknown Object (File)
Tue, Mar 26, 6:28 PM
Unknown Object (File)
Tue, Mar 26, 6:04 PM
Unknown Object (File)
Tue, Mar 26, 5:48 PM
Unknown Object (File)
Tue, Mar 26, 5:31 PM
Unknown Object (File)
Tue, Mar 26, 4:17 PM

Details

Summary

Modernize Differential edges to subclass PhabricatorEdgeType. Largely based on D11045.

Test Plan

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

joshuaspence retitled this revision from to Migrate differential revision edge types to use modern `EdgeType` subclasses.
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)

Migrate remaining differential edges

joshuaspence retitled this revision from Migrate differential revision edge types to use modern `EdgeType` subclasses to Migrate differential revision edges to use modern `EdgeType` subclasses.Jan 1 2015, 1:07 AM
epriestley added a reviewer: epriestley.

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.

This revision is now accepted and ready to land.Jan 1 2015, 1:47 AM
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?

Remove unnecessary cycle detection

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.

joshuaspence retitled this revision from Migrate differential revision edges to use modern `EdgeType` subclasses to Migrate Differential revision edges to use modern `EdgeType` subclasses.Jan 1 2015, 2:26 AM
joshuaspence updated this object.
This revision was automatically updated to reflect the committed changes.