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.

Sign up for GitHub

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

Jump to bottom

Remove MemEncoder #111086

Merged
merged 3 commits into from
May 3, 2023
Merged

Remove MemEncoder #111086

merged 3 commits into from
May 3, 2023

Conversation

nnethercote
Copy link
Contributor

MemEncoder only has one non-test use, and FileEncoder would be more appropriate there anyway.

r? @cjgillot

@nnethercote
Change rlink serialization from MemEncoder to FileEncoder.
b4ba2f0 
Because we're writing to a file, so `FileEncoder` is better because we
don't have to write all the data to memory first.
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 2, 2023
@rustbot
Copy link
Collaborator

rustbot commented May 2, 2023

These commits modify the Cargo.lock file. Random changes to Cargo.lock can be introduced when switching branches and rebasing PRs.
This was probably unintentional and should be reverted before this PR is merged.

If this was intentional then you can ignore this comment.

@nnethercote
Copy link
Contributor Author

Best reviewed one commit at a time.

@nnethercote
Move some Encodable/Decodable tests.
8d359e4 
Round-trip encoding/decoding of many types is tested in
`compiler/rustc_serialize/tests/opaque.rs`. There is also a small amount
of encoding/decoding testing in three files in `tests/ui-fulldeps`.

There is no obvious reason why these three files are necessary. They
were originally added in 2014. Maybe it wasn't possible for a proc
macro to run in a unit test back then?

This commit just moves the testing from those three files into the unit
test.
@nnethercote
Remove MemEncoder.
ebee3f8 
It's only used in tests. Which is bad, because it means that
`FileEncoder` is used in the compiler but isn't used in tests!

`tests/opaque.rs` now tests encoding/decoding round-trips via file.
Because this is slower than memory, this commit also adjusts the
`u16`/`i16` tests so they are more like the `u32`/`i32` tests, i.e. they
don't test every possible value.
@cjgillot
Copy link
Contributor

cjgillot commented May 2, 2023

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 2, 2023
@bors
Copy link
Contributor

bors commented May 2, 2023

⌛ Trying commit ebee3f8 with merge 2633ff012f5590d16063be88ca90ebd1aa021e71...

@bors
Copy link
Contributor

bors commented May 2, 2023

☀️ Try build successful - checks-actions
Build commit: 2633ff012f5590d16063be88ca90ebd1aa021e71 (2633ff012f5590d16063be88ca90ebd1aa021e71)

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (2633ff012f5590d16063be88ca90ebd1aa021e71): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 2.9% [1.9%, 4.3%] 3
Improvements ✅
(primary)		 -3.5% [-3.5%, -3.5%] 1
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -3.5% [-3.5%, -3.5%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 656.873s -> 656.42s (-0.07%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 3, 2023
@nnethercote
Copy link
Contributor Author

Unsurprising that there is no perf effect, given that MemEncoder is currently only used for the rlink stuff.

@cjgillot
Copy link
Contributor

cjgillot commented May 3, 2023

@bors r+

@bors
Copy link
Contributor

bors commented May 3, 2023

📌 Commit ebee3f8 has been approved by cjgillot

It is now in the queue for this repository.

@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 May 3, 2023
@nnethercote
Copy link
Contributor Author

@bors rollup

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request May 3, 2023
@Dylan-DPC
Rollup merge of rust-lang#111086 - nnethercote:rm-MemEncoder, r=cjgillot
ae78f17 
Remove `MemEncoder`

`MemEncoder` only has one non-test use, and `FileEncoder` would be more appropriate there anyway.

r? `@cjgillot`
@Dylan-DPC Dylan-DPC mentioned this pull request May 3, 2023
Closed
@Dylan-DPC Dylan-DPC mentioned this pull request May 3, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 3, 2023
@bors
Auto merge of rust-lang#111153 - Dylan-DPC:rollup-0pq0hh3, r=Dylan-DPC
473f916 
Rollup of 11 pull requests

Successful merges:

 - rust-lang#107978 (Correctly convert an NT path to a Win32 path in `read_link`)
 - rust-lang#110436 (Support loading version information from xz tarballs)
 - rust-lang#110791 (Implement negative bounds for internal testing purposes)
 - rust-lang#110874 (Adjust obligation cause code for `find_and_report_unsatisfied_index_impl`)
 - rust-lang#110908 (resolve: One more attempt to simplify `module_children`)
 - rust-lang#110943 (interpret: fail more gracefully on uninit unsized locals)
 - rust-lang#111062 (Don't bail out early when checking invalid `repr` attr)
 - rust-lang#111069 (remove pointless `FIXME` in `bootstrap::download`)
 - rust-lang#111086 (Remove `MemEncoder`)
 - rust-lang#111097 (Avoid ICEing miri on layout query cycles)
 - rust-lang#111112 (Add some triagebot notifications for nnethercote.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 887dffc into rust-lang:master May 3, 2023
@rustbot rustbot added this to the 1.71.0 milestone May 3, 2023
@nnethercote nnethercote deleted the rm-MemEncoder branch May 4, 2023 00:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@cjgillot cjgillot

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.71.0
Development

Successfully merging this pull request may close these issues.
5 participants
@nnethercote @rustbot @cjgillot @rust-timer @bors