128861 - pldhash: for the table at address 0x0x8aee608, the given entrySize of 28 probably favors chaining over double hashing.

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[timeless]

I only started noticing this recently, but ... 

xpcom/ds/pldhash.c:                "pldhash: for the table at address 0x%p, the given entrySize"
js/src/jsdhash.c:                "jsdhash: for the table at address 0x%p, the given entrySize"

the js form dates to 3/13/01. the ds form is from a sync done 2/23/02.

Comment 1

[Phil Schwartau]

Formally confirming for consideration -

Comment 2

[timeless]

Attachment: use %p instead of 0x%p because %p is hex and hex does 0x on its own

freebsd's man page for fprintf %p says it's like %#x/%#lx and that x is hex
which does the 0x prefix on its own...

Comment 3

[John Bandhauer]

Please don't cc the whole friggin' world on your trivial bugs.

Comment 4

[Bernd]

I am really in doubt whether that sort of developer evangelism is really
necessary. This printf appears on every page load, so it catches not a rare
event but a usual case. I think the corresponding bugs should rather be filed
and until they are fixed the warning should be disabled. It looks to me like
somebody how runs around and cries 'fire' every ten seconds. Instead of starring
at some nice layout debug messages, I see these messages appearing, which I cant
fix anyway. Running the layout regression tests with these messages is fun ;-).
I helped myself by changing the #ifdef debug to #ifdef debug_brendan. So this is
more a political incorrect rant, that adding debug messages should catch rare
events or who add's these messages should also have the resources to fix them,
especially if only a possible improvement is indicated.

Comment 5

[timeless]

i picked cc list based on patcher+r/sr.  if you aren't interested in bugmail for a bug which is the result of code which you reviewed, then we have a problem.

brendan has been nagging me to cc more people when i file bugs near jseng (or in general), and in this case, I filed it as UNCO, so if we can filter on that (hrm, perhaps we can't,... ), you could have ignored it.

Comment 6

[Brendan Eich [:brendan]]

Timeless, I'll fix this for 1.0 to use a better probably-wasteful threshold.

Bernd, this is not a DEBUG_brendan issue.  I'm not the person who wrote all the
*uses* of pldhash in the codebase, and some of those uses may have an entry size
parameter that's too large for efficient memory utilization compared to a
chained hash table entry scheme.

Patch coming up.  Asa, this is DEBUG only and safe for 1.0.

/be

Comment 7

[Brendan Eich [:brendan]]

Attachment: proposed fix

mstoltz's ClassPolicy subclass of PLDHashEntryHdr is probably over the line.  I
see both tables of that entry type with just 7/16 loads after startup w/o
profile selection, to first browser window.  The analysis in pldhash.h applies
with k=4.  jst, what do you think?

/be

Comment 8

[Brendan Eich [:brendan]]

Mitch, ClassPolicy is too big due to the embedded |mPolicy| PLDHashTable for
space-efficient use of the PLDHashTable in which ClassPolicy is the entry type,
unless you keep the table operating at or above 11/16ths load.  The reason is
that all the unused/removed-sentinel entries in the table suck down 12 words of
storage that could go to other users, if you malloc'd the embedded PLDHashTable
and pointed at it from ClassPolicy.

You could use PL_DHashTableSetAlphaBounds to force the table to operate in a
narrow alpha range, but (a) that can lead to lots of thrashing between two
sizes; and (b) the two tables I saw used during startup never grow above the
minimum size (16 entries), and never have more than 7 entries each.  It looks to
me like the embedded |mPolicy| member would better be a pointer.

If there are only 7 (or <= 10) policies in the table, linear search may be good
enough -- how often are these things looked up?

/be

Comment 9

[Mitchell Stoltz (not reading bugmail)]

I was wondering how long it would take someone to call me on this :)

The tables in question are pretty small - a few may have more than 16 entries,
none more than 32. However, these are looked up very often - on almost every
access to a DOM property. That was the point of this whole exercise, to optimize
DOM property access. I think linear search will hurt us. I could switch
ClassPolicy over to a plhash instead of a pldhash; would you recommend that?

Comment 10

[Brendan Eich [:brendan]]

I talked to mitch about this.  It seems to me we could let this alone, because
the tables are so small; or we could change mPolicy from a PLDHashTable to a
pointer to a PLDHashTable, and allocate only those that are needed.  I think the
latter is better, because Murphy and Parkinson say the tables will grow when we
can least afford them to, and when we do not expect them to, beyond our worst
nightmares. :-)

/be

Comment 11

[Mike Shaver (:shaver emeritus)]

Comment on attachment 72451 [details] [diff] [review]
proposed fix

sr=shaver.

Comment 12

[Mitchell Stoltz (not reading bugmail)]

I'm going to do what Brendan suggested, and I will add a patch to this bug. It
doesn't matter to me whether we check in the patch above or not.

Comment 13

[Brendan Eich [:brendan]]

Ok, the patch to jsdhash.c/pldhash.c is subsumed in the patch for bug 120953, so
I'm handing this bug off to mstoltz.

/be

Comment 14

[Asa Dotzler [:asa]]

Comment on attachment 72451 [details] [diff] [review]
proposed fix

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 15

[Asa Dotzler [:asa]]

Comment on attachment 72451 [details] [diff] [review]
proposed fix

oops, didn't mean to approve that. sorry for the spam.

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 72451 [details] [diff] [review]
proposed fix

Already checked in.

/be

Comment 17

[Mitchell Stoltz (not reading bugmail)]

Fixed (with a patch to caps from bug 131025, not the patch above). Any further
PLDHash warnings in the code (I see a few) are not coming from CAPS.

Comment 18

[Phil Schwartau]

timeless, would you mind verifying this one for me? Thanks -

Comment 19

[timeless]

vrfy they're all gone