Item Notification - Code Review

• Host: Bryan
• Date: Monday April 17th, 2006 @ 2pm
• Where: Whoville

Please review the problem and code patch on Bugzilla Bug 5547 prior to the talk.

Agenda

• Explanation of the problem (5 mins)
  • item watchers, transient & persistent
  • how the DV uses them today

• Strategy for the solving the problem (5 mins)
  • centralized management of transient item watchers
  • notification through existing onItemChanged method

• Implementation of the solution (15 mins)

• Discussion
  • Stats (min, avg, max):
    • Before
      • Runtime for functional tests: 89.89s, 90.89s, 93.37s
      • Time spent in setattr: 260ms, 263ms, 269ms
    • After
      • Runtime for functional tests: 89.69s, 90.83s, 92.77s
      • Time spent in setattr: 175ms, 178ms, 186ms
  1. Any reason not to convert collection notifications to be transient as well? Do we benefit from persistence of collection notifications when the blocks aren't rendered?
  2. Should the dictionary keys used in watchingItems be items instead of their UUIDs, or should the values in the sets be UUIDs of blocks instead of blocks themselves? (I don't know which is 'right', and just noticed that I did it inconsistently.)
  3. Is it right to use the same notification method for collection and item notifications, even though the parameter lists are different? (Currently, my item-notification subscribers don't even look at the parameter lists: they just unconditionally resynchronize.)

Results

We had this talk today: JohnAnderson, JeffreyHarris, PhilippeBossut, and BryanStearns.

In answer to the questions above:

1. Yes, we should convert collection notifications to work transiently. (I plan on tackling this work after I get the other changes related to this code done... or possibly during alpha3 if I can't do it by the alpha2 deadline.)
2. Though it's a little strange to use UUIDs of items as keys in one part of the "watchingItems" structure, and use block references deeper in the structure, we agreed that both uses are appropriate: having the top-level keys be UUIDs instead of their items shortens the lookup process upon notification (because onWatchNotification gets a UUID from the repository); using UUIDs instead of actual block refs in the attribute sets wouldn't really add anything. (This is where question #5 arose: see below.)
3. It's not a problem to use the same notification method for collection and item notifications; the parameter lists are really the same (an operation string and a tuple of additional data).

New issues/questions/feedback that came up during the discussion:

1. John suggested that some of the supporting mechanisms currently in DetailSynchronizedAEBlock could be pushed down into Block to make this process more automated (watchingForChanges and stopWatchingForChanges, and the calls to them from render and onDestroyWidget).
2. The mechanism keeps block references in the depths of the watchingItems dictionary - will this cause problems with the repository's item unloading mechanism? No: I talked to Andi afterward about what prevents a repository item from being unloaded, since I had the idea in my head that some mechanism prevents rendered blocks from being unloaded. Andi explained that the repository uses reference counting; this includes references between python objects. Because a rendered block is referenced by a pointer from its widget's blockItem attribute, the block won't get unloaded.
3. We discovered a how-could-this-ever-have-worked spot where I was unmapping a potentially-proxied item by doing item = getattr(item, 'getProxy', item)... This should've been item = getattr(item, 'proxiedItem', item). (I mention this here just to remind myself that code reviews are also useful for catching stupid mistakes :-), in addition to the more-substantive issues we discussed.)
4. In a couple of places, I was being extra-clever in my Python, and got good counsel from Jeffrey and John:
   • In one place, I wanted to iterate over a list attribute that might not be present; I was doing something like:

     
       optList = getattr(obj, 'optList', None)
       for opt in optList or ():
         ...
     

     That "or ()" wasn't clear - it was meant to provide an empty list to (not) iterate over if the optList attribute wasn't present on obj. They suggested that it was clearer just to use the empty tuple as the default value in getattr, since the cost of creating a tuple is minimal -- and this also gets rid of the intermediate variable:

     
       for opt in getattr(obj, 'optList', ()): 
         ...
     

   • In another place, Jeffery observed an idiom he's used: if you've got a dictionary-whose-values-are-lists, adding an entry could look like this:

     
       if not d.has_key(someKey):
         d[someKey] = [ aValue ]
       else:
         d[someKey].append(aValue)
     

     That's fine, but there's a couple of extra hash lookups involved. I'd done this instead:

     
       d.setdefault(someKey, []).append(aValue)
     

     ... which is weird looking at first, mostly because 'setdefault' is a horrible name for the dict method that taks a key and a default parameter, checks to see if that key exists, returns the existing value if it does, or sets the default parameter and returns that if it doesn't. We agreed that this code is good as-is, but since it's not clear, an additional comment would be beneficial.

(All in all, a very useful review - thanks to everyone who participated. ...Bryan)

-- PhilippeBossut - 14 Apr 2006