Skip to content

Remove redundant Span in QueryJobInfo #88567

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Merged
merged 1 commit into from
Sep 2, 2021
Merged

Remove redundant Span in QueryJobInfo #88567

merged 1 commit into from
Sep 2, 2021

Conversation

camelid
Copy link
Member

@camelid camelid commented Sep 1, 2021

Previously, QueryJobInfo was composed of two parts: a QueryInfo and
a QueryJob. However, both QueryInfo and QueryJob have a span
field, which seem to be the same. So, the span was recorded twice.

Now, QueryJobInfo is composed of a QueryStackFrame (the other field
of QueryInfo) and a QueryJob. So, now, the span is only recorded
once.

@camelid
Remove redundant Span in QueryJobInfo
4553a4b 
Previously, `QueryJobInfo` was composed of two parts: a `QueryInfo` and
a `QueryJob`. However, both `QueryInfo` and `QueryJob` have a `span`
field, which seem to be the same. So, the `span` was recorded twice.

Now, `QueryJobInfo` is composed of a `QueryStackFrame` (the other field
of `QueryInfo`) and a `QueryJob`. So, now, the `span` is only recorded
once.
@camelid camelid added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 1, 2021
@rust-highfive
Copy link
Contributor

r? @matthewjasper

(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 Sep 1, 2021
@camelid
Copy link
Member Author

camelid commented Sep 1, 2021

r? @cjgillot

This may have a small improvement of compile-time memory usage, but it's likely to be quite small. Should I still mark this as rollup=never?

@cjgillot
Copy link
Contributor

cjgillot commented Sep 1, 2021

The query infra is absurdly sensitive.
@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 1, 2021
@bors
Copy link
Collaborator

bors commented Sep 1, 2021

⌛ Trying commit 4553a4b with merge 1cfbf261628e807ec0ad3d020b373adc5494b811...

@camelid camelid added the I-compilemem Issue: Problems and improvements with respect to memory usage during compilation. label Sep 1, 2021
@bors
Copy link
Collaborator

bors commented Sep 1, 2021

☀️ Try build successful - checks-actions
Build commit: 1cfbf261628e807ec0ad3d020b373adc5494b811 (1cfbf261628e807ec0ad3d020b373adc5494b811)

@rust-timer
Copy link
Collaborator

Queued 1cfbf261628e807ec0ad3d020b373adc5494b811 with parent ad3407f, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (1cfbf261628e807ec0ad3d020b373adc5494b811): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up.

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

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 1, 2021
@camelid
Copy link
Member Author

camelid commented Sep 1, 2021 •
edited
Loading

I'm not sure why it said there were no significant changes; instruction count is down by ~0.2% on several benchmarks; the only significant regression (according to the perf.rlo webpage) is 0.4% for deep-vector-debug, but IIRC that benchmark is somewhat noisy.

@camelid camelid added the I-compiletime Issue: Problems and improvements with respect to compile times. label Sep 1, 2021
@jyn514
Copy link
Member

jyn514 commented Sep 2, 2021

This greatly increased max-rss for tuple-stress-doc for some reason. Not sure if it's spurious or not.

@cjgillot
Copy link
Contributor

cjgillot commented Sep 2, 2021

I'm not sure why it said there were no significant changes; instruction count is down by ~0.2% on several benchmarks; the only significant regression (according to the perf.rlo webpage) is 0.4% for deep-vector-debug, but IIRC that benchmark is somewhat noisy.

<1% changes are not what I call significant :)

This greatly increased max-rss for tuple-stress-doc for some reason. Not sure if it's spurious or not.

It's spurious, those code path are only used in incremental runs, and the regressed run is not incremental.

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Sep 2, 2021

📌 Commit 4553a4b has been approved by cjgillot

@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 Sep 2, 2021
@m-ou-se m-ou-se mentioned this pull request Sep 2, 2021
Merged
@camelid
Copy link
Member Author

camelid commented Sep 2, 2021

I'm not sure why it said there were no significant changes; instruction count is down by ~0.2% on several benchmarks; the only significant regression (according to the perf.rlo webpage) is 0.4% for deep-vector-debug, but IIRC that benchmark is somewhat noisy.

<1% changes are not what I call significant :)

Well, of course it's not a large improvement, but the web interface shows those changes as "relevant" which I would think would be similar to "significant" :)

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 2, 2021
@bors
Auto merge of rust-lang#88596 - m-ou-se:rollup-cidzt4v, r=m-ou-se
b834c4c 
Rollup of 12 pull requests

Successful merges:

 - rust-lang#88177 (Stabilize std::os::unix::fs::chroot)
 - rust-lang#88505 (Use `unwrap_unchecked` where possible)
 - rust-lang#88512 (Upgrade array_into_iter lint to include Deref-to-array types.)
 - rust-lang#88532 (Remove single use variables)
 - rust-lang#88543 (Improve closure dummy capture suggestion in macros.)
 - rust-lang#88560 (`fmt::Formatter::pad`: don't call chars().count() more than one time)
 - rust-lang#88565 (Add regression test for issue 83190)
 - rust-lang#88567 (Remove redundant `Span` in `QueryJobInfo`)
 - rust-lang#88573 (rustdoc: Don't panic on ambiguous inherent associated types)
 - rust-lang#88582 (Implement rust-lang#88581)
 - rust-lang#88589 (Correct doc comments inside `use_expr_visitor.rs`)
 - rust-lang#88592 (Fix ICE in const check)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f419334 into rust-lang:master Sep 2, 2021
@rustbot rustbot added this to the 1.56.0 milestone Sep 2, 2021
@camelid camelid deleted the query-job-info branch September 2, 2021 21:32
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers
No reviews
Assignees

@cjgillot cjgillot

Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) C-cleanup Category: PRs that clean code up or issues documenting cleanup. I-compilemem Issue: Problems and improvements with respect to memory usage during compilation. I-compiletime Issue: Problems and improvements with respect to compile times. 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.56.0
Development

Successfully merging this pull request may close these issues.
8 participants
@camelid @rust-highfive @cjgillot @rust-timer @bors @jyn514 @matthewjasper @rustbot