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

[DO NOT MERGE] syntax: try ref-counting the AST. #58482

Closed
wants to merge 1 commit into from

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Feb 15, 2019

This an exploratory change, mainly to see what the costs around manipulating ASTs are.
Trying out an arena would be harder, as lifetimes would have to be added everywhere.
Also, AFAIK we don't have a "memory/time graph" of any sort, so it might be hard to profile.

cc @michaelwoerister @Mark-Simulacrum @Zoxc

@rust-highfive
Copy link
Collaborator

r? @michaelwoerister

(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 Feb 15, 2019
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (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.
travis_time:end:01335518:start=1550227402203888641,finish=1550227403088153556,duration=884264915
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
[00:08:09]    Compiling tempfile v3.0.5
[00:08:11]    Compiling arena v0.0.0 (/checkout/src/libarena)
[00:08:12]    Compiling syntax_pos v0.0.0 (/checkout/src/libsyntax_pos)
[00:08:16]    Compiling rustc_errors v0.0.0 (/checkout/src/librustc_errors)
[00:08:35] error: internal compiler error: src/librustc/hir/def.rs:257: attempted .def_id() on invalid def: NonMacroAttr(Builtin)
[00:08:35] thread 'rustc' panicked at 'Box<Any>', src/librustc_errors/lib.rs:588:9
[00:08:35] note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
[00:08:35] error: aborting due to previous error
[00:08:35] 
[00:08:35] 
[00:08:35] 
[00:08:35] note: the compiler unexpectedly panicked. this is a bug.
[00:08:35] 
[00:08:35] note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
[00:08:35] 
[00:08:35] note: rustc 1.33.0-beta.1 (d1add9723 2019-01-17) running on x86_64-unknown-linux-gnu
[00:08:35] 
[00:08:35] note: compiler flags: -Z force-unstable-if-unmarked -C prefer-dynamic -C opt-level=2 -C prefer-dynamic -C debug-assertions=y -C link-args=-Wl,-rpath,$ORIGIN/../lib --crate-type dylib
[00:08:35] note: some of the compiler flags provided by cargo are hidden
[00:08:35] 
[00:08:35] error: Could not compile `syntax`.
[00:08:35] 
[00:08:35] 
[00:08:35] To learn more, run the command again with --verbose.
[00:08:35] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "" "--manifest-path" "/checkout/src/rustc/Cargo.toml" "--message-format" "json"
[00:08:35] expected success, got: exit code: 101
[00:08:35] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:08:35] Build completed unsuccessfully in 0:02:24
[00:08:35] make: *** [all] Error 1
[00:08:35] Makefile:18: recipe for target 'all' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:09e651dc
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Fri Feb 15 10:52:09 UTC 2019
---
199368 ./obj/build/cache/2019-01-18
156148 ./src/llvm-project/clang
155976 ./obj/build/bootstrap/debug/incremental
141204 ./obj/build/bootstrap/debug/incremental/bootstrap-2ahv8almm435e
141200 ./obj/build/bootstrap/debug/incremental/bootstrap-2ahv8almm435e/s-f9icrrc4d9-yihg6n-2ee07hmknei1u
108528 ./src/llvm-project/lldb
97552 ./src/llvm-project/clang/test
93688 ./.git
89964 ./src/llvm-emscripten/test/CodeGen

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 @TimNN. (Feature Requests)

@petrochenkov
Copy link
Contributor

@eddyb
Is proceeding with #57662 (comment) something that was discussed / decided on all hands?
When is the transition supposed to happen? Very soon / in the next months / this year?

@eddyb
Copy link
Member Author

eddyb commented Feb 15, 2019

@petrochenkov It's still only a proposal from me at this point.
We'll have to discuss it further if we are to go ahead with it.

But if I'm to work on IDE-oriented incremental this year I'm not seeing many alternatives.

@eddyb
Copy link
Member Author

eddyb commented Feb 15, 2019

I don't understand the failure, it appears to be an ICE during stage0.

@bors try

@bors
Copy link
Contributor

bors commented Feb 15, 2019

⌛ Trying commit d4de6fd453be44a58c67efe0a1c79fccb4f8f997 with merge bf318f6acedd85b42bf991d650ae7f693a551e1a...

@bors
Copy link
Contributor

bors commented Feb 15, 2019

💔 Test failed - checks-travis

@rust-highfive
Copy link
Collaborator

The job dist-x86_64-linux of your PR failed on Travis (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.
travis_fold:end:services

travis_fold:start:git.checkout
travis_time:start:30effc33
$ git clone --depth=2 --branch=try https://github.com/rust-lang/rust.git rust-lang/rust
---
[00:07:19]    Compiling tempfile v3.0.5
[00:07:20]    Compiling arena v0.0.0 (/checkout/src/libarena)
[00:07:21]    Compiling syntax_pos v0.0.0 (/checkout/src/libsyntax_pos)
[00:07:38]    Compiling rustc_errors v0.0.0 (/checkout/src/librustc_errors)
[00:07:55] error: internal compiler error: src/librustc/hir/def.rs:257: attempted .def_id() on invalid def: NonMacroAttr(Builtin)
[00:07:55] thread 'rustc' panicked at 'Box<Any>', src/librustc_errors/lib.rs:588:9
[00:07:55] note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
[00:07:55] error: aborting due to previous error
[00:07:55] 
[00:07:55] 
[00:07:55] 
[00:07:55] note: the compiler unexpectedly panicked. this is a bug.
[00:07:55] 
[00:07:55] note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
[00:07:55] 
[00:07:55] note: rustc 1.33.0-beta.1 (d1add9723 2019-01-17) running on x86_64-unknown-linux-gnu
[00:07:55] 
[00:07:55] note: compiler flags: -Z force-unstable-if-unmarked -C prefer-dynamic -C opt-level=2 -C linker=clang -C prefer-dynamic -C linker=clang -C debug-assertions=n -C link-args=-Wl,-rpath,$ORIGIN/../lib --crate-type dylib
[00:07:55] note: some of the compiler flags provided by cargo are hidden
[00:07:55] 
[00:07:55] error: Could not compile `syntax`.
[00:07:55] 
---
travis_time:end:022d48a0:start=1550231809808669205,finish=1550231809816914709,duration=8245504
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:01453848
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:3d98a0d4
travis_time:start:3d98a0d4
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:086d9cd9
$ dmesg | grep -i kill

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 @TimNN. (Feature Requests)

@bors bors 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 Feb 15, 2019
@eddyb
Copy link
Member Author

eddyb commented Feb 15, 2019

Oh it happens during compiling syntax. I guess this needs further investigation.

@Zoxc
Copy link
Contributor

Zoxc commented Feb 15, 2019

I got these errors when building locally:

error[E0599]: no method named `clone` found for type `T` in the current scope9: syntax
  --> src\libsyntax\ptr.rs:63:63
   |
63 |         Lrc::try_unwrap(self.ptr).unwrap_or_else(|ptr| (*ptr).clone())
   |                                                               ^^^^^
   |
   = help: items from traits can only be used if the trait is implemented and in scope
   = note: the following traits define an item `clone`, perhaps you need to implement one of them:
           candidate #1: `std::clone::Clone`
           candidate #2: `proc_macro::bridge::server::TokenStream`
           candidate #3: `proc_macro::bridge::server::TokenStreamIter`
           candidate #4: `proc_macro::bridge::server::Group`
           candidate #5: `proc_macro::bridge::server::Literal`
           candidate #6: `proc_macro::bridge::server::SourceFile`

error[E0277]: the trait bound `T: std::clone::Clone` is not satisfied
  --> src\libsyntax\ptr.rs:71:25
   |
71 |         let p: *mut T = Lrc::make_mut(&mut self.ptr);
   |                         ^^^^^^^^^^^^^ the trait `std::clone::Clone` is not implemented for `T`
   |
   = help: consider adding a `where T: std::clone::Clone` bound
   = note: required by `<std::rc::Rc<T>>::make_mut`

error[E0277]: the trait bound `T: std::clone::Clone` is not satisfied
  --> src\libsyntax\ptr.rs:92:25
   |
92 |         let p: *mut T = Lrc::make_mut(&mut self.ptr);
   |                         ^^^^^^^^^^^^^ the trait `std::clone::Clone` is not implemented for `T`
   |
   = help: consider adding a `where T: std::clone::Clone` bound
   = note: required by `<std::rc::Rc<T>>::make_mut`

error[E0277]: the trait bound `T: std::clone::Clone` is not satisfied
   --> src\libsyntax\ptr.rs:124:9
    |
124 |         Lrc::make_mut(&mut self.ptr)
    |         ^^^^^^^^^^^^^ the trait `std::clone::Clone` is not implemented for `T`
    |
    = help: consider adding a `where T: std::clone::Clone` bound
    = note: required by `<std::rc::Rc<T>>::make_mut`

error[E0277]: the trait bound `[T]: std::default::Default` is not satisfied
   --> src\libsyntax\ptr.rs:166:18
    |
166 |         P { ptr: Default::default() }
    |                  ^^^^^^^^^^^^^^^^ the trait `std::default::Default` is not implemented for `[T]`
    |
    = help: the following implementations were found:
              <[T; 24] as std::default::Default>
              <[T; 28] as std::default::Default>
              <[T; 32] as std::default::Default>
              <[T; 6] as std::default::Default>
            and 31 others
    = note: required because of the requirements on the impl of `std::default::Default` for `std::rc::Rc<[T]>`
    = note: required by `std::default::Default::default`

error[E0308]: mismatched types
   --> src\libsyntax\ptr.rs:171:18
    |
171 |         P { ptr: v.into_boxed_slice() }
    |                  ^^^^^^^^^^^^^^^^^^^^ expected struct `std::rc::Rc`, found struct `std::boxed::Box`
    |
    = note: expected type `std::rc::Rc<[T]>`
               found type `std::boxed::Box<[T]>`

error[E0599]: no method named `into_vec` found for type `std::rc::Rc<[T]>` in the current scope
   --> src\libsyntax\ptr.rs:176:18
    |
176 |         self.ptr.into_vec()
    |                  ^^^^^^^^
    |
    = help: did you mean `to_vec`?

error: aborting due to 7 previous errors

Some errors occurred: E0277, E0308, E0599.
For more information about an error, try `rustc --explain E0277`.
error: Could not compile `syntax`.

@petrochenkov
Copy link
Contributor

Looks like the ICE happens during error reporting and that's probably the same thing that was fixed by @Centril in f9e9c91 (method/suggest.rs).

@eddyb
Copy link
Member Author

eddyb commented Feb 15, 2019

@Zoxc @petrochenkov The query stack I get from RUST_BACKTRACE=1 is proving helpful, at least!

@eddyb
Copy link
Member Author

eddyb commented Feb 15, 2019

@bors try

@bors
Copy link
Contributor

bors commented Feb 15, 2019

⌛ Trying commit a05d12b with merge 40f1a011885f5188d36698593a7345499295f6a0...

@bors
Copy link
Contributor

bors commented Feb 15, 2019

☀️ Test successful - checks-travis
State: approved= try=True

@eddyb
Copy link
Member Author

eddyb commented Feb 15, 2019

@rust-timer build 40f1a011885f5188d36698593a7345499295f6a0

@rust-timer
Copy link
Collaborator

Success: Queued 40f1a011885f5188d36698593a7345499295f6a0 with parent f47ec2a, comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 40f1a011885f5188d36698593a7345499295f6a0

@eddyb
Copy link
Member Author

eddyb commented Feb 15, 2019

I wonder if the green is noise, or benefits from sharing.

@Mark-Simulacrum
Copy link
Member

I suspect that the instruction counts are noise; https://perf.rust-lang.org/compare.html?start=f47ec2ad5b6887b3d400aee49e2294bd27733d18&end=40f1a011885f5188d36698593a7345499295f6a0&stat=max-rss seems more likely to be accurate. It doesn't look like there are any major wins though, but no losses either.

@eddyb
Copy link
Member Author

eddyb commented Feb 15, 2019

I certainly expected it be more uniformly a regression but it's hard to tell what may end up shared.

@michaelwoerister michaelwoerister removed their assignment Mar 5, 2019
@TimNN
Copy link
Contributor

TimNN commented Mar 19, 2019

Ping from triage! What is the status of this PR?

@jonas-schievink
Copy link
Contributor

Closing due to inactivity. The changes have already been benchmarked, so this PR might be "done" now. Feel free to reopen and assign someone if this is wrong.

@eddyb
Copy link
Member Author

eddyb commented Jun 12, 2019

I'm now pursuing arena-allocating the AST, not unlike @Zoxc's #57173, so I doubt I'll ever need this.

@eddyb eddyb deleted the ast-rc branch June 12, 2019 11:43
@jonas-schievink jonas-schievink added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants