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

Refactor task system for efficiency #56717

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@Zoxc
Copy link
Contributor

Zoxc commented Dec 11, 2018

Split from #56509

r? @michaelwoerister

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

michaelwoerister commented Dec 12, 2018

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 12, 2018

⌛️ Trying commit 88531f4 with merge 3bb0bed...

bors added a commit that referenced this pull request Dec 12, 2018

Auto merge of #56717 - Zoxc:query-perf3, r=<try>
Refactor task system for efficiency

Split from #56509

r? @michaelwoerister
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 12, 2018

☀️ Test successful - status-travis
State: approved= try=True

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

michaelwoerister commented Dec 12, 2018

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Dec 12, 2018

Success: Queued 3bb0bed with parent a64cdec, comparison URL.

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Dec 12, 2018

Finished benchmarking try commit 3bb0bed

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

michaelwoerister commented Dec 14, 2018

The perf results look like there's no real improvement here. I see -0.4-7% in some cases but also +0.8% in others. As such, I'd prefer not to do these changes.

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

michaelwoerister commented Dec 19, 2018

@Zoxc, would you be OK with closing this PR? It changes quite a bit of code but the benefit of that is doubtful.

@Zoxc

This comment has been minimized.

Copy link
Contributor

Zoxc commented Dec 19, 2018

Could you make your objections a bit more concrete? I like that this splits up with_task_impl which is rather large currently, and that ImplicitCtxt is only changed once when executing a query.

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

michaelwoerister commented Dec 19, 2018

My objections are:

  • Changing code in a multi-user codebase comes at a cost. People who understood the code before have to invest time into understanding it again. A change also needs to be reviewed, which costs time and energy, especially if the change affects complicated logic. It might also introduce new bugs that are not spotted during review, causing follow-up work. So any (non-trivial) change starts out with a kind of a debt that it has to make up for in improved code quality/readability, improved runtime efficiency, or some other improvement. The changes in this PR don't make the code more efficient in practice, increase code duplication, and makes already too complicated logic a bit more complicated, so in my estimation it doesn't offset the debt it introduces.
  • The current version of the code follows a kind of template pattern with with_task_impl giving a skeleton for the logic that needs to be executed for all kinds of task. This reduces code duplication and thus the risk of forgetting to update other the kinds of tasks, when fixing a bug in one.
  • It also gives the information that different kinds of tasks basically follow the same kind of logic, which is quite a big win for making the code more readable, I think.
  • The version in this PR increases the number of public DepGraph::with_* methods from 5 to 7, making it even harder to understand which one to use. I'd rather see the number reduced, if possible.
  • The interaction between query system and task system becomes more circular than before, increasing the coupling of the two systems.
  • It breaks the ProfileQueriesMsgcalls, which to me seems like an indication that there is too much duplication of logic.
@Dylan-DPC

This comment has been minimized.

Copy link
Member

Dylan-DPC commented Jan 14, 2019

ping from triage @michaelwoerister @Zoxc what's the update on this?

@Zoxc Zoxc closed this Jan 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment