JSDHashTableOps.initEntry patch now crashes @ chrome reg.

VERIFIED FIXED in mozilla0.9.1

Status

()

P1
blocker
VERIFIED FIXED
18 years ago
17 years ago

People

(Reporter: shaver, Assigned: brendan)

Tracking

({js1.5, smoketest})

Trunk
mozilla0.9.1
js1.5, smoketest
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

I've got a patch for people to peek at while I build tests.
Created attachment 29841 [details] [diff] [review]
add JS_DHASH_RAWADD and documentation, and fix missed jsdhash->pldhash in plify_jsdhash.sed
Brendan says he doesn't like this approach, and also says that he has a
counter-proposal.
(Assignee)

Comment 3

18 years ago
That patch can't work: JS_DHASH_LOOKUP doesn't set entry->keyHash, so the random
logic based on op that loads from ((JSDHashEntry *)key)->keyHash will load 0.

I don't see a need for raw-add, unless there is a raw-lookup that returns the
keyHash it computes.  But even then, I think code that wants to look, but not
touch, and then add only if the lookup returned a free entry, should instead
just add.

Given the all-zeroes state of entry storage, after adding a brand new entry,
only keyHash will be non-zero, and the caller of JS_DHashTableOperate can
inspect other members of its JSDHashEntryHdr extension struct looking for a zero
where no initialized entry would have a zero.  If zero is ambiguous, there is no
issue.  If you need to construct (C++) or otherwise initialize an entry before
keyHash is set, the initEntry hook in the next patch can be used.

Shaver, you buy this?

/be
(Assignee)

Comment 4

18 years ago
Created attachment 30733 [details] [diff] [review]
my proposed fix (some whitespace and similar nit-picking in addition to initEntry and ChangeTable diffs)
(Assignee)

Comment 5

18 years ago
Forgot to say that adding an entry for key, where key already has an entry, does
no harm.  That finishes the case analysis: cases that want to lookup and add iff
not-found should just add.

To ensure that initEntry is not called more than once per add (in the case where
the table needs to grow), ChangeTable now takes a JSDHashEntryHdr **findEntry
in/out param, which may be null if no entry needs to be tracked across the copy
to the grown table.

/be
Looks pretty good.  Can I convince you to document that use case in jsdhash.h's
JSDHashTableOperate comment?

sr=shaver.
Assignee: shaver → brendan
(Assignee)

Updated

18 years ago
Status: NEW → ASSIGNED
Keywords: js1.5, mozilla0.9.1
Priority: -- → P1
Target Milestone: --- → mozilla0.9.1

Comment 7

18 years ago
So the initEntry stuff is added to support placement new, right? What is the
sequence of events that occurs when you rehash the table? I'm just trying to
think how a C++ object derived from PLDHashEntryHdr would be implemented.

  allocTable    allocate raw bytes with malloc, no ctors run.
  freeTable     free raw bytes with free, no dtors run.
  initEntry     call placement new using default ctor
  moveEntry     call placement new using copy ctor, run dtor
                on old entry? (or will that be clearEntry'd?)
  clearEntry    run dtor which should call PLDHashClearEntryStub()
  finalize      no-op

Does that seem correct? If so, I understand what's going on, and r=waterson.
(Assignee)

Comment 8

18 years ago
  allocTable    allocate raw bytes with malloc, no ctors run.
  freeTable     free raw bytes with free, no dtors run.

Right and right.

  initEntry     call placement new using default ctor

Check-a-roonie.

  moveEntry     call placement new using copy ctor, run dtor
                on old entry? (or will that be clearEntry'd?)

Right the first time -- moveEntry "moves", it does not "copy".  Running just the
copy ctor but not the dtor would be a cp, and need a clearEntry "rm" to ape mv,
but the pldhash code does not call clearEntry on the old table entry.  Move it!

  clearEntry    run dtor which should call PLDHashClearEntryStub()

Almost -- you can use PL_DHashClearEntryStub if you've nothing better to do, but
if you specialize your own clearEntry, no need to call the stub.  Stub here does
not mean "base class impl, always delegate to me last".

  finalize      no-op

Right.

I'll comment all this better, stick in a final patch, and go when it's ready.

/be
(Assignee)

Comment 9

18 years ago
Created attachment 34512 [details] [diff] [review]
final patch, per review comments
(Assignee)

Updated

18 years ago
Summary: Need JS_DHASH_RAWADD to avoid hash recomputation → Need JSDHashTableOps.initEntry hook
(Assignee)

Comment 10

18 years ago
Fix checked in.

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 11

18 years ago
Marking Verified - 
Status: RESOLVED → VERIFIED

Comment 12

18 years ago
linux CVS is crashing all over the place for me now:
nsCacheEntryHashTable::GetKey () from dist/bin/components/libnkcache.so
Will clobber.
(Assignee)

Comment 13

18 years ago
I am a bucket-head.  Clients who use initEntry work fine when adding and the
table grows, but old-style clients die.  Better patch coming up.

/be
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

18 years ago
Status: REOPENED → ASSIGNED
(Assignee)

Comment 14

18 years ago
Created attachment 34712 [details] [diff] [review]
much simpler fix, I am filled with shame for not writing this up front!
(Assignee)

Comment 15

18 years ago
My stepwise refinement of JS_DHashTableOperate went awry.  Once I had factored
ChangeTable out of inline code in JS_DHashTableOperate, I should have simplified
the JS_DHASH_ADD and _REMOVE cases to call SearchTable and ChangeTable in the
right order for each case.

gordon, waterson, gimme r= and sr= love.

/be
(Assignee)

Comment 16

18 years ago
Created attachment 34715 [details] [diff] [review]
let's try that again (ignore last patch please)

Comment 17

18 years ago
I'm crashing here at startup during chrome registry; your patch fixes that. 
I'm almost sure this will be a smoketest blocker. [s]r=waterson
Severity: normal → blocker
Keywords: smoketest

Updated

18 years ago
Summary: Need JSDHashTableOps.initEntry hook → JSDHashTableOps.initEntry patch now crashes @ chrome reg.
(Assignee)

Comment 18

18 years ago
waterson: were you up to date in xpcom/ds before trying the latest patch?  If
not, that might account for the crash during chrome registry.  What was the
stack backtrace?

/be

Comment 19

18 years ago
Brendan, I like it; less "clever" and easier to follow (I always paused a moment 
to remember what biasedDeltaLog2 meant).  If you've run this with the cache, then 
r=gordon.

Comment 20

18 years ago
note to smoketest watchers, we will open the tree for this, but keep smoketest
status
and will hold the tree tomorrow for this if needed.  -mcafee & jj
(Assignee)

Comment 21

18 years ago
My patch works for me, I breakpoint and run mozilla and see lots of people using
pldhash these days!

/be
(Assignee)

Comment 22

18 years ago
Waterson got a libpr0n bug that blocked the tree opening, diagnosed enough to
bounce to pavlov, which gave me an opening to check in the fix.  /me slinks away
now with his head hung low

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago18 years ago
Resolution: --- → FIXED
(Assignee)

Comment 23

18 years ago
*** Bug 81283 has been marked as a duplicate of this bug. ***

Comment 24

18 years ago
Marking Verified - 
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.