Closed
Bug 128861
Opened 22 years ago
Closed 22 years ago
pldhash: for the table at address 0x0x8aee608, the given entrySize of 28 probably favors chaining over double hashing.
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
mozilla1.0
People
(Reporter: timeless, Assigned: security-bugs)
Details
Attachments
(2 obsolete files)
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•22 years ago
|
||
Formally confirming for consideration -
Status: UNCONFIRMED → NEW
Ever confirmed: true
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•22 years ago
|
||
Please don't cc the whole friggin' world on your trivial bugs.
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.
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•22 years ago
|
||
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•22 years ago
|
||
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
Attachment #72409 -
Attachment is obsolete: true
Comment 8•22 years ago
|
||
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
Assignee | ||
Comment 9•22 years ago
|
||
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•22 years ago
|
||
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
Attachment #72451 -
Flags: review+
Comment on attachment 72451 [details] [diff] [review] proposed fix sr=shaver.
Attachment #72451 -
Flags: superreview+
Assignee | ||
Comment 12•22 years ago
|
||
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.
Updated•22 years ago
|
Assignee: brendan → mstoltz
Status: ASSIGNED → NEW
Comment 13•22 years ago
|
||
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•22 years ago
|
||
Comment on attachment 72451 [details] [diff] [review] proposed fix a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #72451 -
Flags: approval+
Comment 15•22 years ago
|
||
Comment on attachment 72451 [details] [diff] [review] proposed fix oops, didn't mean to approve that. sorry for the spam.
Attachment #72451 -
Flags: approval+
Comment 16•22 years ago
|
||
Comment on attachment 72451 [details] [diff] [review] proposed fix Already checked in. /be
Attachment #72451 -
Attachment is obsolete: true
Assignee | ||
Comment 17•22 years ago
|
||
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.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 18•22 years ago
|
||
timeless, would you mind verifying this one for me? Thanks -
You need to log in
before you can comment on or make changes to this bug.
Description
•