From: Michael H. <mic...@el...> - 2005-09-22 07:59:43
|
Hi, yesterday I spent a couple of hours with looking into recursive locking. It's not as easy as it looks at first :-( First let me propose a small refactoring: Right now MetadataCollection, Relation and Procedure all inherit from MetadataItem. Why don't we create a new class MetadataItemWithChildren, which will be base class for those three. This would move some of the common code out of MetadataCollection, making the instantiated templated code smaller. It would also be the one place to override lockSubject() and unlockSubject(), which I had to do in all three classes otherwise. Or is there anything speaking against this? The more difficult thing is the management of the update lock counter for children. There is code in MetadataCollection to create new children, but they are always created with the lock counter set to 0, even if the parent has a higher lock count. My preference would be to make the initial lock counter value a parameter to the constructor, but that would need changes in all of the classes. Another way I see is to create protected methods getLockCounter() and setLockCounter(), but that seems really ugly, and opens up a lot of possibilities for trouble and bugs. Any comments, ideas, please? Thanks -- Michael Hieke |
From: Nando D. <na...@de...> - 2005-09-22 08:12:23
|
Michael, M> First let me propose a small refactoring: Right now MetadataCollection, M> Relation and Procedure all inherit from MetadataItem. Why don't we M> create a new class MetadataItemWithChildren, which will be base class M> for those three. This would move some of the common code out of M> MetadataCollection, making the instantiated templated code smaller. It M> would also be the one place to override lockSubject() and M> unlockSubject(), which I had to do in all three classes otherwise. Or M> is there anything speaking against this? again, just one word: Composite. ;-) Have a look at it for insights, nomenclature, etc. M> The more difficult thing is the management of the update lock counter M> for children. There is code in MetadataCollection to create new M> children, but they are always created with the lock counter set to 0, M> even if the parent has a higher lock count. I'm not sure I get it. Is your problem due to the necessity of locking/unlocking single collection items? OTTOMH I'd say that a collection item locks and unlocks its containing collection, and hasn't got a lockcount of its own, but I am missing some details for sure. Ciao -- Nando Dessena http://www.flamerobin.org |
From: Michael H. <mic...@el...> - 2005-09-22 08:34:44
|
Nando, Nando Dessena wrote: > M> First let me propose a small refactoring: Right now MetadataCollection, > M> Relation and Procedure all inherit from MetadataItem. Why don't we > M> create a new class MetadataItemWithChildren, which will be base class > M> for those three. This would move some of the common code out of > M> MetadataCollection, making the instantiated templated code smaller. It > M> would also be the one place to override lockSubject() and > M> unlockSubject(), which I had to do in all three classes otherwise. Or > M> is there anything speaking against this? > > again, just one word: Composite. ;-) > Have a look at it for insights, nomenclature, etc. I know about it. I would rather not make MetadataItem a composite, since we would be mixing stuff. Most of the DBH objects do not have children (Exception, Trigger, ...), while others (Server, Database) contain vectors of other DBH objects and are DBH objects in their own right. MetadataCollection is no "real" DBH object itself. Let's not muddle those concepts. > M> The more difficult thing is the management of the update lock counter > M> for children. There is code in MetadataCollection to create new > M> children, but they are always created with the lock counter set to 0, > M> even if the parent has a higher lock count. > > I'm not sure I get it. Is your problem due to the necessity of > locking/unlocking single collection items? The problem is exactly what I wrote about: MetadataCollection creates children in an unlocked state, even when it is locked itself. Right now it can not even call newitem->lockSubject() in a loop, because its own lock counter is private. And in any case, creating with the correct initial value is surely cleaner than locking it in a loop. > OTTOMH I'd say that a collection item locks and unlocks its > containing collection, and hasn't got a lockcount of its own, but I > am missing some details for sure. Since a collection is a MetadataItem it inherits the lock counter. Not much we can do about it. Not that we would want to, it is necessary to create the new collection item with the correct initial lock count. Thanks -- Michael Hieke |
From: Nando D. <na...@de...> - 2005-09-26 18:06:39
|
Michael, >> again, just one word: Composite. ;-) >> Have a look at it for insights, nomenclature, etc. M> I know about it. I would rather not make MetadataItem a composite, M> since we would be mixing stuff. Most of the DBH objects do not have M> children (Exception, Trigger, ...), while others (Server, Database) M> contain vectors of other DBH objects and are DBH objects in their own M> right. MetadataCollection is no "real" DBH object itself. Let's not M> muddle those concepts. I think I see your point. There are cases in which we might want to do something on a metadata item but not all the items it contains, is that right? Well, although I think it's possible in theory, do we have some real example? I mean, if we restrict the subject to locking, why wouldn't that work? M> The problem is exactly what I wrote about: MetadataCollection creates M> children in an unlocked state, even when it is locked itself. Right now M> it can not even call newitem->lockSubject() in a loop, because its own M> lock counter is private. And in any case, creating with the correct M> initial value is surely cleaner than locking it in a loop. Agreed. Well I guess I'd just use a friend definition and be done with it then. Ciao -- Nando Dessena http://www.flamerobin.org |
From: Michael H. <mg...@gm...> - 2005-09-26 20:09:29
|
Nando, Nando Dessena wrote: >> I know about it. I would rather not make MetadataItem a composite, >> since we would be mixing stuff. Most of the DBH objects do not >> have children (Exception, Trigger, ...), while others (Server, >> Database) contain vectors of other DBH objects and are DBH objects >> in their own right. MetadataCollection is no "real" DBH object >> itself. Let's not muddle those concepts. > > I think I see your point. There are cases in which we might want to > do something on a metadata item but not all the items it contains, is > that right? Well, although I think it's possible in theory, do we > have some real example? I mean, if we restrict the subject to > locking, why wouldn't that work? no, I'd say I know why you would like the Composite pattern being implemented. I would like it too, in fact. (Under different circumstances, see below.) But I was thinking about the following: If there was a real composite implementation in FR we would have a common base class, and all nodes of the DBH tree, whether leaf nodes or not, would inherit from this base class. It would (among others) have the methods getChild(size_t index) and size_t getChildrenCount(), so one could simply walk the tree from the root node, iterating over the child nodes. But when you apply this to current DBH objects, for example the current Database class, things get difficult. We would no longer have an interface with methods like: MetadataCollection<Generator>::const_iterator generatorsBegin(); MetadataCollection<Generator>::const_iterator generatorsEnd(); MetadataCollection<Domain>::const_iterator domainsBegin(); MetadataCollection<Domain>::const_iterator domainsEnd(); MetadataCollection<Table>::const_iterator tablesBegin(); MetadataCollection<Table>::const_iterator tablesEnd(); Instead one of the objects returned by Database::getChild() would be a collection of generators, one a collection of domains, and so on. These would all have to be real classes instead of templates, since some code should then be moved into those (specialized) classes. This would most probably make the code base larger, but clearer and more extensible. It would also take *a lot* of work. Most of it design work, because on one hand everything should be polymorphic (no more check for node types), but OTOH one would not want to have all methods as stubs in the base class. Designing such a class hierarchy isn't easy. It's gratifying though, when the hard work is over, and everything looks and works great :-) But since we haven't the time, and it would not create new functionality for FR - such a major refactoring is out of the question, right now. And we would *have* to do this The Right Way(TM), any smaller attempt would just muddle things. IMHO, of course. >> The problem is exactly what I wrote about: MetadataCollection >> creates children in an unlocked state, even when it is locked >> itself. Right now it can not even call newitem->lockSubject() in a >> loop, because its own lock counter is private. And in any case, >> creating with the correct initial value is surely cleaner than >> locking it in a loop. > > Agreed. Well I guess I'd just use a friend definition and be done > with it then. Ok. I did not touch this area though, because I think that Milan's idea with the single DBH lock has a lot of merit. Did you think about it some more, Milan? Thanks -- Michael Hieke |
From: Milan B. <albis@EUnet.yu> - 2005-09-27 12:31:52
|
Michael Hieke wrote: > Ok. I did not touch this area though, because I think that Milan's idea > with the single DBH lock has a lot of merit. Did you think about it > some more, Milan? I have, and I'm still for it. The fact that we "might want to lock some object only" one day, doesn't stike me as a big drawback. We can still implement per object locking once (if ever) we need it. IMO, of course. -- Milan Babuskov http://www.flamerobin.org |
From: Nando D. <na...@de...> - 2005-09-27 16:39:58
|
Michael, Milan, M> Ok. I did not touch this area though, because I think that Milan's idea M> with the single DBH lock has a lot of merit. I can't see big advantages/disadvantages either way, so I'd stick with the "natural" approach of locking single items. Delegating another entity to keep a list of locked objects, which is basically putting object state outside of the object itself, might have its merits but the move needs to be justified. If your only problem is how to code the ability to create an object in a locked state, then I believe that a friend constructor or other suitable machanism will do and you won't be regretting it. IOW I can't see sufficient reasons to look elsewhere. OTOH Milan's idea may well turn out to be the best invention since sliced bread, but it also has the potential to turn into a corner in the future. So I'd prefer the more natural and flexible solution, all other factors (like implementation difficulty) being equal. Ciao -- Nando Dessena http://www.flamerobin.org |
From: Milan B. <albis@EUnet.yu> - 2005-09-27 17:45:14
|
Nando Dessena wrote: > I can't see big advantages/disadvantages either way, so I'd stick > with the "natural" approach of locking single items. Delegating > another entity to keep a list of locked objects, which is basically > putting object state outside of the object itself, might have its > merits but the move needs to be justified. For mere locking we don't need to "keep a list". We need to keep a list if we wish to integrate the "invalidation stuff" into locking, and make all that a single mechanism. The real question is do we want that. Here's the idea. Let's say we run into statement that changes table's description and it needs to be re-loaded. The table would ask for lock and proceed to update the description. The lock manager would either give a lock, or not. In case it does not give a lock, it would put table "on hold". Later, when lock is released it should "call" the table, and let it reload the description from database. The information that description needs to be loaded should be in the table object itself, not in lock manager. As you can see, my idea could be implemented even without the list of object. Once global lock is released, just call each object and let it update itself if needed. Looking at all this, perhaps we need a 3-state flags for objects' properties which load at run time. Currently we have something like: bool descriptionLoadedM. Maybe we should have flag with three states: notLoaded, loaded, needsReload. The "needsReload" would mean that that part of object is "invalid" and needs to be loaded when lock is released. The difference versus "notLoaded" is in the fact that properties that are "notLoaded" yet were not needed, so no need to reload. Also, releasing a lock on object that has some "needsReload" properties would mean that notify() needs to be called (observer pattern). > So I'd prefer the more natural and > flexible solution, In case we go for that, I have few ideas: - checking for lock should check all parents up to the root node - releasing the lock should call update() for the current object + all child objects recursively - 3-state flags should be used anyway I'm still not sure which method is better, as I can't anticipate what the future needs will be. -- Milan Babuskov http://fbexport.sourceforge.net http://www.flamerobin.org |
From: Michael H. <mg...@gm...> - 2005-09-27 19:32:52
|
Milan, Nando, Milan Babuskov wrote: > Here's the idea. Let's say we run into statement that changes table's > description and it needs to be re-loaded. The table would ask for > lock and proceed to update the description. The lock manager would > either give a lock, or not. In case it does not give a lock, it would > put table "on hold". Later, when lock is released it should "call" > the table, and let it reload the description from database. The > information that description needs to be loaded should be in the > table object itself, not in lock manager. if I'm not mistaken you're mixing the locking stuff with the updating of invalidated data. Unlocking an object should *never* lead to a direct reload of invalidated data. It's a case of "don't call us, we will call you". Let me recap: Invalidating the internal state of an object should cause it to call notifyObservers(). To make this as efficient as possible we want that to happen only when the object is not locked. Thus when it is locked, we need to delay the call. It does not matter whether the lock is global or per-object, this information (whether to call notifyObservers()) has to be per-object. Either in the object itself, or in a list of those objects. I agree with Nando, handling this inside of the object feels more natural. Obvious name of the necessary member field: needsNotifyObserversM. When the object is unlocked, the delayed call to notifyObservers() can be performed. That's all there is to it, no reloading takes place at this stage! If no other object observes this one, then nothing happens. If another object observes, then it will query information from our changed object, in its update() method. It will call our object's getter methods, and only inside of them information will be reloaded, if necessary. This (lazy evaluation) is vital, it's the key to minimal updates. > As you can see, my idea could be implemented even without the list of > object. Once global lock is released, just call each object and let > it update itself if needed. that trade-off would be rather unfortunate. We'd still have a private member to hold information about "reload needed". But instead of the fine-grained locking in the "natural" solution we would now need to traverse the whole DBH tree, to give each object the chance to notify its observers, if necessary. Even if none or only one of them has changed. This I find worse in fact than the solution with the list of objects that need updating. > Looking at all this, perhaps we need a 3-state flags for objects' > properties which load at run time. Currently we have something like: > bool descriptionLoadedM. Maybe we should have flag with three > states: notLoaded, loaded, needsReload. The "needsReload" would mean > that that part of object is "invalid" and needs to be loaded when > lock is released. The difference versus "notLoaded" is in the fact > that properties that are "notLoaded" yet were not needed, so no need > to reload. Also, releasing a lock on object that has some > "needsReload" properties would mean that notify() needs to be called > (observer pattern). There is no real difference between notLoaded and needsReload. See above, you are mixing functionality of class Subject (lock counter, info whether notifyObservers() needs to be called) with functionality of higher level objects (is a certain part of the internal state valid or not, and needs to be reloaded). > In case we go for that, I have few ideas: > > - checking for lock should check all parents up to the root node I don't see much benefit over the other way around. > - releasing the lock should call update() for the current object + all > child objects recursively You mean notifyObservers(), right? Anyway, IMHO it's better to let each object decide for itself what to do after the lock has been released. And propagating the lock counter to all children will take of the recursion you mention. > - 3-state flags should be used anyway I disagree. Thanks -- Michael Hieke |
From: Milan B. <albis@EUnet.yu> - 2005-09-28 13:01:54
|
Michael Hieke wrote: > If another object observes, then it will query information from our > changed object, in its update() method. It will call our object's > getter methods, and only inside of them information will be reloaded, if > necessary. This (lazy evaluation) is vital, it's the key to minimal > updates. Ok, agreed. >> Looking at all this, perhaps we need a 3-state flags for objects' >> properties which load at run time. Currently we have something like: >> bool descriptionLoadedM. Maybe we should have flag with three >> states: notLoaded, loaded, needsReload. The "needsReload" would mean >> that that part of object is "invalid" and needs to be loaded when >> lock is released. The difference versus "notLoaded" is in the fact >> that properties that are "notLoaded" yet were not needed, so no need >> to reload. Also, releasing a lock on object that has some >> "needsReload" properties would mean that notify() needs to be called >> (observer pattern). > > There is no real difference between notLoaded and needsReload. If we don't mix locking with invalidating, then yes, there is no difference. So, let me get this clear: When SQL parser finds a statement, and either a) updates the object directly b) sets the somethingLoadedM flag to false and leave the rest to observer pattern. Yes, that does sound like the most natural and efficient way, so I agree with you. >> - checking for lock should check all parents up to the root node > > I don't see much benefit over the other way around. I'm not sure I understand what you mean by that? >> - releasing the lock should call update() for the current object + all >> child objects recursively > > You mean notifyObservers(), right? Yes. Sorry about that. > Anyway, IMHO it's better to let each > object decide for itself what to do after the lock has been released. > And propagating the lock counter to all children will take of the > recursion you mention. Ok. >> - 3-state flags should be used anyway > > I disagree. I agree with your disagreement :) I take the 3-state flag idea back, unless a strong case for it shows up. -- Milan Babuskov http://www.flamerobin.org |
From: Milan B. <mi...@km...> - 2005-09-22 09:21:56
|
Michael Hieke wrote: > First let me propose a small refactoring: Right now MetadataCollection, > Relation and Procedure all inherit from MetadataItem. Why don't we > create a new class MetadataItemWithChildren, which will be base class > for those three. This would move some of the common code out of > MetadataCollection, making the instantiated templated code smaller. It > would also be the one place to override lockSubject() and > unlockSubject(), which I had to do in all three classes otherwise. Or > is there anything speaking against this? There isn't anything, I even considered to do it, but never got to it. Please, go ahead with it. > The more difficult thing is the management of the update lock counter > for children. There is code in MetadataCollection to create new > children, but they are always created with the lock counter set to 0, > even if the parent has a higher lock count. > My preference would be to make the initial lock counter value a > parameter to the constructor, but that would need changes in all of the > classes. Another way I see is to create protected methods > getLockCounter() and setLockCounter(), but that seems really ugly, and > opens up a lot of possibilities for trouble and bugs. > > Any comments, ideas, please? I dislike the whole per-whatever locking idea. If you ask me, I would use a single lock that holds the entire DBH. The lock itself would build a list of objects that tried to update themselves. When lock is completely released, it would call update() on each object from that list. This would require adding two things: - copy constructors for each lockable object (to update the pointer in lock-list) - some code in ~MetadataItem() destructor to remove the pointer from lock-list -- Milan Babuskov http://fbexport.sourceforge.net http://www.flamerobin.org |
From: Michael H. <mic...@el...> - 2005-09-22 09:41:13
|
Milan, Milan Babuskov wrote: > There isn't anything, I even considered to do it, but never got to > it. Please, go ahead with it. Will do, when locking per object stays as is. Otherwise there's not much of a reason to do it now. > I dislike the whole per-whatever locking idea. > > If you ask me, I would use a single lock that holds the entire DBH. > The lock itself would build a list of objects that tried to update > themselves. When lock is completely released, it would call update() > on each object from that list. That looks like a good idea at first, but could paint us into a corner later on. Suppose you need to lock one object, but don't *want* to lock all other updates too? There might be cases in the future we do not envision now, what with background threads and everything... I would like to keep the per-object locking, it's what I am comfortable with, and what works very well in a lot of code I know. There only needs to be a way to create DBH objects with the correct lock counter value, or a way to set this value from the parent, but not from other objects... Thanks -- Michael Hieke |
From: Milan B. <mi...@km...> - 2005-09-22 11:08:24
|
Michael Hieke wrote: >> If you ask me, I would use a single lock that holds the entire DBH. >> The lock itself would build a list of objects that tried to update >> themselves. When lock is completely released, it would call update() >> on each object from that list. > > That looks like a good idea at first, but could paint us into a corner > later on. Suppose you need to lock one object, but don't *want* to lock > all other updates too? Well, you don't lock them, just delay them. > There might be cases in the future we do not > envision now, what with background threads and everything... You could be right, OTOH locks really don't last long. I need to think about this. We also need to implement that invalidation stuff, and this could be an easy shortcut for it. -- Milan Babuskov http://fbexport.sourceforge.net http://www.flamerobin.org |
From: Michael H. <mg...@gm...> - 2005-09-22 17:06:54
|
Milan, Milan Babuskov wrote: > I dislike the whole per-whatever locking idea. > > If you ask me, I would use a single lock that holds the entire DBH. > The lock itself would build a list of objects that tried to update > themselves. When lock is completely released, it would call update() > on each object from that list. I have thought some more about your idea, and it has one really great feature: It creates a single place where the batches of updates are performed. Easy to Freeze() the tree control before, and to Thaw() it once this is done. Thanks -- Michael Hieke |