Last Comment Bug 647367 - rm jshash.h and remaining uses
: rm jshash.h and remaining uses
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: Nicholas Nethercote [:njn]
:
Mentors:
Depends on: 566700 649576 661903
Blocks: 666042
  Show dependency treegraph
 
Reported: 2011-04-01 16:51 PDT by Luke Wagner [:luke]
Modified: 2012-08-17 09:53 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP: rm jshash.h (42.54 KB, patch)
2011-07-20 13:52 PDT, Chris Leary [:cdleary] (not checking bugmail)
no flags Details | Diff | Review
Sequester jshash.{h,cpp} in js/jsd/. (12.38 KB, patch)
2012-07-17 23:07 PDT, Nicholas Nethercote [:njn]
luke: review+
Details | Diff | Review

Description Luke Wagner [:luke] 2011-04-01 16:51:05 PDT
Almost all uses of the bucket-based hash table have been removed save 3.  We could numHashTableImplementations-- if these last 3 were finished off.
Comment 1 Luke Wagner [:luke] 2011-04-01 17:02:01 PDT
Whoa, JSAtomList and JSHashTable are rather intertwined.  That one is non-trivial.
Comment 2 Chris Leary [:cdleary] (not checking bugmail) 2011-04-03 13:45:22 PDT
(In reply to comment #1)
> Whoa, JSAtomList and JSHashTable are rather intertwined.  That one is
> non-trivial.

No kidding! The JSAtomList starts off as a linked list of JSHashEntry (which is effectively just a generic linked list node). Once the list hits critical mass the JSAtomList table-ifies with explicitly managed stable ordering of the entries during the transition.

JSAtomList's implementation manages the hash table's guts to be a multimap with special properties: a lookup that hits causes a node to be moved to the front of the hash chain; hoisted definition nodes are added at the end of the hash chain; any number of shadowed definitions can coexist in a single hash bucket.

I'm mulling over a few solutions.
Comment 3 Luke Wagner [:luke] 2011-04-03 16:02:19 PDT
Wow.  Well, if you are going break an abstraction, might as well break the hell out of it!
Comment 4 Chris Leary [:cdleary] (not checking bugmail) 2011-06-28 16:22:09 PDT
billm's taking care of the scripFilenameTable use in bug 661903 and jorendorff is self-hosting sharp var support in bug 486643. There's one more minor usage in traceviz that follows the same form as the script filename table. When billm's patch lands we can generalize a CStringHasher.

Now we wait.
Comment 5 Chris Leary [:cdleary] (not checking bugmail) 2011-07-20 13:52:02 PDT
Created attachment 547230 [details] [diff] [review]
WIP: rm jshash.h

Decided to see if I could remove jshash in the engine. Only took about an hour. JS binary goes down by ~20K. However, there are still some uses in jsd! That's for another day.
Comment 6 Nicholas Nethercote [:njn] 2012-07-17 23:07:59 PDT
Created attachment 643259 [details] [diff] [review]
Sequester jshash.{h,cpp} in js/jsd/.

The removal of sharps made this a lot easier.

Although js/src/ barely uses jshash.{h,cpp}, js/jsd/ still does.  So I just
moved those files into js/jsd/.  (I'm not sure if that'll show up in
Bugzilla's nicely-formatted patch viewers.)  I figure this is a win because
I have a notion that js/jsd/ is headed for the scrap-heap -- is that right?
And even if it's not, this greatly reduces the visibility of those files.

A few things from those moved files were used in js/src/ and
js/xpconnect/src/.  Here's what I did with them:

- I replaced JSHashNumber with js::HashNumber;  they're both typedefs of
  uint32_t.

- I moved JS_HashString, which has a single use, into jsstr.{h,cpp}.

- I inlined the single use of JS_GOLDEN_RATIO.

- And I removed ATOM_HASH because it is dead code.
Comment 7 Luke Wagner [:luke] 2012-07-17 23:42:25 PDT
Comment on attachment 643259 [details] [diff] [review]
Sequester jshash.{h,cpp} in js/jsd/.

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

Sweet!  I agree with your reasoning on it to jsd.

::: js/src/jsatom.h
@@ +100,5 @@
>  #endif
> +
> +    static const js::HashNumber goldenRatio = 0x9E3779B9U;
> +
> +    return n * goldenRatio;

Even better, I think you could replace the body with:

  return HashGeneric((void *)JSID_BITS(id));

from mozilla/HashFunctions.h.  (This (void *) wouldn't be necessary if you added a AddToHash(uint32_t hash, uint64_t value) overload to HashFunctions.h.)

::: js/src/jsstr.h
@@ +18,5 @@
>  #include "vm/Unicode.h"
>  
> +/* General-purpose C string hash function. */
> +extern JS_PUBLIC_API(js::HashNumber)
> +JS_HashString(const void *key);

I think you kept this around for the 1 use in ScriptFilenameHasher.  For that, you could use HashString in mozilla/HashFunctions.h instead.  With that change, I think you could kill JS_HashString and make it a non-public extern in jsd.
Comment 8 Nicholas Nethercote [:njn] 2012-07-18 18:45:43 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/61d052e202c8
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-07-18 19:35:12 PDT
http://mozillamemes.tumblr.com/post/21637966463/yes-i-have-a-condition-that-forces-me-to-insert

Sorry, but this caused Windows bustage, so I had to back it out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fcb28092c13

https://tbpl.mozilla.org/php/getParsedLog.php?id=13654674&tree=Mozilla-Inbound

e:/builds/moz2_slave/m-in-w32/build/js/jsd/jshash.cpp(68) : error C2491: 'JS_NewHashTable' : definition of dllimport function not allowed

e:/builds/moz2_slave/m-in-w32/build/js/jsd/jshash.cpp(106) : error C2491: 'JS_HashTableDestroy' : definition of dllimport function not allowed

e:/builds/moz2_slave/m-in-w32/build/js/jsd/jshash.cpp(138) : error C2491: 'JS_HashTableRawLookup' : definition of dllimport function not allowed

e:/builds/moz2_slave/m-in-w32/build/js/jsd/jshash.cpp(218) : error C2491: 'JS_HashTableRawAdd' : definition of dllimport function not allowed

e:/builds/moz2_slave/m-in-w32/build/js/jsd/jshash.cpp(248) : error C2491: 'JS_HashTableAdd' : definition of dllimport function not allowed

e:/builds/moz2_slave/m-in-w32/build/js/jsd/jshash.cpp(270) : error C2491: 'JS_HashTableRawRemove' : definition of dllimport function not allowed

e:/builds/moz2_slave/m-in-w32/build/js/jsd/jshash.cpp(288) : error C2491: 'JS_HashTableRemove' : definition of dllimport function not allowed

e:/builds/moz2_slave/m-in-w32/build/js/jsd/jshash.cpp(304) : error C2491: 'JS_HashTableLookup' : definition of dllimport function not allowed

e:/builds/moz2_slave/m-in-w32/build/js/jsd/jshash.cpp(323) : error C2491: 'JS_HashTableEnumerateEntries' : definition of dllimport function not allowed

e:/builds/moz2_slave/m-in-w32/build/js/jsd/jshash.cpp(418) : error C2491: 'JS_HashTableDump' : definition of dllimport function not allowed

e:/builds/moz2_slave/m-in-w32/build/js/jsd/jshash.cpp(430) : error C2491: 'JS_HashString' : definition of dllimport function not allowed

e:/builds/moz2_slave/m-in-w32/build/js/jsd/jshash.cpp(442) : error C2491: 'JS_CompareValues' : definition of dllimport function not allowed
Comment 10 Nicholas Nethercote [:njn] 2012-08-08 23:36:06 PDT
Second attempt:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c589e94b3e0
Comment 11 Ed Morley [:emorley] 2012-08-09 04:51:41 PDT
https://hg.mozilla.org/mozilla-central/rev/3c589e94b3e0
Comment 12 Luke Wagner [:luke] 2012-08-17 09:53:43 PDT
\o/

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