Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla24
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Assignee

Description

6 years ago
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.
Assignee

Comment 1

6 years ago
I suppose we could rename the files to xpchash.{h,cpp} and the types to
XPCHash*, but it doesn't seem worth the effort.
Attachment #764529 - Flags: review?(luke)
Attachment #764529 - Flags: review?(bobbyholley+bmo)
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.
Assignee

Comment 3

6 years ago
(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!
Yeah, that would probably be easier to do than convert them to more "modern" hash tables...
Assignee

Updated

6 years ago
Attachment #764529 - Attachment is obsolete: true
Attachment #764529 - Flags: review?(luke)
Attachment #764529 - Flags: review?(bobbyholley+bmo)
Assignee

Updated

6 years ago
Summary: Move jsdhash.{h,cpp} from js/src/ to js/xpconnect/src/, to reduce its exposure → Remove jsdhash.{h,cpp}
Assignee

Comment 5

6 years ago
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.
Attachment #764554 - Flags: review?(bobbyholley+bmo)
Assignee

Comment 6

6 years ago
Now even mintier!
Attachment #764556 - Flags: review?(luke)
Comment on attachment 764556 [details] [diff] [review]
(part 2) - Remove jsdhash.{h,cpp}.

weeeeee
Attachment #764556 - Flags: review?(luke) → review+
Assignee

Updated

6 years ago
Duplicate of this bug: 783820
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! :-)
Attachment #764554 - Flags: review?(bobbyholley+bmo) → review?(continuation)
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
Attachment #764554 - Flags: review?(continuation) → review+
I know next to next to nothing about hash tables, but this should be okay, assuming PLDhash and JSDhash haven't diverged too much.
Assignee

Comment 13

6 years ago
> I know next to next to nothing

I see what you did there.
https://hg.mozilla.org/mozilla-central/rev/65e7eb47b5a2
https://hg.mozilla.org/mozilla-central/rev/5900380df45d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24

Updated

6 years ago
Depends on: 902062
You need to log in before you can comment on or make changes to this bug.