Skip to content

/ servo

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

Make the rule tree actually threadsafe #26227

Merged
merged 2 commits into from Apr 20, 2020
Merged

Make the rule tree actually threadsafe #26227

merged 2 commits into from Apr 20, 2020

Conversation

@nox
Copy link
Member

nox commented Apr 20, 2020

RuleTree::gc is now a safe method that any thread can call
at any time, and StrongRuleNode values can all be dropped
whenever their owner want to, on any thread.
@highfive
Copy link

highfive commented Apr 20, 2020

Heads up! This PR modifies the following files:

  • @emilio: components/style/rule_tree/unsafe_box.rs, components/style/rule_tree/core.rs, components/style/rule_tree/map.rs
@highfive
Copy link

highfive commented Apr 20, 2020

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify style code, but no tests are modified. Please consider adding a test!
@nox
Copy link
Member Author

nox commented Apr 20, 2020

r? @emilio
@highfive highfive assigned emilio and unassigned Manishearth Apr 20, 2020
@nox
Copy link
Member Author

nox commented Apr 20, 2020

@bors-servo try
bors-servo added a commit that referenced this pull request Apr 20, 2020
@bors-servo
Auto merge of #26227 - servo:layout-2020-rule-tree-threadsafe, r=<try>
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
63d7644 
Make the rule tree actually threadsafe

RuleTree::gc is now a safe method that any thread can call
at any time, and StrongRuleNode values can all be dropped
whenever their owner want to, on any thread.
@bors-servo
Copy link
Contributor

bors-servo commented Apr 20, 2020

⌛ Trying commit a9737da with merge 63d7644...
@bors-servo
Copy link
Contributor

bors-servo commented Apr 20, 2020

💔 Test failed - status-taskcluster
@emilio
emilio reviewed Apr 20, 2020
View changes
components/style/rule_tree/core.rs Show resolved Hide resolved
components/style/rule_tree/core.rs Outdated Show resolved Hide resolved
@nox nox force-pushed the layout-2020-rule-tree-threadsafe branch from a9737da to 51767e5 Apr 20, 2020
@nox
Change Map::get_or_insert_with to Map::entry
1c2de56
@nox nox force-pushed the layout-2020-rule-tree-threadsafe branch 2 times, most recently from 9f04b13 to 0b480a9 Apr 20, 2020
@nox
Copy link
Member Author

nox commented Apr 20, 2020

I think it would indeed make more sense to never decrement free_count and to only swap it with zero, what do you think @emilio? I'm afraid the decrement in swap_free_list_and_gc can underflow.
@emilio
emilio approved these changes Apr 20, 2020
View changes
Copy link
Member

emilio left a comment

This looks good to me, thanks!

