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

Streamline `Compiler` #64016

Open
wants to merge 4 commits into
base: master
from

Conversation

@nnethercote
Copy link
Contributor

commented Aug 30, 2019

A few commits to clean up Compiler.

r? @Zoxc

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

☔️ The latest upstream changes (presumably #63827) made this pull request unmergeable. Please resolve the merge conflicts.

@Zoxc

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

Some of the changes here seems questionable (the shuffling of code and the removal of compile). This also conflicts with #59904.

@nnethercote nnethercote changed the title Streamling `Compiler` Streamline `Compiler` Aug 30, 2019

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

This is where I express some frustration.

  • Your feedback is not actionable. I have spent time trying to improve the quality of the code. Please be more specific in your criticisms, so that I (a) actually know what they are, and (b) can respond to them and/or act on them.
  • You have said this conflicts with #59904. That is one of a chain of 5 PRs. None of them have any explanation of what they are doing beyond the PR title. They all date from March, two of them (#59904 and #59338) are closed, and the other two are tagged with S-blocked or S-waiting-on-team. It is difficult for me to understand the intention and status of these PRs in their current state.
  • My motivation for looking at this code was that I want to try moving metadata generation earlier, to see if it improves the effectiveness of pipelining. This requires understanding the main passes of the compiler, which turns out to be far more complex than I expected due to the implicit ordering that occurs in this query-based design. My first attempt had no effect because I didn't realize that passes could be called multiple times, with the latter calls effectively being no-ops. I spent an afternoon diagramming the passes and their dependencies in order to understand how it works. (The diagram is below.) The changes in this PR minimize and simplify dependencies, merge two unnecessarily-separated passes, and remove an inconsistent function. I think they are worthwhile, and I would appreciate a more respectful engagement with them.

IMG-4423

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2019

Here is an updated picture showing things after my changes. The graph is neater partly because I did a better job drawing it, but also partly because it's simpler: it has three fewer boxes (passes::register_plugins, compile, and ongoing_codegen & link have been combined into codegen_and_link) and fewer arrows.

IMG-4424

nnethercote added 2 commits Aug 29, 2019
Remove `lower_to_hir()` call from `prepare_output()`.
It's a false dependency. The result isn't used and there are no
relevant side-effects.

@nnethercote nnethercote force-pushed the nnethercote:Compiler-fiddling branch from c1c386e to 392b91f Sep 1, 2019

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2019

#63827 landed over the weekend, which removed one of the Compiler::compile() calls. As a result, the last patch is now more compelling.

@nnethercote nnethercote force-pushed the nnethercote:Compiler-fiddling branch from 392b91f to 92a0cbd Sep 1, 2019

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

The S-waiting-on-author tag is incorrect, but I don't know how to remove it.

@lzutao

This comment was marked as resolved.

Copy link
Contributor

commented Sep 3, 2019

@rustbot modify labels: -S-waiting-on-author S-waiting-on-review

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

r? @oli-obk

Because they reviewed #56732.

@rust-highfive rust-highfive assigned oli-obk and unassigned Zoxc Sep 5, 2019


// Drop GlobalCtxt after starting codegen to free memory
mem::drop(compiler.global_ctxt()?.take());
compiler.codegen_and_link()?;

This comment has been minimized.

Copy link
@oli-obk

oli-obk Sep 6, 2019

Contributor

having reviewed @Zoxc's PRs it seems to me like only this commit is actually problematic for the querification PRs (like those PRs would have to revert this commit to be doable). As far as I can tell (both from the code and your graphs) it's not that much additional complexity to keep around either.

What do you think?

This comment has been minimized.

Copy link
@nnethercote

nnethercote Sep 6, 2019

Author Contributor

Thank you for the feedback. Are you referring to #59404? If so:

  • If there's a good reason to keep the ongoing_codegen/link split then I am fine to remove this commit.
  • Is there a good reason? Does #59404 and its predecessors use the existing passes because they are there, rather than because they are necessary?

This comment has been minimized.

Copy link
@Zoxc

Zoxc Sep 7, 2019

Contributor
  • If there's a good reason to keep the ongoing_codegen/link split then I am fine to remove this commit.

The main reason to keep this separate because ongoing_codegen needs the GlobalCtxt to be around, while we want to free GlobalCtxt before linking. When we remove queries from rustc_interface this mean this cannot be a combined operation.

Another reason is that it's reasonable for front-ends to do only code generation, but no linking, although that is not yet possible.

This comment has been minimized.

Copy link
@nnethercote

nnethercote Sep 8, 2019

Author Contributor

I don't understand the point about GlobalCtxt. My patch preserves the "free GlobalCtxt before linking" behaviour.

As for the second point, I would argue that YAGNI applies.

This comment has been minimized.

Copy link
@oli-obk

oli-obk Sep 8, 2019

Contributor

The problem isn't that your PR doesn't keep the early drop, it's the way the query PR works, where the GlobalCtxt lives as long as a closure and is automatically dropped at the end of it. So you can't call the combined function inside the closure and you can't call it outside the closure, thus needing to split it up again.

@@ -261,9 +260,6 @@ pub fn register_plugins<'a>(
});
}

