Last Comment Bug 549683 - use jshashtable instead of jsdhash in jsscope.cpp and jspropertytree.cpp
: use jshashtable instead of jsdhash in jsscope.cpp and jspropertytree.cpp
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Terrence Cole [:terrence]
:
Mentors:
Depends on: 497789
Blocks:
  Show dependency treegraph
 
Reported: 2010-03-02 11:59 PST by Brendan Eich [:brendan]
Modified: 2012-04-03 02:04 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v0 (20.11 KB, patch)
2012-03-23 15:51 PDT, Terrence Cole [:terrence]
no flags Details | Diff | Review
v1: I spoke too soon. (19.69 KB, patch)
2012-03-23 16:06 PDT, Terrence Cole [:terrence]
jwalden+bmo: review+
Details | Diff | Review
v2: With review feedback (19.61 KB, patch)
2012-03-30 10:38 PDT, Terrence Cole [:terrence]
terrence: review+
Details | Diff | Review

Description Brendan Eich [:brendan] 2010-03-02 11:59:36 PST
Nuff said.

/be
Comment 1 Terrence Cole [:terrence] 2012-03-23 15:51:44 PDT
Created attachment 608896 [details] [diff] [review]
v0

This allows us to stop building jsdhash.cpp for SpiderMonkey, but we do have to keep it and keep jsdhash.h in INSTALLED_HEADERS for xpconnect.
Comment 2 Terrence Cole [:terrence] 2012-03-23 16:06:31 PDT
Created attachment 608899 [details] [diff] [review]
v1: I spoke too soon.

It looks like XPConnect uses the symbols out of libjs.  What an odd build system we have.
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2012-03-29 18:21:45 PDT
Comment on attachment 608899 [details] [diff] [review]
v1: I spoke too soon.

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

\o/

::: js/src/builtin/TestingFunctions.cpp
@@ +253,1 @@
>      bool                ok;

While you're touching this, mind moving |bool ok;| to the end?  Keep things in an order more conducive to packing, during future changes.

And, hey, let's get rid of the pointless typedef, too.  :-)

@@ -355,5 @@
>  
>      JS_TracerInit(&countTracer.base, JS_GetRuntime(cx), CountHeapNotify);
> -    if (!JS_DHashTableInit(&countTracer.visited, JS_DHashGetStubOps(),
> -                           NULL, sizeof(JSDHashEntryStub),
> -                           JS_DHASH_DEFAULT_CAPACITY(100))) {

Why this change?  Wasn't it much better to spew out three lines of obscure arguments and internal controls to initialize the hashset, versus just doing it in a single line with no arguments?  r-, not hardcore enough.

::: js/src/jsapi.cpp
@@ +2609,5 @@
>      char            edgeName[1];    /* name of the edge from parent->thing
>                                         into thing */
>  };
>  
> +typedef HashSet<void *, PointerHasher<void *, 3>, SystemAllocPolicy> VisitedSet;

Could you file a followup to augment PointerHasher to assert that the zero-bit are actually zero, if the right value of an additional template parameter is specified?  (Or maybe it should always assert, I don't know if there are actually some uses that wouldn't consider non-zero stripped-off bits to be a bug.)

@@ +2758,2 @@
>          return false;
>      }

Remove the {} now, and add a blank line afterward.

@@ +2761,1 @@
>      dtrc.ok = JS_TRUE;

true

@@ +2771,5 @@
>      } else {
>          JS_TraceChildren(&dtrc.base, startThing, startKind);
>      }
>  
> +    size_t depth = 1;

Move this down past the |if|.

::: js/src/jsdbgapi.cpp
@@ +983,3 @@
>      nbytes += sizeof(JSString);
>      nbytes += (atom->length() + 1) * sizeof(jschar);
>      return nbytes;

I'd convert this to a single return statement, with the current line-by-line conceptual splitting of size components.
Comment 4 Terrence Cole [:terrence] 2012-03-30 10:12:10 PDT
(In reply to Jeff Walden (:Waldo) (remove +bmo to email) from comment #3)
> While you're touching this, mind moving |bool ok;| to the end?  Keep things
> in an order more conducive to packing, during future changes.

Oh, brilliant.  That's a good tool to have at my disposal.  Thank you!
 
> @@ -355,5 @@
> >  
> >      JS_TracerInit(&countTracer.base, JS_GetRuntime(cx), CountHeapNotify);
> > -    if (!JS_DHashTableInit(&countTracer.visited, JS_DHashGetStubOps(),
> > -                           NULL, sizeof(JSDHashEntryStub),
> > -                           JS_DHASH_DEFAULT_CAPACITY(100))) {
> 
> Why this change?  Wasn't it much better to spew out three lines of obscure
> arguments and internal controls to initialize the hashset, versus just doing
> it in a single line with no arguments?  r-, not hardcore enough.

I'm trying, but Luke keeps r-ing my patches to the new HashTable. :-)
 
> ::: js/src/jsapi.cpp
> Could you file a followup to augment PointerHasher to assert that the
> zero-bit are actually zero, if the right value of an additional template
> parameter is specified?  (Or maybe it should always assert, I don't know if
> there are actually some uses that wouldn't consider non-zero stripped-off
> bits to be a bug.)

You're ruining the mystery!

https://bugzilla.mozilla.org/show_bug.cgi?id=740874
Comment 5 Terrence Cole [:terrence] 2012-03-30 10:38:31 PDT
Created attachment 610933 [details] [diff] [review]
v2: With review feedback

https://tbpl.mozilla.org/?tree=Try&rev=81bf46bed3d1
Comment 6 Terrence Cole [:terrence] 2012-04-02 13:17:49 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/a214d423b525
Comment 7 Terrence Cole [:terrence] 2012-04-02 13:20:10 PDT
Thanks for the cleanup Andrew!  That was incredibly odd.
Comment 8 Marco Bonardo [::mak] 2012-04-03 02:04:52 PDT
https://hg.mozilla.org/mozilla-central/rev/a214d423b525

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