884649 - Remove jsdhash.{h,cpp}

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

JSDHash is no longer used within SpiderMonkey;  it's only used in XPConnect.  I
suggest we move it into js/xpconnect/, to reduce its exposure. 

There's precedent for this:  I moved jshash.{h,cpp} from js/src/ to js/jsd/ in
bug 647367, and that left a fresh minty flavour in everybody's mouths.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Move jsdhash.{h,cpp} from js/src/ to js/xpconnect/src/, to reduce its exposure.

I suppose we could rename the files to xpchash.{h,cpp} and the types to
XPCHash*, but it doesn't seem worth the effort.

Comment 2

[Andrew McCreight [:mccr8]]

I wrote some patches to convert these to normal browser hash tables, but I deemed them unreviewable as they were large and tedious. ;)  That was in bug 781589.

Comment 3

[Nicholas Nethercote [inactive]]

(In reply to Andrew McCreight [:mccr8] from comment #2)
> I wrote some patches to convert these to normal browser hash tables, but I
> deemed them unreviewable as they were large and tedious. ;)  That was in bug
> 781589.

From a quick look those patches look ok to me...

That does give me another idea, though:  convert all those xpconnect JSDHashes to PLDHashes and remove jsdhash.{h,cpp} altogether!

Comment 4

[Andrew McCreight [:mccr8]]

Yeah, that would probably be easier to do than convert them to more "modern" hash tables...

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Used PLDHash instead of JSDHash in xpconnect.

This is mostly trivial, just replacing "JS" with "PL" in lots of places.

The one exception is that JSDHashMatchEntry's return type is JSBool, but
PLDHashMatchEntry's is bool.  So I had to modify a few Match() methods
accordingly.  This was easy;  they were already returning true/false rather
than JS_TRUE/JS_FALSE.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Remove jsdhash.{h,cpp}.

Now even mintier!

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 764556 [details] [diff] [review]
(part 2) - Remove jsdhash.{h,cpp}.

weeeeee

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 764554 [details] [diff] [review]
(part 1) - Used PLDHash instead of JSDHash in xpconnect.

I know next to nothing about hash tables. But from comment 2, it sounds like Andrew does! :-)

Comment 10

[Andrew McCreight [:mccr8]]

Comment on attachment 764554 [details] [diff] [review]
(part 1) - Used PLDHash instead of JSDHash in xpconnect.

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

thanks

::: js/xpconnect/src/XPCMaps.cpp
@@ +274,5 @@
>  {
>      size_t n = 0;
>      n += mallocSizeOf(this);
>      // The second arg is NULL because this is a "shallow" measurement of the map.
> +    n += mTable ? PL_DHashTableSizeOfIncludingThis(mTable, NULL, mallocSizeOf) : 0;

nullptr please

::: js/xpconnect/src/xpcprivate.h
@@ +85,5 @@
>  #include <stdarg.h>
>  #include <math.h>
>  #include "xpcpublic.h"
>  #include "jsapi.h"
> +#include "pldhash.h"

please move this after jsprf to preserve some kind of local alphabetical ordering

Comment 11

[Andrew McCreight [:mccr8]]

I know next to next to nothing about hash tables, but this should be okay, assuming PLDhash and JSDhash haven't diverged too much.

Comment 12

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/65e7eb47b5a2
https://hg.mozilla.org/integration/mozilla-inbound/rev/5900380df45d

Comment 13

[Nicholas Nethercote [inactive]]

> I know next to next to nothing

I see what you did there.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/65e7eb47b5a2
https://hg.mozilla.org/mozilla-central/rev/5900380df45d