Skip to content

Conversation

brson
Copy link
Contributor

@brson brson commented Aug 25, 2013

This does two things: 1) stops compressing metadata, 2) stops copying the metadata section, instead holding a reference to the buffer returned by the LLVM section iterator.

Not compressing metadata requires something like 7x the storage space, but makes running tests about 9% faster. This has been a time improvement on all platforms I've tested, including windows. I considered leaving compression as an option but it doesn't seem to be worth the complexity since we don't currently have any use cases where we need to save that space.

In order to avoid copying the metadata section I had to hack up extra::ebml a bit to support unsafe buffers. We should probably move it into librustc so that it can evolve to support the compiler without worrying about having a crummy interface.

r? @graydon

@emberian
Copy link
Contributor

So this should cut off ~3 minutes from cycle time? Also:

we don't currently have any use cases where we need to save that space.

Not true. The Ubuntu PPA and the Arch repo both have nightlies, bloating that by 7x would annoy at least me, they're already ~17MB.

@thestinger
Copy link
Contributor

@cmr: I wouldn't be surprised if the Arch package got smaller with this compression off, because xz will do a better job.

@brson
Copy link
Contributor Author

brson commented Aug 25, 2013

@cmr Yes, 3 minutes sounds about right. As @thestinger noted when we distribute binaries they tend to be compressed independently.

Additionally it's been suggested that in cases where we need to save that space we probably want to strip the metadata from the binaries completely.

bors added a commit that referenced this pull request Aug 25, 2013
This does two things: 1) stops compressing metadata, 2) stops copying the metadata section, instead holding a reference to the buffer returned by the LLVM section iterator.

Not compressing metadata requires something like 7x the storage space, but makes running tests about 9% faster. This has been a time improvement on all platforms I've tested, including windows. I considered leaving compression as an option but it doesn't seem to be worth the complexity since we don't currently have any use cases where we need to save that space.

In order to avoid copying the metadata section I had to hack up extra::ebml a bit to support unsafe buffers. We should probably move it into librustc so that it can evolve to support the compiler without worrying about having a crummy interface.

r? @graydon
@bors bors closed this Aug 25, 2013
brson added a commit to brson/rust that referenced this pull request Aug 26, 2013
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jun 4, 2022
…tiple, r=Manishearth

Fix `manual_range_contains` false negative with chains of `&&` and `||`

Fixes rust-lang#8745

Since the precedence for `&&` is the same as itself the HIR for a chain of `&&` ends up with a right skewed tree like:

```
     &&
    /  \
  &&   c2
 /  \
... c1
```

So only the leftmost `&&` was actually "fully" checked, the top level was just `c2` and `&&` so the `manual_range_contains` lint won't apply. This change makes it also check `c2` with `c1`.

There's a bit of a hacky solution in the [second commit](rust-lang/rust-clippy@257f097) to check if the number of open/closing parens in the snippet match. This is to prevent a case like `((x % 2 == 0) || (x < 0)) || (x >= 10)` from offering a suggestion like `((x % 2 == 0) || !(0..10).contains(&x)` which now won't compile.

Any suggestions for that paren hack welcome, kinda new to working on this so not too sure about possible solutions :) it's weird because I don't know how else to check for parens in HIR considering they're removed when lowering AST.

changelog: Fix [`manual_range_contains`] false negative with chains of `&&` and `||`
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.

4 participants