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

syntax: replace OptVec with plain Vec. #12675

Closed
wants to merge 1 commit into from

Conversation

huonw
Copy link
Member

@huonw huonw commented Mar 4, 2014

syntax: replace OptVec with plain Vec.

The new vector representation has no allocations when empty, so OptVec
is now useless overhead.

The new vector representation has no allocations when empty, so OptVec
is now useless overhead.
@huonw
Copy link
Member Author

huonw commented Mar 4, 2014

@thestinger raised a good point on IRC: using a plain Vec uses more memory in the empty case than the old OptVec did, and maybe we want to preserve/restore that behaviour. Furthermore, the AST is immutable, so the capacity field is entirely useless.

Currently IRFY isn't showing the memory use of the OptVec's ~[] -> Vec change (which increased it from 1 word to 4), so it might be reasonable to wait until that's measured before deciding whether this or changing the internal representation of OptVec entirely is better.

@nikomatsakis
Copy link
Contributor

This is nice. (Eventually all OptVec should change to ~[T], I think, which should also be zero alloc when empty.)

@pnkfelix
Copy link
Member

pnkfelix commented Mar 7, 2014

cc me

@alexcrichton
Copy link
Member

r=me (looks like bors got confused)

@huonw
Copy link
Member Author

huonw commented Mar 9, 2014

I cancelled the build, pending memory benchmarks (#12675 (comment)).

pnkfelix added a commit to pnkfelix/rust that referenced this pull request Mar 10, 2014
There is a broader revision (that does this across the board) pending
in rust-lang#12675, but that is awaiting the arrival of more data (to decide
whether to keep OptVec alive by using a non-Vec internally).

For this code, the representation of lifetime lists needs to be the
same in both ScopeChain and in the ast and ty structures.  So it
seemed cleanest to just use `vec_ng::Vec`, now that it has a cheaper
empty representation than the current `vec` code.
pnkfelix added a commit to pnkfelix/rust that referenced this pull request Mar 12, 2014
There is a broader revision (that does this across the board) pending
in rust-lang#12675, but that is awaiting the arrival of more data (to decide
whether to keep OptVec alive by using a non-Vec internally).

For this code, the representation of lifetime lists needs to be the
same in both ScopeChain and in the ast and ty structures.  So it
seemed cleanest to just use `vec_ng::Vec`, now that it has a cheaper
empty representation than the current `vec` code.
@huonw
Copy link
Member Author

huonw commented Mar 17, 2014

Mem benches have landed: http://huonw.github.io/isrustfastyet/mem/#910012a,0017056

So the doubly-fat-pointer Vec change made the compiler use 200 MB more memory when compiling librustc. I'll revive and reopen in a few days (maybe tomorrow).

@huonw huonw closed this Mar 17, 2014
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.

5 participants