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

Use TLS less in rustc_span #73110

Closed
wants to merge 1 commit into from

Conversation

Aaron1011
Copy link
Member

Following the approach suggested in
#59718 (comment),
we store rustc_span::Globals in ParseSess. This allows us to avoid
TLS lookups during Span/SyntaxContext/ExpnData hashing, since we can get
access to ParseSess via HashStableContext

@rust-highfive
Copy link
Collaborator

r? @davidtwco

(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 Jun 8, 2020
@Aaron1011
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Jun 8, 2020

⌛ Trying commit ef67fa1673fb7bec5a74d473e81fad7a51c0ad77 with merge 38f031dba70e2d2ebd0484ffc5d7a659ab15d245...

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-8 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
##[section]Starting: Linux x86_64-gnu-llvm-8
##[section]Starting: Initialize job
Agent name: 'Azure Pipelines 2'
Agent machine name: 'fv-az578'
Current agent version: '2.169.1'
##[group]Operating System
16.04.6
LTS
LTS
##[endgroup]
##[group]Virtual Environment
Environment: ubuntu-16.04
Version: 20200517.1
Included Software: https://github.com/actions/virtual-environments/blob/ubuntu16/20200517.1/images/linux/Ubuntu1604-README.md
##[endgroup]
Agent running as: 'vsts'
Prepare build directory.
Set build variables.
Download all required tasks.
Download all required tasks.
Downloading task: Bash (3.163.3)
Checking job knob settings.
   Knob: AgentToolsDirectory = /opt/hostedtoolcache Source: ${AGENT_TOOLSDIRECTORY} 
   Knob: AgentPerflog = /home/vsts/perflog Source: ${VSTS_AGENT_PERFLOG} 
Start tracking orphan processes.
##[section]Finishing: Initialize job
##[section]Starting: Configure Job Name
==============================================================================
---
========================== Starting Command Output ===========================
[command]/bin/bash --noprofile --norc /home/vsts/work/_temp/26ae8ebb-b90d-4e9e-87fb-47447d3dbbba.sh

##[section]Finishing: Disable git automatic line ending conversion
##[section]Starting: Checkout rust-lang/rust@refs/pull/73110/merge to s
Task         : Get sources
Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
Version      : 1.0.0
Author       : Microsoft
---
##[command]git remote add origin https://github.com/rust-lang/rust
##[command]git config gc.auto 0
##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
##[command]git config --get-all http.proxy
##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/73110/merge:refs/remotes/pull/73110/merge
---
 ---> cb2676f08729
Step 5/8 : ENV RUST_CONFIGURE_ARGS       --build=x86_64-unknown-linux-gnu       --llvm-root=/usr/lib/llvm-8       --enable-llvm-link-shared       --set rust.thin-lto-import-instr-limit=10
 ---> Using cache
 ---> df25ce111862
Step 6/8 : ENV SCRIPT python2.7 ../x.py test --exclude src/tools/tidy &&            python2.7 ../x.py test src/test/mir-opt --pass=build                                   --target=armv5te-unknown-linux-gnueabi &&            python2.7 ../x.py test src/tools/tidy
 ---> 599b9ac96b27
Step 7/8 : ENV NO_DEBUG_ASSERTIONS=1
 ---> Using cache
 ---> 091087e35a36
---
   Compiling rustc_parse_format v0.0.0 (/checkout/src/librustc_parse_format)
   Compiling chalk-rust-ir v0.10.0
   Compiling rustc_ast_pretty v0.0.0 (/checkout/src/librustc_ast_pretty)
   Compiling rustc_hir v0.0.0 (/checkout/src/librustc_hir)
   Compiling rustc_query_system v0.0.0 (/checkout/src/librustc_query_system)
   Compiling chalk-solve v0.10.0
   Compiling rustc_hir_pretty v0.0.0 (/checkout/src/librustc_hir_pretty)
   Compiling rustc_parse v0.0.0 (/checkout/src/librustc_parse)
   Compiling rustc_ast_lowering v0.0.0 (/checkout/src/librustc_ast_lowering)
---
   Compiling rustc_parse_format v0.0.0 (/checkout/src/librustc_parse_format)
   Compiling chalk-rust-ir v0.10.0
   Compiling rustc_ast_pretty v0.0.0 (/checkout/src/librustc_ast_pretty)
   Compiling rustc_hir v0.0.0 (/checkout/src/librustc_hir)
   Compiling rustc_query_system v0.0.0 (/checkout/src/librustc_query_system)
   Compiling chalk-solve v0.10.0
   Compiling rustc_hir_pretty v0.0.0 (/checkout/src/librustc_hir_pretty)
   Compiling rustc_parse v0.0.0 (/checkout/src/librustc_parse)
   Compiling rustc_ast_lowering v0.0.0 (/checkout/src/librustc_ast_lowering)
---
............................i...............i....................................................... 5200/10285
.................................................................................................... 5300/10285
............................................................................i....................... 5400/10285
......................................................................i............................. 5500/10285
.......................................................................................ii.ii........ 5600/10285
i...i............................................................................................... 5700/10285
......................................i............................................................. 5900/10285
............................................................................................ii...... 6000/10285
...............................i.................................................................... 6100/10285
.................................................................................................... 6200/10285
.................................................................................................... 6200/10285
.................................................................................................... 6300/10285
......................................................ii...i..ii...........i........................ 6400/10285
.................................................................................................... 6600/10285
.................................................................................................... 6700/10285
.................................................................................................... 6700/10285
.......................................................................................i..ii........ 6800/10285
.................................................................................................... 7000/10285
.................................................................................................... 7100/10285
.........................................i.......................................................... 7200/10285
.................................................................................................... 7300/10285
---
.................................................................................................... 8200/10285
.................................................................................................... 8300/10285
................................................................................i................... 8400/10285
.................................................................................................... 8500/10285
..................................iiiiii.iiiiii.i................................................... 8600/10285
.................................................................................................... 8800/10285
.................................................................................................... 8900/10285
.................................................................................................... 9000/10285
.................................................................................................... 9100/10285
---
Suite("src/test/codegen") not skipped for "bootstrap::test::Codegen" -- not in ["src/tools/tidy"]
Check compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)

