nsContentUtils::RemoveScriptBlocker should release-assert that it's on the main thread

RESOLVED FIXED in Firefox 40

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jld, Assigned: jld)

Tracking

Trunk
mozilla40
Points:
---

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
nsContentUtils::RemoveScriptBlocker can run a variety of interesting main-thread-only code, including arbitrary content JS; this probably justifies a release assertion that it's called on the main thread.

We could also do the check in AddScriptBlocker; I'm not sure how incrementally useful this is.  If I have to pick one I'd rather have the assertion in RemoveScriptBlocker, because that's the one that runs the runnables.
(Assignee)

Comment 2

3 years ago
Created attachment 8584909 [details] [diff] [review]
Patch

The patch is trivial, but the performance implications might not be.
Attachment #8584909 - Flags: review?(bugs)

Comment 3

3 years ago
Could you do some microbenchmarking here? Something silly like
data:text/html,<script>setInterval(function () {  var s = (new Date()).getTime();  for (var i = 0; i < 500000; ++i ) document.documentElement.setAttribute("foo", i);  document.body.textContent = ((new Date()).getTime() - s) + "ms" }, 50); </script>
and look at if RemoveScriptBlocker shows up worse in the profile than it shows up currently?

Comment 4

3 years ago
Comment on attachment 8584909 [details] [diff] [review]
Patch

I'm somewhat reluctant to add more release asserts because it hints that
patches aren't being tested enough using debug builds and in that case 
better fix would be to increase testing on debug builds.

But r+, if you don't see the assert badly in opt build profiles.
Attachment #8584909 - Flags: review?(bugs) → review+
(Assignee)

Comment 5

3 years ago
I added some RDTSCs to RemoveScriptBlocker, and tried the test case in comment #3 on OS X natively and in a Linux VM (VMware Fusion).  I don't have a Windows VM at the moment, but if Windows implements TLS significantly differently then it might be worth gathering data there as well.

The assertion costs about 18 cycles (6.4ns); the rest of RemoveScriptBlocker is about 280 (100ns) natively and 180 (64ns) in the VM, and I don't know why that difference exists.

The loop calls RemoveScriptBlocker 1000000 times (2x per iteration) and runs in about 500-520 ms natively and 660-690 ms in the VM.

So this is a significant increase relative to RemoveScriptBlocker itself, but it's <1.5% of the setAttribute microbenchmark.  I'm not sure how we feel about that.
(Assignee)

Comment 6

3 years ago
Thoughts about comment #5?
Flags: needinfo?(bugs)

Comment 8

3 years ago
Anything above 0.5% or so sounds rather high to me.
(we have the issue in general that we have tons of "doesn't-show-too-badly-in-profiles" stuff, which makes optimizations rather hard.)

Perhaps release assert only in nightlies and aurora?
Flags: needinfo?(bugs)
(Assignee)

Comment 10

3 years ago
Created attachment 8589925 [details] [diff] [review]
Patch [v2; less assertive]

MOZ_DIAGNOSTIC_ASSERT is MOZ_RELEASE_ASSERT on nightly/aurora and MOZ_ASSERT on beta/release, so that works.
Attachment #8584909 - Attachment is obsolete: true
Attachment #8589925 - Flags: review+
Jed, is this ready to land?
(Assignee)

Comment 12

3 years ago
(In reply to Andrew McCreight [:mccr8] from comment #11)
> Jed, is this ready to land?

Looks like it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c14f9f9660bb

Thanks for the reminder.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c4dcce22831f
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.