don't expose outstanding request count, just whether a context is "active"

RESOLVED FIXED in mozilla18

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla18
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Posted patch patchSplinter Review
We currently expose the exact request count of a context when all we need is the boolean query "is anything running in this context".
Attachment #662317 - Flags: review?(continuation)
Comment on attachment 662317 [details] [diff] [review]
patch

Review of attachment 662317 [details] [diff] [review]:
-----------------------------------------------------------------

Nice! I'm glad this weird overprecision will be gone. I don't know if Olli wants to look at this too, as he understands the outstanding request stuff a little better than me, but it looks straightforward enough.

::: dom/base/nsJSEnvironment.cpp
@@ +1207,5 @@
>  nsJSContext::GetCCRefcnt()
>  {
>    nsrefcnt refcnt = mRefCnt.get();
>    if (NS_LIKELY(mContext))
> +    refcnt += js::ContextHasOutstandingRequests(mContext) ? 1 : 0;

This might be better as:

if (mContext && js::ContextHasOutstandingblahblah) {
  refcnt += 1;
}

I doubt this code is perf sensitive enough that the NS_LIKELY is worth making the line longer. You should also add the braces if you are going to touch all of this anyways, to match browser style.

Maybe put a comment here like "See JSContextParticipant::TraverseImpl for explanation."

::: js/xpconnect/src/nsXPConnect.cpp
@@ +923,5 @@
> +        // JSContexts do not have an internal refcount and always have a single
> +        // owner (e.g., nsJSContext). Thus, the default refcount is 1. However,
> +        // in the (abnormal) case of synchronous cycle-collection, the context
> +        // may be actively executing code in which case we want to treat it as
> +        // rooted by adding an extra refcount.

Great comment, thanks.
Attachment #662317 - Flags: review?(continuation) → review+
Read the patch quickly, looks ok to me.
https://hg.mozilla.org/mozilla-central/rev/6be84c0e0f0e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.