running 193 tests
iiii......i..............ii.i..........i......................i...........i..i................i....i 100/193
.............i.i.i...iii..iiiiiiiiiiiiiiiii.......................iii................ii......

 finished in 5.452
Suite("src/test/codegen-units") not skipped for "bootstrap::test::CodegenUnits" -- not in ["src/tools/tidy"]
Check compiletest suite=codegen-units mode=codegen-units (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
---
Suite("src/test/assembly") not skipped for "bootstrap::test::Assembly" -- not in ["src/tools/tidy"]
Check compiletest suite=assembly mode=assembly (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)

running 21 tests
iiiiiiiiiiiiiiiiiiiii

 finished in 0.151
Suite("src/test/incremental") not skipped for "bootstrap::test::Incremental" -- not in ["src/tools/tidy"]
Check compiletest suite=incremental mode=incremental (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
---
Suite("src/test/debuginfo") not skipped for "bootstrap::test::Debuginfo" -- not in ["src/tools/tidy"]
Check compiletest suite=debuginfo mode=debuginfo (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)

running 115 tests
iiiii..i.....i..i...i..i.i.i..i..i..ii....i.i....ii..........iiii.........i.....i..i.......ii.i.ii.. 100/115
...iiii.....ii.

 finished in 14.907
Suite("src/test/ui-fulldeps") not skipped for "bootstrap::test::UiFullDeps" -- not in ["src/tools/tidy"]
Uplifting stage1 rustc (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
---
   Compiling rustc_errors v0.0.0 (/checkout/src/librustc_errors)
error[E0308]: mismatched types
  --> src/librustc_errors/json/tests.rs:44:29
   |
44 |     rustc_span::GLOBALS.set(&globals, || rustc_span::GLOBALS.set(&globals, f))
   |                             ^^^^^^^^ expected struct `std::rc::Rc`, found struct `rustc_span::Globals`
   = note: expected reference `&std::rc::Rc<rustc_span::Globals>`
              found reference `&rustc_span::Globals`

error[E0308]: mismatched types
error[E0308]: mismatched types
  --> src/librustc_errors/json/tests.rs:44:66
   |
44 |     rustc_span::GLOBALS.set(&globals, || rustc_span::GLOBALS.set(&globals, f))
   |                                                                  ^^^^^^^^ expected struct `std::rc::Rc`, found struct `rustc_span::Globals`
   = note: expected reference `&std::rc::Rc<rustc_span::Globals>`
              found reference `&rustc_span::Globals`

error: aborting due to 2 previous errors
error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0308`.
error: could not compile `rustc_errors`.

To learn more, run the command again with --verbose.


command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "2" "--release" "--locked" "--color" "always" "--features" " llvm" "--manifest-path" "/checkout/src/rustc/Cargo.toml" "-p" "rustc_errors" "--" "--quiet"


failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test --exclude src/tools/tidy
Build completed unsuccessfully in 1:13:56
Build completed unsuccessfully in 1:13:56
== clock drift check ==
  local time: Mon Jun  8 01:44:24 UTC 2020
  network time: Mon, 08 Jun 2020 01:44:24 GMT
== end clock drift check ==

##[error]Bash exited with code '1'.
##[section]Finishing: Run build
##[section]Starting: Checkout rust-lang/rust@refs/pull/73110/merge to s
Task         : Get sources
Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
Version      : 1.0.0
Author       : Microsoft
Author       : Microsoft
Help         : [More Information](https://go.microsoft.com/fwlink/?LinkId=798199)
==============================================================================
Cleaning any cached credential from repository: rust-lang/rust (GitHub)
##[section]Finishing: Checkout rust-lang/rust@refs/pull/73110/merge to s
Cleaning up task key
Start cleaning up orphan processes.
Terminate orphan process: pid (4179) (python)
##[section]Finishing: Finalize Job

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

@bors
Copy link
Contributor

bors commented Jun 8, 2020

☀️ Try build successful - checks-azure
Build commit: 38f031dba70e2d2ebd0484ffc5d7a659ab15d245 (38f031dba70e2d2ebd0484ffc5d7a659ab15d245)

@rust-timer
Copy link
Collaborator

Queued 38f031dba70e2d2ebd0484ffc5d7a659ab15d245 with parent 0262de5, future comparison URL.

@petrochenkov petrochenkov self-assigned this Jun 8, 2020
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (38f031dba70e2d2ebd0484ffc5d7a659ab15d245): comparison url.

@petrochenkov
Copy link
Contributor

Looks like noise mostly.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 8, 2020
@Aaron1011
Copy link
Member Author

This had a larger impact locally - I suspect that having debug assertions on may have interfered with some optimizations that would have otherwise happened.

@davidtwco
Copy link
Member

r? @petrochenkov

@Aaron1011
Copy link
Member Author

I'll re-evaluate this PR after #72121 lands

@Aaron1011 Aaron1011 added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 24, 2020
@bjorn3
Copy link
Member

bjorn3 commented Aug 2, 2020

#72121 has landed.

@Aaron1011
Copy link
Member Author

I'm going to wait for #74932 before revisiting this.

@petrochenkov
Copy link
Contributor

#74932 has landed.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Aug 8, 2020
@Aaron1011
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Aug 8, 2020

⌛ Trying commit fddd771 with merge 5ab61fc94ec843fcd588f426711c86c4d42f17ce...

@bors
Copy link
Contributor

bors commented Aug 8, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 5ab61fc94ec843fcd588f426711c86c4d42f17ce (5ab61fc94ec843fcd588f426711c86c4d42f17ce)

@rust-timer
Copy link
Collaborator

Queued 5ab61fc94ec843fcd588f426711c86c4d42f17ce with parent c989ac1, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (5ab61fc94ec843fcd588f426711c86c4d42f17ce): 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

@bjorn3
Copy link
Member

bjorn3 commented Aug 8, 2020

Up to 0.4% performance hit. Most is <0.2%

@Aaron1011
Copy link
Member Author

Aaron1011 commented Aug 8, 2020

Closing this, as the basic premise (it's faster to access SessionGlobals via a Session instead of TLS) doesn't seem to be true.

@Aaron1011 Aaron1011 closed this Aug 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants