Replace Rc with Lrc for shared data #48586

Zoxc · 2018-02-27T16:17:18Z

This replaces Rcs reachable from TyCtxt with Lrc. This has no effect unless cfg(parallel_queries) is set. It also contains a fix for the Decodable impl for Arc.

r? @nikomatsakis

nikomatsakis · 2018-02-27T18:19:20Z

cc @rust-lang/compiler - I think this step seems pretty clearly desirable.

michaelwoerister · 2018-02-27T18:29:59Z

I'll take a closer look tomorrow!

nikomatsakis · 2018-02-27T18:49:08Z

r? @michaelwoerister

nikomatsakis · 2018-02-27T18:49:20Z

@michaelwoerister -- hope that's ok, just feeling a bit overwhelmed with reviews right now :)

michaelwoerister

Thanks for the PR, @Zoxc! This looks good to me. I'd still like to wait for feedback about the change in libproc_macro but otherwise this has my r+.

We should make sure to add at least one build that compiles with parallel queries enabled to our CI. I'll open an issue for that.

It would also be interesting to see the performance impact of switching to atomic reference counting. We can do a try-build once the PR is merged.

michaelwoerister · 2018-02-28T09:51:01Z

src/libproc_macro/lib.rs

@@ -306,7 +307,7 @@ pub struct LineColumn {
 #[unstable(feature = "proc_macro", issue = "38356")]
 #[derive(Clone)]
 pub struct SourceFile {
-    filemap: Rc<FileMap>,
+    filemap: Lrc<FileMap>,


@rust-lang/compiler @alexcrichton, is it safe to modify these definitions? Or are they part of a public interface?

Nah these are unstable and just private fields, so should be all good!

Great, thanks for the quick feedback!

This is IMO problematic because it will eventually be observable in user code - you should impl !Send and !Sync on SourceFile to hide the difference before we forget and stabilize it.

There probably also other types this applies to in libproc_macro.

michaelwoerister · 2018-02-28T10:13:06Z

src/libsyntax/ext/expand.rs

@@ -437,7 +438,7 @@ impl<'a, 'b> MacroExpander<'a, 'b> {
        }
    }

-    fn expand_invoc(&mut self, invoc: Invocation, ext: Rc<SyntaxExtension>) -> Option<Expansion> {
+    fn expand_invoc(&mut self, invoc: Invocation, ext: Lrc<SyntaxExtension>) -> Option<Expansion> {


These should probably just use &SyntaxExtension.

michaelwoerister · 2018-02-28T10:28:29Z

@rust-lang/infra: Upping the priority since this patch has such a big risk of bitrotting.

@bors p=10

kennytm · 2018-02-28T10:31:22Z

[01:22:43]    Compiling rustc_driver v0.0.0 (file:///checkout/src/librustc_driver)
[01:22:44] error[E0433]: failed to resolve. Use of undeclared type or module `Lrc`
[01:22:44]    --> librustc_driver/test.rs:108:40
[01:22:44]     |
[01:22:44] 108 |                                        Lrc::new(CodeMap::new(FilePathMapping::empty())));
[01:22:44]     |                                        ^^^ Use of undeclared type or module `Lrc`
[01:22:44] 
[01:22:45] error: aborting due to previous error

michaelwoerister · 2018-03-02T09:29:09Z

@bors r+

bors · 2018-03-02T09:29:09Z

📌 Commit 5539554 has been approved by michaelwoerister

bors · 2018-03-02T09:39:57Z

🔒 Merge conflict

michaelwoerister · 2018-03-02T10:09:34Z

@Zoxc, you can just r=me after resolving such merge conflicts.

Zoxc · 2018-03-02T17:27:22Z

@bors r=michaelwoerister

bors · 2018-03-02T20:28:28Z

💔 Test failed - status-appveyor

Manishearth · 2018-03-02T20:52:25Z

@bors retry

timeout

smashes something

bors · 2018-03-02T21:23:17Z

⌛ Testing commit fce7201 with merge c57c60edd9b07424c98a8bbcf4d8ce0a495e0c72...

bors · 2018-03-03T00:24:48Z

💔 Test failed - status-appveyor

Manishearth · 2018-03-03T00:27:05Z

@bors retry

appveyor timeout again

smashes another thing

bors · 2018-03-03T02:02:52Z

⌛ Testing commit fce7201 with merge e8af0f4...

@nikomatsakis

Replace Rc with Lrc for shared data This replaces `Rc`s reachable from `TyCtxt` with `Lrc`. This has no effect unless `cfg(parallel_queries)` is set. It also contains a fix for the `Decodable` impl for `Arc`. r? @nikomatsakis

bors · 2018-03-03T04:54:23Z

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing e8af0f4 to master...

michaelwoerister · 2018-03-05T08:19:04Z

🎉

bstrie · 2018-03-08T01:34:55Z

🎉

smashes something

rust-highfive assigned nikomatsakis Feb 27, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 27, 2018

Zoxc force-pushed the atomic-rc branch from 5bd8046 to 830fd56 Compare February 27, 2018 17:47

rust-highfive assigned michaelwoerister and unassigned nikomatsakis Feb 27, 2018

Centril added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 27, 2018

michaelwoerister reviewed Feb 28, 2018

View reviewed changes

Zoxc force-pushed the atomic-rc branch 2 times, most recently from 4e2efcf to 5539554 Compare March 2, 2018 05:09

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 Mar 2, 2018

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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 2, 2018

Zoxc added 3 commits March 2, 2018 10:45

Fix Decodable impl for Arc

878f5b0

Replace Rc with Lrc for shared data

b74e97c

Impl !Send and !Sync for SourceFile

fce7201

Zoxc force-pushed the atomic-rc branch from 5539554 to fce7201 Compare March 2, 2018 09:51

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 2, 2018

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 Mar 2, 2018

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 Mar 2, 2018

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 Mar 3, 2018

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 Mar 3, 2018

bors mentioned this pull request Mar 3, 2018

Update Cargo submodule #48549

Merged

bors merged commit fce7201 into rust-lang:master Mar 3, 2018

Zoxc deleted the atomic-rc branch March 3, 2018 06:37

aturon mentioned this pull request Aug 16, 2019

Audit sources of shared state across rustc #63643

Open

37 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace Rc with Lrc for shared data #48586

Replace Rc with Lrc for shared data #48586

Zoxc commented Feb 27, 2018

nikomatsakis commented Feb 27, 2018

michaelwoerister commented Feb 27, 2018

nikomatsakis commented Feb 27, 2018

nikomatsakis commented Feb 27, 2018

michaelwoerister left a comment

michaelwoerister Feb 28, 2018

alexcrichton Feb 28, 2018

michaelwoerister Feb 28, 2018

eddyb Feb 28, 2018

Zoxc Mar 2, 2018

michaelwoerister Feb 28, 2018

michaelwoerister commented Feb 28, 2018

kennytm commented Feb 28, 2018

michaelwoerister commented Mar 2, 2018

bors commented Mar 2, 2018

bors commented Mar 2, 2018

michaelwoerister commented Mar 2, 2018 •

edited

Loading

Zoxc commented Mar 2, 2018

bors commented Mar 2, 2018

Manishearth commented Mar 2, 2018

bors commented Mar 2, 2018

bors commented Mar 3, 2018

Manishearth commented Mar 3, 2018

bors commented Mar 3, 2018

bors commented Mar 3, 2018

michaelwoerister commented Mar 5, 2018

bstrie commented Mar 8, 2018

Replace Rc with Lrc for shared data #48586

Replace Rc with Lrc for shared data #48586

Conversation

Zoxc commented Feb 27, 2018

nikomatsakis commented Feb 27, 2018

michaelwoerister commented Feb 27, 2018

nikomatsakis commented Feb 27, 2018

nikomatsakis commented Feb 27, 2018

michaelwoerister left a comment

Choose a reason for hiding this comment

michaelwoerister Feb 28, 2018

Choose a reason for hiding this comment

alexcrichton Feb 28, 2018

Choose a reason for hiding this comment

michaelwoerister Feb 28, 2018

Choose a reason for hiding this comment

eddyb Feb 28, 2018

Choose a reason for hiding this comment

Zoxc Mar 2, 2018

Choose a reason for hiding this comment

michaelwoerister Feb 28, 2018

Choose a reason for hiding this comment

michaelwoerister commented Feb 28, 2018

kennytm commented Feb 28, 2018

michaelwoerister commented Mar 2, 2018

bors commented Mar 2, 2018

bors commented Mar 2, 2018

michaelwoerister commented Mar 2, 2018 • edited Loading

Zoxc commented Mar 2, 2018

bors commented Mar 2, 2018

Manishearth commented Mar 2, 2018

bors commented Mar 2, 2018

bors commented Mar 3, 2018

Manishearth commented Mar 3, 2018

bors commented Mar 3, 2018

bors commented Mar 3, 2018

michaelwoerister commented Mar 5, 2018

bstrie commented Mar 8, 2018

michaelwoerister commented Mar 2, 2018 •

edited

Loading