Code review comment for lp:~mnordhoff/loggerhead/statictuples

Revision history for this message
Matt Nordhoff (mnordhoff) wrote :

So, I haven't worked on this in a long time. In my opinion, this is
what's blocking it (I'm probably forgetting some things, though):

* The disk caching code has to convert between StaticTuples and normal
tuples, which is ugly and quite slow.

* For people without C extensions, or with an old version of bzr,
_static_tuple_py adds some overhead. It's not much, but Loggerhead has a
frightening number of tuples, so it could be a problem.

* I'm not 100% sure StaticTuple's intern cache (a SimpleSet or dict) is
entirely thread-safe. IIRC the answer was "the GIL should handle it",
but I _did_ once have a crash from some sort of bizarre corruption.

* The interning. Honestly, interning hurts my brain. I'm not sure what
all of the consequences are. In particular, _static_tuple_py's cache
never, ever removes things, so it could get quite large. Plus, some
other things could probably be interned (e.g. revno strings).

* There are some TODO comments about minor things that should either be
removed or actually fixed.
--
Matt Nordhoff

« Back to merge proposal