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

Refactor compression cache in v0 symbol mangler #87583

Merged
merged 3 commits into from
Jul 30, 2021

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Jul 29, 2021

  • Remove redundant option around compression caches (they are always present).
  • Flatten compression caches into symbol mangler to avoid dynamic memory allocation.
  • Implement printer for &mut SymbolMangler instead of SymbolMangler to avoid passing now slightly larger symbol mangler by value.

tmiasko added 3 commits July 29, 2021 13:28
Compression caches are always present. Remove unnecessary option.
The compression caches currently don't have any dedicated functionality
that would benefit from being separated. Incorporating caches directly
into the symbol manger also avoids dynamic memory allocation.

The symbol mangler, which is often passed by value, is now slightly
larger. This aspect will be addressed by a follow-up commit.
to avoid passing the symbol mangler by value.
@rust-highfive
Copy link
Collaborator

r? @wesleywiser

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 29, 2021
@wesleywiser
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jul 29, 2021

📌 Commit 0eabbf8 has been approved by wesleywiser

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 29, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 30, 2021
Rollup of 10 pull requests

Successful merges:

 - rust-lang#87052 (Optimize fmt::PadAdapter::wrap)
 - rust-lang#87522 (Fix assert in diy_float)
 - rust-lang#87553 (Fix typo in rustc_driver::version)
 - rust-lang#87554 (2229: Discr should be read when PatKind is Range)
 - rust-lang#87564 (min_type_alias_impl_trait is going to be removed in 1.56)
 - rust-lang#87574 (Update the examples in `String` and `VecDeque::retain`)
 - rust-lang#87583 (Refactor compression cache in v0 symbol mangler)
 - rust-lang#87585 (Add missing links for core::char types)
 - rust-lang#87594 (fs File get_path procfs usage for netbsd same as linux.)
 - rust-lang#87602 ([backtraces]: look for the `begin` symbol only after seeing `end`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 1757d6b into rust-lang:master Jul 30, 2021
@rustbot rustbot added this to the 1.56.0 milestone Jul 30, 2021
@tmiasko tmiasko deleted the compression-cache branch July 30, 2021 12:27

/// The length of the prefix in `out` (e.g. 2 for `_R`).
start_offset: usize,
/// The values are start positions in `out`, in bytes.
Copy link
Member

@eddyb eddyb Jul 30, 2021

Choose a reason for hiding this comment

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

Couple nitpicks (feel free to ignore I guess):

  • comment was not doc comment because it applies to all 3 fields
  • the names of the fields should've probably been adjusted (or the separate struct kept), since now they've lost a bit of context

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack. Will keep this in mind when making further changes there.

I was looking into the idea of using only shortening back-references, but differences seems to be negligible. For cargo, total symbol size reduction of -0.016% (nm target/debug/cargo | grep -o '_R.*' | wc --bytes). Could be more useful with const generics, I suppose.

@eddyb
Copy link
Member

eddyb commented Jul 30, 2021

Remove redundant option around compression caches (they are always present).

I wonder if I should've have made -Z symbol-mangling-version=v0-uncompressed access the None case.
Anyway, this was mostly for testing, I don't think we care much about uncompressed v0 going forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants