The default bug view has changed. See this FAQ.

clean up cycle collector JS tracing callbacks

RESOLVED FIXED in mozilla16

Status

()

Core
XPCOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

Trunk
mozilla16
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

15.35 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
I noticed that there are a few ways the JS tracing callbacks could be cleaned up while looking at bug 754151.

A common pattern is:
  if(tmp->mResultArrayBuffer) {
    NS_IMPL_CYCLE_COLLECTION_TRACE_JS_CALLBACK(tmp->mResultArrayBuffer,
                                               "mResultArrayBuffer")
  }
This has two issues.  First, TRACE_JS_CALLBACK already does a nullcheck, so we don't need to do another one.  Secondly, the inner part is the same as NS_IMPL_CYCLE_COLLECTION_TRACE_JS_MEMBER_CALLBACK, so we might as well use that.

Another common pattern that is used all over the place is:
  if (JSVAL_IS_GCTHING(tmp->mCachedKey)) {
    void *gcThing = JSVAL_TO_GCTHING(tmp->mCachedKey);
    NS_IMPL_CYCLE_COLLECTION_TRACE_JS_CALLBACK(gcThing, "mCachedKey")
  }

First of all, we can replace this with a macro NS_IMPL_CYCLE_COLLECTION_TRACE_JSVAL_MEMBER_CALLBACK(mCachedKey), because it is a very common pattern.  The second improvement we can make is to replace JSVAL_IS_GCTHING with JSVAL_IS_TRACEABLE.  Traceable just means non-null GCthing.  This allows us to eliminate the inner null check.  Though it involves an or-branch, so maybe it isn't actually any better.
(Assignee)

Comment 1

5 years ago
Created attachment 623342 [details] [diff] [review]
clean it up!
Assignee: nobody → continuation
(Assignee)

Comment 2

5 years ago
Created attachment 624084 [details] [diff] [review]
clean it up (removed the fairly pointless comment)

Try run looked fine here.  Ben, I'm flagging you for review because most of these changes are in IndexedDB and you know the CC.
Attachment #623342 - Attachment is obsolete: true
Attachment #624084 - Flags: review?(bent.mozilla)
Comment on attachment 624084 [details] [diff] [review]
clean it up (removed the fairly pointless comment)

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

Sorry this waited so long :(
Attachment #624084 - Flags: review?(bent.mozilla) → review+
(Assignee)

Comment 4

5 years ago
(In reply to ben turner [:bent] from comment #3)
> Sorry this waited so long :(

No problem, this isn't anything time critical. Just part of the slow fight against entropy. Thanks for the review!

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7d5ee4528c8
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/b7d5ee4528c8
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.