I think the atomic fences could be a bit more limited, and just be loads. Other than that and a minor nit this looks good.
components/style/rule_tree/core.rs Show resolved Hide resolved
components/style/rule_tree/core.rs Outdated
) {
Ok(_) => {
// Increment the free count by one.
root.free_count.fetch_add(1, Ordering::Relaxed);

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio Apr 20, 2020

Member

We should probably rename .free_count to .estimated_free_count or .approximate_free_count something, as it can't really be relied on for other than heuristics.

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio Apr 20, 2020

Member

Ah, actually you made it so it's not resetted to zero but decremented, so this is alright. Thanks!

This comment has been minimized.

Sign in to view
Copy link
@nox

nox Apr 20, 2020

Author Member

I changed it to approximate_free_count and I made swap_free_list_and_gc just swap it to 0, because what I did initially could make it underflow.

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio Apr 20, 2020

Member

Yeah, you're right. For posterity the bad case is: you insert into the free-list successfully, and another thread steals it and completes a gc before you get a chance to increment the count. Then the other thread would decrement the count before you get the chance to increment it, potentially underflowing.
components/style/rule_tree/core.rs Show resolved Hide resolved
components/style/rule_tree/core.rs Show resolved Hide resolved
@nox nox force-pushed the layout-2020-rule-tree-threadsafe branch from 0b480a9 to 50f4a0e Apr 20, 2020
@nox
Make the rule tree actually threadsafe
Loading status checks…
7f54d14 
RuleTree::gc is now a safe method that any thread can call
at any time, and StrongRuleNode values can all be dropped
whenever their owner want to, on any thread.
@nox nox force-pushed the layout-2020-rule-tree-threadsafe branch from 50f4a0e to 7f54d14 Apr 20, 2020
@emilio
Copy link
Member

emilio commented Apr 20, 2020

@bors-servo r=me
@emilio
Copy link
Member

emilio commented Apr 20, 2020

err,

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Apr 20, 2020

📌 Commit 7f54d14 has been approved by emilio
@bors-servo
Copy link
Contributor

bors-servo commented Apr 20, 2020

⌛ Testing commit 7f54d14 with merge 4c4f588...
@bors-servo
Copy link
Contributor

bors-servo commented Apr 20, 2020

☀️ Test successful - status-taskcluster
Approved by: emilio
Pushing 4c4f588 to master...
@bors-servo bors-servo merged commit 4c4f588 into master Apr 20, 2020
2 checks passed
2 checks passed
Community-TC (pull_request) TaskGroup: success
Details
homu Test successful
Details
@bors-servo bors-servo deleted the layout-2020-rule-tree-threadsafe branch Apr 20, 2020
@nox nox mentioned this pull request Apr 20, 2020
Closed
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Apr 23, 2020
@nox @emilio
Bug 1632363 - Change Map::get_or_insert_with to Map::entry. r=emilio
7b4fc70 
Cherry-picked from servo/servo#26227
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Apr 23, 2020
@nox @emilio
Bug 1632363 - Make the rule tree actually threadsafe. r=emilio
9c1d8b5 
RuleTree::gc is now a safe method that any thread can call
at any time, and StrongRuleNode values can all be dropped
whenever their owner want to, on any thread.

Cherry-picked from servo/servo#26227
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Apr 23, 2020
@nox @emilio
Bug 1632363 - Change Map::get_or_insert_with to Map::entry. r=emilio
eca5e09 
Cherry-picked from servo/servo#26227
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Apr 23, 2020
@nox @emilio
Bug 1632363 - Make the rule tree actually threadsafe. r=emilio
8a7cfb0 
RuleTree::gc is now a safe method that any thread can call
at any time, and StrongRuleNode values can all be dropped
whenever their owner want to, on any thread.

Cherry-picked from servo/servo#26227
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Apr 25, 2020
@marco-c
Bug 1632363 - Change Map::get_or_insert_with to Map::entry. r=emilio
4901163 
Cherry-picked from servo/servo#26227

UltraBlame original commit: 5a8370c76868c513d36a2e981b7bd90781785748
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Apr 25, 2020
@marco-c
Bug 1632363 - Make the rule tree actually threadsafe. r=emilio
378de40 
RuleTree::gc is now a safe method that any thread can call
at any time, and StrongRuleNode values can all be dropped
whenever their owner want to, on any thread.

Cherry-picked from servo/servo#26227

UltraBlame original commit: 08a6488edafb6a74e2282ed815e7caf20a85a327
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Apr 25, 2020
@marco-c
Bug 1632363 - Change Map::get_or_insert_with to Map::entry. r=emilio
e14521c 
Cherry-picked from servo/servo#26227

UltraBlame original commit: 5a8370c76868c513d36a2e981b7bd90781785748
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Apr 25, 2020
@marco-c
Bug 1632363 - Make the rule tree actually threadsafe. r=emilio
b253cc2 
RuleTree::gc is now a safe method that any thread can call
at any time, and StrongRuleNode values can all be dropped
whenever their owner want to, on any thread.

Cherry-picked from servo/servo#26227

UltraBlame original commit: 08a6488edafb6a74e2282ed815e7caf20a85a327
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Apr 25, 2020
@marco-c
Bug 1632363 - Change Map::get_or_insert_with to Map::entry. r=emilio
2eaeefc 
Cherry-picked from servo/servo#26227

UltraBlame original commit: 5a8370c76868c513d36a2e981b7bd90781785748
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Apr 25, 2020
@marco-c
Bug 1632363 - Make the rule tree actually threadsafe. r=emilio
8db86d9 
RuleTree::gc is now a safe method that any thread can call
at any time, and StrongRuleNode values can all be dropped
whenever their owner want to, on any thread.

Cherry-picked from servo/servo#26227

UltraBlame original commit: 08a6488edafb6a74e2282ed815e7caf20a85a327
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilio emilio

Assignees

@emilio emilio

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@nox @highfive @bors-servo @emilio @Manishearth
You can’t perform that action at this time.