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

Another rollup to cut down on the PR queue #8448

Closed
wants to merge 20 commits into from

Conversation

erickt
Copy link
Contributor

@erickt erickt commented Aug 11, 2013

This includes the following PRs:
#8408: r=aatch
#8412: r=alexcrichton
#8413: r=cmr
#8421: r=thestinger
#8423: r=bstrie
#8424: r=brson
#8427: r=thestinger

stepancheg and others added 20 commits August 9, 2013 02:19
`enum Token` was 192 bytes (64-bit), as pointed out by pnkfelix; the only
bloating variant being `INTERPOLATED(nonterminal)`.

Updating `enum nonterminal` to use ~ where variants included big types,
shrunk size_of(Token) to 32 bytes (64-bit).

I am unsure if the `nt_ident` variant should have an indirection, with
ast::ident being only 16 bytes (64-bit), but without this, enum Token
would be 40 bytes.

A dumb benchmark says that compilation time is unchanged, while peak
memory usage for compiling std.rs is down 3%

Before::

    $ time ./x86_64-unknown-linux-gnu/stage1/bin/rustc --cfg stage1 src/libstd/std.rs
    19.00user 0.39system 0:19.41elapsed 99%CPU (0avgtext+0avgdata 627820maxresident)k
    0inputs+28896outputs (0major+228665minor)pagefaults 0swaps
    $ time ./x86_64-unknown-linux-gnu/stage1/bin/rustc -O --cfg stage1 src/libstd/std.rs
    31.64user 0.34system 0:32.02elapsed 99%CPU (0avgtext+0avgdata 629876maxresident)k
    0inputs+22432outputs (0major+229411minor)pagefaults 0swaps

After::

    $ time ./x86_64-unknown-linux-gnu/stage1/bin/rustc --cfg stage1 src/libstd/std.rs
    19.07user 0.45system 0:19.55elapsed 99%CPU (0avgtext+0avgdata 609384maxresident)k
    0inputs+28896outputs (0major+221997minor)pagefaults 0swaps

    $ time ./x86_64-unknown-linux-gnu/stage1/bin/rustc -O --cfg stage1 src/libstd/std.rs
    31.90user 0.34system 0:32.28elapsed 99%CPU (0avgtext+0avgdata 612080maxresident)k
    0inputs+22432outputs (0major+223726minor)pagefaults 0swaps
This can be applied to statics and it will indicate that LLVM will attempt to
merge the constant in .data with other statics.

I have preliminarily applied this to all of the statics generated by the new
`ifmt!` syntax extension. I compiled a file with 1000 calls to `ifmt!` and a
separate file with 1000 calls to `fmt!` to compare the sizes, and the results
were:

fmt           310k
ifmt (before) 529k
ifmt (after)  202k

This now means that ifmt! is both faster and smaller than fmt!, yay!
A lot of people are hitting stack overflows in rustc. This will make it
easier to experiment with stack size.
before:

test add ... bench: 164 ns/iter (+/- 1)

after:

test add ... bench: 113 ns/iter (+/- 2)
before:

test bench_with_capacity ... bench: 104 ns/iter (+/- 4)

after:

test bench_with_capacity ... bench: 56 ns/iter (+/- 1)
…o rollup

Conflicts:
	src/libsyntax/ext/tt/macro_parser.rs
	src/libsyntax/parse/token.rs
@thestinger
Copy link
Contributor

@erickt: it wasn't what caused the failure in this PR, but my overflow checked operations had an issue on 32-bit I reported as #8449 - just going to try landing the fixed pull separately

@thestinger
Copy link
Contributor

Closing for now.

@thestinger thestinger closed this Aug 11, 2013
@kud1ing
Copy link

kud1ing commented Aug 11, 2013

I changed the error messages from expected Foo but found Bar to expected Foo, found Bar in my commit.
That fails in ifmt-bad-plural.rs and ifmt-bad-select.rs which were added in another commit.

@erickt
Copy link
Contributor Author

erickt commented Aug 11, 2013

@thestinger: thanks for closing! It was a noble effort, but I'm glad at least my first rollup got out.

@kud1ing: yeah I had a fix for that folded into my merge. Was that your PR? If so, have you updated it to work with HEAD?

@Centril Centril added the rollup A PR which is a rollup label Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rollup A PR which is a rollup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants