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

Box Block, fn_decl, variant and Ty in the AST. #10676

Merged
merged 1 commit into from
Dec 1, 2013

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Nov 26, 2013

Note: I only tested on top of my #10670 PR, size reductions come from both change sets.

With this, more enums are shrinked, the most significant one being ast_node, from 104 bytes (master) to 96 (#10670) and now to 32 bytes.

My own testcase requires 200MB less when compiling (not including the other 200MB gained in #10670), and rustc-stage2 is down by about 130MB.

I believe there is more to gain by fiddling with the enums' layouts.

@@ -236,9 +236,8 @@ impl Ctx {
visit::walk_stmt(self, stmt, ());
}

fn map_block(&mut self, b: &Block) {
// clone is FIXME #2543
self.map.insert(b.id, node_block((*b).clone()));
Copy link
Member Author

Choose a reason for hiding this comment

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

I removed this clone, but not others (the variant one just below and some in the parser), any suggestions?

Copy link
Member

Choose a reason for hiding this comment

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

oddly #2543 was closed by PR #9088 even though this comment remained in the source. I assume @nikomatsakis did mean to close it, since that PR removed a large number of the copies, but clearly not all of them...

@sanxiyn
Copy link
Member

sanxiyn commented Nov 28, 2013

Should this really use @ box instead of ~ box?

@eddyb
Copy link
Member Author

eddyb commented Nov 28, 2013

~T doesn't allow sharing - however, I'm preparing a new patch where I use P<T> instead of @T, and P(value) instead of @value to allocate.
This P<T> trick works with type P<T> = &'static T (coming from an arena), and with the upcoming smart pointer additions (the custom deref and borrow traits), type P<T> = Rc<T> could work just as well.

I will try to implement a proper arena, instead of leaking, with safe-ish (always fail on ptr deref) post-drop behavior (maybe by mmap-ing with MAP_NORESERVE and no permissions, after munmap-ing each chunk).

@eddyb
Copy link
Member Author

eddyb commented Nov 28, 2013

Nevermind my previous idea, I'm using @T again, but I've kept the P<T> wrappers to help with future refactoring. I still can't use type P<T> = Rc<T> because the smart pointer improvements discussed by @nikomatsakis are required.

@pcwalton
Copy link
Contributor

What improvements are those in particular? I would like to remove @ from the language ASAP…

@emberian
Copy link
Member

memory graph

@emberian
Copy link
Member

(Note that this is without -O)

@thestinger
Copy link
Contributor

@pcwalton: AFAIK he just means the dereference operator overload

@eddyb
Copy link
Member Author

eddyb commented Nov 29, 2013

@pcwalton custom (auto-)dereference, (auto-)borrow and (optionally) use-after-move handler - so explicit cloning to split ownership isn't required.
But this last one is rare enough, so I can live with a P::split_ownership(&P<T>) wrapper that takes a reference and returns a new P<T> pointing to the same object (ref-count bump for Rc<T>).
Oh, and using Rc<T> without replacing every @T in the AST won't work right now, because the ~RcBox allocation ends up being a @RcBox and then it segfaults (because of the different layout).

@pnkfelix
Copy link
Member

For me, sha 468657c fails make during an attempt to build librustdoc atop the stage1 compiler.

@eddyb
Copy link
Member Author

eddyb commented Nov 30, 2013

Rebased and updated rustdoc - my bad, I always forget to fix the tests.

@emberian
Copy link
Member

Squash and r=me

bors added a commit that referenced this pull request Dec 1, 2013
**Note**: I only tested on top of my #10670 PR, size reductions come from both change sets.

With this, [more enums are shrinked](https://gist.github.com/eddyb/08fef0dfc6ff54e890bc), the most significant one being `ast_node`, from 104 bytes (master) to 96 (#10670) and now to 32 bytes.

My own testcase requires **200MB** less when compiling (not including the other **200MB** gained in #10670), and rustc-stage2 is down by about **130MB**.

I believe there is more to gain by fiddling with the enums' layouts.
@bors bors closed this Dec 1, 2013
@bors bors merged commit a9c4b18 into rust-lang:master Dec 1, 2013
@eddyb eddyb deleted the ast-box-in-enums branch December 1, 2013 12:26
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.

7 participants