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

Move trans, back, driver, and back into a new crate, rustc_trans. #19002

Closed
wants to merge 1 commit into from

Conversation

nikomatsakis
Copy link
Contributor

Move trans, back, driver, and back into a new crate, rustc_trans (which probably needs a better name). This is not a principled split per se, though it does make some sense, as the things which have been moved all fall into "back end" territory. The primary motivation is reducing memory usage. Measurements with massif suggest that peak memory usage after this change is 1.5GB. In comparison, measurements on master show a range from 1.8 to 2.5 GB of usage.

r? @brson
cc @alexcrichton
cc #18784

@rust-highfive
Copy link
Collaborator

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

@ghost
Copy link

ghost commented Nov 16, 2014

❤️

@@ -119,7 +117,8 @@ fn runtest(test: &str, cratename: &str, libs: Vec<Path>, externs: core::Externs,
maybe_sysroot: Some(os::self_exe_path().unwrap().dir_path()),
addl_lib_search_paths: RefCell::new(libs),
crate_types: vec!(config::CrateTypeExecutable),
output_types: vec!(write::OutputTypeExe),
output_types: vec!(config
::OutputTypeExe),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extraneous newline?

@pcwalton
Copy link
Contributor

Might want to update README.txt in librustc now.

@pcwalton
Copy link
Contributor

Also (this can be a followup of course) we might as well rename librustc to librustc_middle at this point. (Or even librustc_sema following clang? I always liked the name "sema" better than "middle", especially since trans was historically part of "middle"…)

@arielb1
Copy link
Contributor

arielb1 commented Nov 16, 2014

How much does this affect compile times (by reducing inlining)?

@alexcrichton
Copy link
Member

This looks amazing @nikomatsakis! I think @pcwalton's right in that rustc doesn't make too too much sense any more now that it's sharded so much, but r=me anyway on this PR.

@nikomatsakis
Copy link
Contributor Author

On Sun, Nov 16, 2014 at 12:22:59PM -0800, Alex Crichton wrote:

This looks amazing @nikomatsakis! I think @pcwalton's right in that rustc doesn't make too too much sense any more now that it's sharded so much, but r=me anyway on this PR.

Yes, I agree, the names are bad, and I liked his suggestions. I'm
inclined to leave that for a follow-up PR that just does the renaming.
I'm not sure what would be best, though @pcwalton's suggestions make
sense (though I'd probably go ahead and spell out "semantic" or
"analysis" rather than "sema" (I presume that is what it stands for))

@nikomatsakis
Copy link
Contributor Author

On Sun, Nov 16, 2014 at 07:40:39AM -0800, arielb1 wrote:

How does this affect compile times (by reducing inlining)?

I haven't measured compilation time, though I would expect an
improvement because of increased ability to parallelize and because
there is less overall code to compile at a time.

…uces memory usage significantly and opens opportunities for more parallel compilation.
@nikomatsakis
Copy link
Contributor Author

Closing because bors seems confused by this PR.

bors added a commit that referenced this pull request Nov 18, 2014
Reduces memory usage significantly and opens opportunities for more parallel compilation.

This PR was previously #19002 but I closed it because bors didn't seem to recognize the `r+` annotations there.
bors added a commit that referenced this pull request Nov 18, 2014
Reduces memory usage significantly and opens opportunities for more parallel compilation.

This PR was previously #19002 but I closed it because bors didn't seem to recognize the `r+` annotations there.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants