Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider encapsulating performance behavior of ProjectDependencyGraph #41350

Open
sharwell opened this issue Jan 31, 2020 · 0 comments
Open

Consider encapsulating performance behavior of ProjectDependencyGraph #41350

sharwell opened this issue Jan 31, 2020 · 0 comments

Comments

@sharwell
Copy link
Member

#40358 and #41288 clean up ProjectDependencyGraph and improve performance. However, some of the performance characteristics of the graph require callers to interact with the API in non-obvious ways.

From @jasonmalinowski in #40358 (comment):

Rather than exposing this Try* method, are we better off just having a method which is "DoesATransitivelyDependOnB"? This brings up a few nice benefits:

  1. You don't need the odd Try* method wart which exposes the internal laziness.
  2. You can easily optimize it in other ways too and everybody gets the benefit. (i.e. if A just doesn't have dependencies at all, you know the answer as a shortcut. Or you can check the non-transitive ones first before invoking the computation....)
  3. The implementation here is making an implicit assumption about the other calls in the system. Checking both our forward and reverse maps is a great idea, but the implicit assumption is that if we have to compute one, we should compute the reverse transitive map. If we just move to a boolean-returning method, then other code like
    var dependents = _dependencyGraph.GetProjectsThatThisProjectTransitivelyDependsOn(fromProjectId);

    can also be updated to return this method so the assumption is consistent for everyone.
  4. It means you can get rid of your dependencies local since the deferring of that computation is now happening inside of ProjectDependencyGraph automatically. This might remove your need for the local function.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants