Tk Library Source Code

View Ticket
Login
Ticket UUID: 1528614
Title: Using the C version of struct::tree crashes Tcl 8.4.13
Type: Bug Version: None
Submitter: hgiese Created on: 2006-07-25 20:28:10
Subsystem: struct :: tree Assigned To: andreas_kupries
Priority: 9 Immediate Severity:
Status: Closed Last Modified: 2006-07-28 04:21:32
Resolution: Fixed Closed By: andreas_kupries
    Closed on: 2006-07-27 20:44:54
Description:
Hello out there,
all of a sudden I got the error message 'malformed
bucket chain in Tcl_DeleteHashEntry'.
The only significant change I could think of was the
introduction of tcllib's tree module, so I started
investigating a bit.
The attached file demonstrates the bug.

I only observed it on Win98 (because my XP machine runs
Tcl 8.4.9) and on 8.4.9 the bug doesn't happen. Also,
it is limited to the C(ritcl) version - the pure Tcl
version is clean. So I got a workaround and am still a
happy camper.

Best regards and keep up the good work
Helmut Giese
User Comments: andreas_kupries added on 2006-07-28 04:21:32:
Logged In: YES 
user_id=75003

Hm. Could actually have a huge impact on runtime in
pathological cases. For the better. Consider a very large
tree, several thousand nodes, all auto-generated.

I start from the bottom (node1) to find and fill any gaps.
If there are none I have to iterate over the thousands of
names to to find the max. Takes a long time.

The tcl version is most impacted by this. The C version is
way faster, but even so this only means that the point where
such the iteration takes a noticeable amount of time is
pushed to higher numbers of nodes, not that the problem goes
away.

So, doing some precomputation when desrializaing makes sense.

On the other hand, this precomputation has to 'string match'
or 'regexp' each node name and see if it is of the form
'nodeFOO', FOO an integer number. This is not cheap in
runtime I believe.

Well, in the end what we need is to benchmark this and see
how the various cases behave and what the big-O constants are.


--
And thanks again for the concise test case. This made the
debugging a snap.

andreas_kupries added on 2006-07-28 03:44:54:
Logged In: YES 
user_id=75003

Fix committed to head.
t_newnodename extended to skip existing nodes while
generating names.
tn_new extended with check for duplicate name, panic. This
latter ensures that, should we have other places which can
cause generation of dup names, we panic early and with a
good error message, and with damaged hashtable during
destruction.

hgiese added on 2006-07-27 17:12:48:
Logged In: YES 
user_id=522169

Hi Andreas,
just a last thought:
On deserialising you are bound to look at each node. Why not
note 'in passing' the highest "auto generated name" and when
thru using it to initialise the 'unique number'?
This should be negligible wrt changes in the code and
run-time impact.
Ok, I'll forget about it now and leave the rest to you.
Greetings
Helmut

hgiese added on 2006-07-27 16:22:45:
Logged In: YES 
user_id=522169

Hi Andreas,
I have been thinking a bit about this issue.
You can use the same approach the Tcl version uses - that is
on every node creation you keep incrementing the counter and
compare the resulting name against all existing nodes. This
is an O(n) operation for each newly created node.
How about the following approach: On deserialisiation you
check for the highest node index present and set your
'unique number' from there. This is also an O(n) operation
but it happens only once.

Why aren't you on vacation at this time of the year?
Greetings from a very hot place
Helmut

andreas_kupries added on 2006-07-27 01:19:05:
Logged In: YES 
user_id=75003

This can actually be seen as two problems.

(1) The auto-generation of node-names is out of whack for a
tree loaded from a serialization. It creates node-names
which are already present.

(2) The code for the destruction of tree is not able to
handle a tree with duplicate node names.

The latter is the crash. However (1) is the real problem.
All parts of the tree implementation assume that node names
are unique. all the methods for rename, etc. check that the
result is not violating this assumption.

Ok, the plan: Make the destruction more robust against dup
names, if possible. By detecting an already deleted node.
Panic with a proper message. Then fix the generation of dup
names by the C impl. after a tree was loaded from file. Put
this example into the testsuite.

andreas_kupries added on 2006-07-27 01:07:45:

File Added - 186333: valgrind.txt

Logged In: YES 
user_id=75003

I can confirm the same crash with Tcl 8.4.CVS and
struct::tree critcl. (Aborted with a "malformed bucket chain
in Tcl_DeleteHashEntry").

And I managed to get some more information where the
problems by using valgrind.

Hm. I wonder if I can manage to get more by compiling
struct::tree with symbols.

Ok, than you very much Helmut for investigating this so far,
i.e. making the problem reproducible, and also reducing it
to such a simple and small script.

hgiese added on 2006-07-26 03:28:11:

File Added - 186221: treetst2.tcl

Attachments: