Skip to content

Work towards thread safety in rustc - #46779

Merged
bors merged 8 commits into
rust-lang:masterfrom
Zoxc:par-merge-without-sync
Dec 22, 2017
Merged

Work towards thread safety in rustc#46779
bors merged 8 commits into
rust-lang:masterfrom
Zoxc:par-merge-without-sync

Conversation

@Zoxc

@ZoxcZoxc commented Dec 17, 2017

Copy link
Copy Markdown
Contributor

This PR is split out from #45912. It contains changes which do not require the sync module.

@rust-highfive

rust-highfive commented Dec 17, 2017

Copy link
Copy Markdown
Contributor

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

@ZoxcZoxc assigned arielb1 and unassigned estebankDec 17, 2017
@Zoxc
Zoxcforce-pushed the par-merge-without-sync branch from c67d30e to 6b295aaCompareDecember 17, 2017 01:58
@kennytmkennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 17, 2017
bjorn3
bjorn3 requested changes Dec 17, 2017
View reviewed changes
Comment threadsrc/libserialize/collection_impls.rs Outdated

@bjorn3bjorn3Dec 17, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing trailing newline

Comment threadsrc/libsyntax_pos/lib.rs Outdated

@bjorn3bjorn3Dec 17, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please make the interner thread-safe?

Comment threadsrc/libsyntax_pos/symbol.rs Outdated

@bjorn3bjorn3Dec 17, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

@Zoxc

Zoxc commented Dec 17, 2017

Copy link
Copy Markdown
ContributorAuthor

@bjorn3 Thread-safety for the interners happens in a later commit.

@bjorn3

bjorn3 commented Dec 17, 2017

Copy link
Copy Markdown
Member

@Zoxc that commit doesnt seem to be included in this pr.

@Zoxc

Zoxc commented Dec 17, 2017

Copy link
Copy Markdown
ContributorAuthor

It is in #45912.

@michaelwoerister

michaelwoerister commented Dec 18, 2017

Copy link
Copy Markdown
Member

Each PR must leave the compiler in safe/valid state. Implementing Send/Sync for Symbol without making the interners threadsafe is not an option.

@Zoxc

Zoxc commented Dec 18, 2017

Copy link
Copy Markdown
ContributorAuthor

@michaelwoerister The !Send impls are not there for thread safety, but as a way to help ensure that Symbol types are only used with the interner they are created from. This trick no longer works when multiple threads share an interner. It also doesn't work if there are multiple interners per thread (which some of my upcoming patches allow).

@michaelwoerister

michaelwoerister commented Dec 18, 2017

Copy link
Copy Markdown
Member

My concern is just that this PR, as it is, would leave the compiler in a state where I can send Symbols between threads and thus get wrong results from the auto-generated Eq implementation. The !Send implementation should only be removed if the same PR makes sure that that cannot lead to logic errors.

arielb1
arielb1 reviewed Dec 18, 2017
View reviewed changes
Comment threadsrc/librustc_errors/lib.rs Outdated

@arielb1arielb1Dec 18, 2017•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

err_count probably needs to be done in a smarter way than here to assure thread-safety, so could you add a FIXME? In any case this is better than the way before this.

@ZoxcZoxcDec 19, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In which case is this not thread safe?

arielb1
arielb1 reviewed Dec 18, 2017
View reviewed changes
Comment threadsrc/librustc/ich/caching_codemap_view.rs Outdated

@arielb1arielb1Dec 18, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need a better locking discipline for the codemap? It feels like "too many locks, not enough discipline". Maybe just add a fixme to it?

@ZoxcZoxcDec 19, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that I just convert all RefCells into locks, there's probably plenty of places where there are too many locks.

@arielb1arielb1Dec 21, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

arielb1
arielb1 reviewed Dec 18, 2017
View reviewed changes
Comment threadsrc/libsyntax_pos/lib.rs Outdated

@arielb1arielb1Dec 18, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this sound?

arielb1
arielb1 reviewed Dec 18, 2017
View reviewed changes
Comment threadsrc/libsyntax_pos/symbol.rs Outdated

@arielb1arielb1Dec 18, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this sound?

@ZoxcZoxcDec 18, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, InternedString storing a &'static str isn't sound at all. I'll have to change it to using a u32 like Symbol.

@ZoxcZoxcDec 19, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well that made it unable to implement Deref<Target=str>, so I went with the approach suggested in a FIXME and just leak interned strings.

@Zoxc
Zoxcforce-pushed the par-merge-without-sync branch from 6b295aa to dbee5b9CompareDecember 19, 2017 01:00
@Zoxc
Zoxcforce-pushed the par-merge-without-sync branch from dbee5b9 to 84ce4f1CompareDecember 21, 2017 18:22
@Zoxc

Zoxc commented Dec 21, 2017

Copy link
Copy Markdown
ContributorAuthor

@bors r=arielb1

@bors

bors commented Dec 21, 2017

Copy link
Copy Markdown
Collaborator

📌 Commit 84ce4f1 has been approved by arielb1

@bors

bors commented Dec 22, 2017

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 84ce4f1 with merge 2c037d5...

bors added a commit that referenced this pull request Dec 22, 2017
@bors
Auto merge of#46779- Zoxc:par-merge-without-sync, r=arielb1
2c037d5
Work towards thread safety in rustc
This PR is split out from #45912. It contains changes which do not require the `sync` module.
@bors

bors commented Dec 22, 2017

Copy link
Copy Markdown
Collaborator

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing 2c037d5 to master...

@bors
bors merged commit 84ce4f1 into rust-lang:masterDec 22, 2017
@borsbors mentioned this pull request Dec 22, 2017
Merged
eddyb
eddyb reviewed Dec 22, 2017
View reviewed changes
Comment threadsrc/libsyntax/attr.rs
slot.set(r + 1);
r
});
let id = NEXT_ATTR_ID.fetch_add(1, Ordering::SeqCst);

@eddybeddybDec 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rust-lang/compiler Not sure how I feel about this kind of stuff - it will behave non-deterministically in any process running multiple rustc threads in parallel (atm rustc "owns" a thread).
IMO all thread-locals should change to scoped thread locals initialized to point to some memory owned by the thread that started the compilation, if we want multiple threads per rustc "instance".

@ZoxcZoxcDec 22, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I'll make this either a scoped thread local or put it in ParseSess if that happens to be easy.

@aturonaturon mentioned this pull request Aug 16, 2019
Closed
37 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bjorn3bjorn3bjorn3 requested changes
+2 more reviewers
@eddybeddybeddyb left review comments
@arielb1arielb1arielb1 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@arielb1arielb1

Labels

S-waiting-on-reviewStatus: Awaiting review from the assignee but also interested parties.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@Zoxc@rust-highfive@bjorn3@michaelwoerister@bors@eddyb@arielb1@kennytm@estebank