// If necessary, compute the dependency graph (in the background).
compiler.dep_graph_future().ok();

This comment has been minimized.

Copy link
@Zoxc

Zoxc Sep 7, 2019

Contributor

Moving the dep graph loading to a later point is a performance regression. This is currently done as early as possible, although I'd like for it be done immediately when creating the compiler context, but that requires some changes as currently we need to do this after parsing to pick up the crate name.

This comment has been minimized.

Copy link
@nnethercote

nnethercote Sep 8, 2019

Author Contributor

The performance run shows that these five commits provide a slightly performance improvement.

).map_err(|_| ErrorReported)?;

Ok(())
})
}

pub fn compile(&self) -> Result<()> {

This comment has been minimized.

Copy link
@Zoxc

Zoxc Sep 7, 2019

Contributor

This is meant as an simple utility function and an example for users of rustc_interface of something that mirrors a regular compilation session without all the complexities of the full rustc driver (in rustc_driver).

This comment has been minimized.

Copy link
@nnethercote

nnethercote Sep 8, 2019

Author Contributor

If an example is needed, why not provide it in documentation? Don't leave unnecessary code in the product.

This comment has been minimized.

Copy link
@pnkfelix

pnkfelix Sep 17, 2019

Member

Do we have CI testable example code snippets in the rustc-guide?

  • Or maybe we could put such a snippet into the rustdoc comments that we process for rustc's generated documentation?

If either of the above is true, then I could go along with @nnethercote 's reasoning here, though then I would want that migration to happen either before this PR, or as part of this PR.

(Otherwise, I'm with @Zoxc; Its hard enough for people to get started using the compilier as a library or hacking on rustc itself; simple examples of how to use things like rustc_interface are in their own way part of our product...)

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented Sep 7, 2019

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2019

⌛️ Trying commit 92a0cbd with merge 468729d...

bors added a commit that referenced this pull request Sep 7, 2019
Auto merge of #64016 - nnethercote:Compiler-fiddling, r=<try>
Streamline `Compiler`

A few commits to clean up `Compiler`.

r? @Zoxc
@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2019

☀️ Try build successful - checks-azure
Build commit: 468729d

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2019

@rust-timer

This comment has been minimized.

Copy link

commented Sep 8, 2019

Success: Queued 468729d with parent ef54f57, comparison URL.

@rust-timer

This comment has been minimized.

Copy link

commented Sep 8, 2019

Finished benchmarking try commit 468729d, comparison URL.

Move call site of `dep_graph_future()`.
`Compiler::register_plugins()` calls `passes::register_plugins()`, which
calls `Compiler::dep_graph_future()`. This is the only way in which a
`passes` function calls a `Compiler` function.

This commit moves the `dep_graph_future()` call out of
`passes::register_plugins()` and into `Compiler::register_plugins()`,
which is a more sensible spot for it. This will delay the loading of the
dep graph slightly -- from the middle of plugin registration to the end
of plugin registration -- but plugin registration is fast enough
(especially compared to expansion) that the impact should be neglible.

@nnethercote nnethercote force-pushed the nnethercote:Compiler-fiddling branch from 92a0cbd to 443e6d2 Sep 11, 2019

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

I removed the commit that merges ongoing_codegen with link. I think the other four commits are still worth landing.

Remove `Compiler::compile()`.
`Compiler::compile()` is different to all the other `Compiler` methods
because it lacks a `Queries` entry. It only has one call site, which is
in a test that doesn't need its specific characteristics.

This patch removes `Compile::compile()` and replaces the call to it in
the test with a call to `Compile::link()`, which is similar enough for
the test's purposes.

@nnethercote nnethercote force-pushed the nnethercote:Compiler-fiddling branch from 443e6d2 to bdf5b54 Sep 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.