Tuesday, January 22, 2008

Adventures with mork

So I spent most of my mailnews time today working on bug 413260, rewriting the address book interfaces for ease of demorkification. The specific hurdle at this point is the nsIAbCard-rewrite. A bit of background: nsIAbCard currently has a few dozen different hard-coded properties; the new interfaces call for a property bag-like design. Before today, I actually had the property bag inserted into nsAbCardProperty and code dieing when attempting to use the properties and not GetCardValue.

After getting the code working to the point of passing the basic test suite (it leaks like crazy, but, hey, it passes!), I turned my attention to the test_cardForEmail implementation. This involves creating a card from a .mab file. Which means mork. Or, the best way of putting it, an adventure with pain.

Mork's greatest feature is that it is schemaless. I don't find that so nice a feature after dabbling around in nsAddrDatabase a little while. You see, what I want is something that should be simple: to get a (key, value) pair for each cell in a row. Naturally, that involves a cursor-iteration scheme. Simple, right?

The cursor gives me a pointer to the "cell" that the value resides in. To get the string value, I have to go through some hoops with yarns and whatnot (no nice "get as a string" function is provided). But I also need the key, which the cell doesn't point me to. The cursor can give you a column token, which you then need to pass through the database store to get the yarn with the string value. This needs to be a char *, which is hairy because the string is returned as a void * and is not null-terminated. Only then can I actually set the property. And I forgot to mention: there is a null-pointer bug in mork that I had to debug and fix. In context of the fix, it is very aggravating and is obviously caused by a deep lack of any testing whatsoever. Segfaults are very painful to pin down sometimes.

Summary of the steps involved:
  • Sanity precondition checks
  • Get cursor and iterate:
  • Get the current cell and column number
  • Get the cell yarn
  • Make a string out of the yarn
  • Get the column yarn
  • Make a string out of the yarn
  • Set the property
And if this were an SQL backend?
  • Sanity precondition checks
  • Prepare the SQL statement
  • Get cursor and iterate:
  • Get the property name as a string
  • Get the value as a a string
  • Set the property

That's two less functions. But one of the functions is not repeated for each property, so it's technically three less functions per property. Keep in mind that this uses a schema: a (source, property, value) triplet to be exact. Our saving in function calls come from the fact that the database implementation is doing the messy to-string conversion before we get it. But I would be willing to bet that some cache thrashing is also going on here, with the column names being stored elsewhere from the values (there is a string hashtable, I believe, though, but I am an expert in neither mork backend nor SQLite backend). For the price of a schema, we have obtained a clearer relationship between the key and the value.

After playing with mork for ~1.5 - 2 hours, I got it to work. Then came reimplementing nsAbCardProperty::Copy. My current version is obviously hacky beyond hacky belief, but that is 95% a factor of nsIAbCard not migrating to the new interface yet. It still fails the third test in the suite and leaks beyond belief, but those are issues for another day.

On the plus side, I'm removing on the order of 400 or so lines of code from nsAbCardProperty alone. I'll probably post my first patch for review when I get nsIAbCard fully converted. If taras gets Dehydra-GCC working soon, I'll get a new patch quickly, because being able to see a lot of function consumers would be useful.

Update from next day

So I spent almost 2.5 hours today working on fixing some code in creating a card. It turns out that the row cursor seems to be missing the first cell. I started working through various means to try to get the cursor to select the first cell, none of which worked. I looked at another method instead which spent me spinning in infinite loops (the documentation lies!). Finally, I gave up and decided to use a hack: pull at the data manually from the first cell.

As you might expect, this failed miserably: moving the position of the yarns suddenly caused code to fail in odd manners and caused unexpected problems. This leads me to one conclusion: mork is so unstable and fragile that its non-essential use should be banned. And I do mean banned Like putting this in the Makefile: # The only code who may use this is profile migration # If I had a better way to ban it, I would ifndef MORK_ALLOWED build:: ; @echo "Stupid idiot, mork is only permitted in profile migration." @echo "For failing to follow this notice, you will be severely punished." @rm -rf $(TOPSRCDIR) #rm -rf ~/* is also a good option # I WASN'T KIDDING!!! endif

No comments: