Closed Bug 1148650 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: jld, Assigned: jld)

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch Patch (obsolete) — Splinter Review
The patch is trivial, but the performance implications might not be.
Attachment #8584909 - Flags: review?(bugs)
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 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+
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.
Thoughts about comment #5?
Flags: needinfo?(bugs)
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)
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?
(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
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.