Last Comment Bug 754478 - clean up cycle collector JS tracing callbacks
: clean up cycle collector JS tracing callbacks
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Andrew McCreight [:mccr8]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-11 15:34 PDT by Andrew McCreight [:mccr8]
Modified: 2012-06-10 15:27 PDT (History)
9 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
clean it up! (15.47 KB, patch)
2012-05-11 15:53 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
clean it up (removed the fairly pointless comment) (15.35 KB, patch)
2012-05-15 09:59 PDT, Andrew McCreight [:mccr8]
bent.mozilla: review+
Details | Diff | Splinter Review

Description Andrew McCreight [:mccr8] 2012-05-11 15:34:32 PDT
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.
Comment 1 Andrew McCreight [:mccr8] 2012-05-11 15:53:32 PDT
Created attachment 623342 [details] [diff] [review]
clean it up!
Comment 2 Andrew McCreight [:mccr8] 2012-05-15 09:59:30 PDT
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.
Comment 3 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-06-08 15:41:17 PDT
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 :(
Comment 4 Andrew McCreight [:mccr8] 2012-06-10 11:52:48 PDT
(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
Comment 5 Ryan VanderMeulen [:RyanVM] 2012-06-10 15:27:44 PDT
https://hg.mozilla.org/mozilla-central/rev/b7d5ee4528c8

Note You need to log in before you can comment on or make changes to this bug.