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

Precompute ancestors when checking privacy #81574

Merged
merged 1 commit into from
Feb 18, 2021
Merged

Precompute ancestors when checking privacy #81574

merged 1 commit into from
Feb 18, 2021

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Jan 30, 2021

Precompute ancestors of the old error node set so that check for private
types and traits in public interfaces can in constant time determine if
the current item has any descendants in the old error set.

This removes disparity in compilation time between public and private type
aliases reported in #50614 (from 30 s to 5 s, in an example making extensive use
of private type aliases).

No functional changes intended.

Precompute ancestors of the old error node set so that check for private
types and traits in public interfaces can in constant time determine if
the current item has any descendants in the old error set.

No functional changes intended.
@rust-highfive
Copy link
Collaborator

r? @oli-obk

(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 Jan 30, 2021
@tmiasko
Copy link
Contributor Author

tmiasko commented Jan 30, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@bors
Copy link
Contributor

bors commented Jan 30, 2021

⌛ Trying commit b01976a with merge 1e3fee80c53fecef512bac75b0b96f4e90484bbe...

@bors
Copy link
Contributor

bors commented Jan 30, 2021

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

@rust-timer
Copy link
Collaborator

Queued 1e3fee80c53fecef512bac75b0b96f4e90484bbe with parent fd20a8b, future comparison URL.

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

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 30, 2021
@camelid camelid added the A-visibility Area: Visibility / privacy label Jan 30, 2021
@rust-timer
Copy link
Collaborator

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

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

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

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 31, 2021
@tmiasko
Copy link
Contributor Author

tmiasko commented Jan 31, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@bors
Copy link
Contributor

bors commented Jan 31, 2021

⌛ Trying commit 712bb1c15a6a987c4feb8d4f64a6f573c33c9e31 with merge 7aa7d821ffc49849236e46d009860941d8106fac...

@bors
Copy link
Contributor

bors commented Jan 31, 2021

☀️ Try build successful - checks-actions
Build commit: 7aa7d821ffc49849236e46d009860941d8106fac (7aa7d821ffc49849236e46d009860941d8106fac)

@rust-timer
Copy link
Collaborator

Queued 7aa7d821ffc49849236e46d009860941d8106fac with parent 04caa63, future comparison URL.

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

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 31, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (7aa7d821ffc49849236e46d009860941d8106fac): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

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

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 31, 2021
@petrochenkov petrochenkov self-assigned this Jan 31, 2021
@tmiasko
Copy link
Contributor Author

tmiasko commented Jan 31, 2021

There was an extra commit that attempted an additional micro optimization, but it is far from clear if it has any benefits so I removed it.

There is a large regression for deeply-nested-async. The old error set is empty for this benchmark. Using artifiacts from CI, the regression is reproducible but instruction count differences are in unrelated area of privacy checking. The cause appears to be different codegen partitioning of rustc-privacy as far as I can see.

@petrochenkov petrochenkov removed their assignment Feb 2, 2021
@oli-obk
Copy link
Contributor

oli-obk commented Feb 15, 2021

The regression can potentially be fixed by adding some targeted #[inline] attributes, but considering that is a 0.005s regression when the improvements are in the 0.2ms region without any increase in code complexity let's merge this

@bors r+

@bors
Copy link
Contributor

bors commented Feb 15, 2021

📌 Commit b01976a has been approved by oli-obk

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 15, 2021
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Feb 15, 2021
@bors
Copy link
Contributor

bors commented Feb 15, 2021

⌛ Testing commit b01976a with merge 3e1d74b413edb132708fb24af1ed3ddc29e9dfcc...

@bors
Copy link
Contributor

bors commented Feb 16, 2021

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 16, 2021
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@oli-obk
Copy link
Contributor

oli-obk commented Feb 17, 2021

@bors retry

@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 Feb 17, 2021
@tmiasko
Copy link
Contributor Author

tmiasko commented Feb 17, 2021

Opened #82221 to track test hang which caused time out. It seems to be happening quite a bit, although I haven't encountered one locally.

@bors
Copy link
Contributor

bors commented Feb 18, 2021

⌛ Testing commit b01976a with merge cb2effd...

@bors
Copy link
Contributor

bors commented Feb 18, 2021

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing cb2effd to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 18, 2021
@bors bors merged commit cb2effd into rust-lang:master Feb 18, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 18, 2021
@tmiasko tmiasko deleted the p branch February 18, 2021 13:05
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
  CACHE_DOMAIN: ci-caches.rust-lang.org
  TOOLSTATE_REPO_ACCESS_TOKEN: ***
##[endgroup]
Cloning into 'rust-toolstate'...
/home/runner/work/rust/rust/src/tools/publish_toolstate.py:121: DeprecationWarning: 'U' mode is deprecated
📣 Toolstate changed by rust-lang/rust#81574!
  with open(path, 'rU') as f:

Tested on commit rust-lang/rust@cb2effd44e667d133e31ef334e30d10195218ce6.
Direct link to PR: <https://github.com/rust-lang/rust/pull/81574>

💔 miri on windows: test-fail → build-fail (cc @RalfJung @eddyb @oli-obk).
💔 miri on linux: test-fail → build-fail (cc @RalfJung @eddyb @oli-obk).
Traceback (most recent call last):
Traceback (most recent call last):
  File "/home/runner/work/rust/rust/src/tools/publish_toolstate.py", line 338, in <module>
    response = urllib2.urlopen(urllib2.Request(
  File "/usr/lib/python3.8/urllib/request.py", line 222, in urlopen
    return opener.open(url, data, timeout)
  File "/usr/lib/python3.8/urllib/request.py", line 522, in open
    req = meth(req)
  File "/usr/lib/python3.8/urllib/request.py", line 1281, in do_request_
    raise TypeError(msg)
TypeError: POST data should be bytes, an iterable of bytes, or a file object. It cannot be of type str.
##[error]Process completed with exit code 1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-visibility Area: Visibility / privacy merged-by-bors This PR was explicitly merged by bors. 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.

9 participants