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

Bypass the lock in get_property_value #18386

Merged
merged 1 commit into from Sep 6, 2017

Conversation

@bholley
Copy link
Contributor

commented Sep 6, 2017

This measurably improves performance - see https://bugzilla.mozilla.org/show_bug.cgi?id=1355599#c21


This change is Reviewable

MozReview-Commit-ID: ECkLn5TPBDO
@highfive

This comment has been minimized.

Copy link

commented Sep 6, 2017

Heads up! This PR modifies the following files:

  • @canaltinova: components/style/shared_lock.rs
  • @emilio: components/style/shared_lock.rs, ports/geckolib/glue.rs
@highfive

This comment has been minimized.

Copy link

commented Sep 6, 2017

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!
@bholley

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2017

@highfive highfive assigned upsuper and unassigned emilio Sep 6, 2017
@upsuper

This comment has been minimized.

Copy link
Member

commented Sep 6, 2017

I'm with @SimonSapin here that we probably shouldn't take this.

42ms in 1M getting doesn't really sound like a huge problem, and given that Blink is even slower here, it is probably not a very critical thing needs to be optimized that much. Also since we are getting closer to beta uplift, this really seems to be the kind of change that has certain risk without a big improvement.

It is true that CSSOM happens only in the main thread, so the change itself should be safe. But in case we mess things up somehow, this could lead to more weird behavior and obscure crash reports.

@upsuper

This comment has been minimized.

Copy link
Member

commented Sep 6, 2017

I'm... not really happy with adding more unsafe code like this... but if you insist that it's important for performance, I guess I'm fine...

@upsuper

This comment has been minimized.

Copy link
Member

commented Sep 6, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2017

📌 Commit 48c95cd has been approved by upsuper

@bholley

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2017

@upsuper and I discussed this on IRC. The important bits are:
(1) The performance increase here is measurable and relevant. Adding an extra zero to the iteration count on the microbenchmark in bug 1355599, Safari gets 500ms. Stylo gets in the 2000s, and this patch shaves ~200ms off of that. The fact that Chrome was slower here is a recent regression in chrome.
(2) Thus far, nobody has provided an example of a bug that this locking protects us from. In Gecko, CSSOM is main-thread-only. If somebody is calling Servo_DeclarationBlock_GetPropertyValueById in a racey way, they're also calling all of the C++ CSSOM code up the callstack in a racey way.

I agree we should default to provably-safe patterns, but when a performance tradeoff arises we need to weigh the performance impact against the benefit. And given that this particular tradeoff is about checking for mutable aliasing in stack frame N, but stack frames N-1...0 are in C++ and have no protections against mutable aliasing, I don't think the protections are buying us much.

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2017

⌛️ Testing commit 48c95cd with merge e3568ba...

bors-servo added a commit that referenced this pull request Sep 6, 2017
Bypass the lock in get_property_value

This measurably improves performance - see https://bugzilla.mozilla.org/show_bug.cgi?id=1355599#c21

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18386)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2017

@bors-servo bors-servo merged commit 48c95cd into servo:master Sep 6, 2017
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
})
// This callsite is hot enough that the lock acquisition shows up in profiles.
// Using an unchecked read here improves our performance by ~10% on the
// microbenchmark in bug 1355599.

This comment has been minimized.

Copy link
@nox

nox Sep 6, 2017

Member

How is this not unsound? That should clearly be explained.

This comment has been minimized.

Copy link
@bholley

bholley Sep 6, 2017

Author Contributor

Because there's only one SharedLock in Gecko, and it's backed by an AtomicRefCell, and thus would panic if the invariants were ever violated (this is all gecko-specific FFI code). So this is all just belt-and-suspenders, and less relevant to Gecko which binds all CSSOM stuff to the main thread.

This comment has been minimized.

Copy link
@nox

nox Sep 7, 2017

Member

What would panic?

This comment has been minimized.

Copy link
@bholley

bholley Sep 7, 2017

Author Contributor

Acquiring the SharedLock, in the case that it was already acquired, would panic, rather than block (since it's backed by an AtomicRefCell in Gecko, as opposed to an RwLock in Servo). This is why this machinery is just a release-mode assertion in geckolib builds, and the condition it's asserting against is not particularly likely.

@ionutgoldan

This comment has been minimized.

Copy link

commented Sep 20, 2017

This brought a noticeable improvement:

== Change summary for alert #9251 (as of September 06 2017 06:09 UTC) ==

Improvements:

5% Stylo Servo_DeclarationBlock_GetPropertyById_Bench windows7-32 opt 232,901.17 -> 221,106.42

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=9251

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.