Re: [lmi] is DBDictionary's sigleton nature relied on anywhere?

[Greg Chicares]

On 2012-06-11 17:16Z, Václav Slavík wrote:
> 
> the subject says it — does any code anywhere rely on DBDictionary being
> a singleton?

I guess you could say the singleton serves to prevent the defect you've
discovered from manifesting itself.

> I don’t quite follow all of that code yet, but from what I understood,
> it’s not relied on in most places (and it’s an obstacle in one) and the
> code using DBDictionary follows the same pattern: it calls instance(),
> immediately followed by some form of Init() to load data into it.
> 
> The only place where this pattern isn’t followed is in
> product_database::entity_from_key(), where DBDictionary is only
> initialized as part of product_database construction and then later
> repeatedly queried in entity_from_key().
> 
> And this is the part that confuses me: as far as I understand it
> (possibly wrongly), product_database — which is not a singleton,
> unlike DBDictionary — behaves as an obfuscated singleton: if you
> create 2+ instances of it, Query() calls on _any_ instance will
> return data from the most recently created one (worse, even
> after it’s destroyed).

Yeah.

A known defect in lmi's pre-wx (pre-2005) predecessor was that
its product editor didn't work correctly if more than one database
file was open: in that case, changing one instance (in one GUI
window) might modify a different file. I don't know whether the
same defect exists in lmi's product editor.

> This would be OK if only one instance of product_database could exist
> at a time, but that’s not the case, I can see multiple coexisting
> instances if I add asserts to check for them. Although they do access
> the same database in my testing, I’m unsure if that’s always the case.

I believe that end users are immune to that if they never use the
product editor.

> Is this behavior intentional? Am I misunderstanding something?

No, and no.

> I _think_ it’s a defect (one that my caching changes would fix naturally),
> but I’m not sufficiently familiar with these parts of the code to be sure...

It certainly is a defect. Let me know if there's any particular part
of the code that you'd like me to try to explain.

This code is at the very core of lmi. It's used almost everywhere
else throughout the system. It's probably the oldest part, and the
part I've maintained the most conservatively. It's so old, in fact,
that it predates standard C++ containers--for instance, where the
DBDictionary::Add() documentation says:

  /// Set a value. (The historical name "Add" is now misleading.)

we originally had a "dictionary" template class, because std::map<>
had not been standardized then; and that "dictionary" class had an
Add() member that probably did something like std::map::operator[].

The singleton serves only to implement caching; it has no other
purpose. It's a rather degenerate implementation of caching, with
only one slot. But that was "good enough" in an era when power
users had thirty-two megabytes of RAM. Because of the way lmi is
used in practice, I'd guess that cache misses occur on the order
of only ten percent of the time.

In case you haven't noticed, Input::DoAdaptExternalities() manages
the cache; that's probably the "magic" that prevents the defect that
you've pointed out from expressing itself in normal production use.

This old junk is not "precious" to me personally--it's nothing to be
proud of, especially today--so I don't mind if you change it. The
singleton in particular can be eradicated. I would like to retain
DBDictionary's inheritance from these classes:
    ,        public  xml_serializable  <DBDictionary>
    ,        public  MemberSymbolTable <DBDictionary>
because they provide for backward compatibility, which is extremely
useful. And I should also mention that there's a large amount of
non-distributed code that uses this class to write proprietary xml
files, using a pattern like this:
    InitDB();
    Add(database_entity(DB_AllowRetirees       , true));
    Add(...hundreds of other parameters...) ...
    WriteDB(AddDataDir("MyProprietaryProduct.database"));
and we aren't ready to retire that non-distributed code yet, so we do
need to maintain backward compatibility in the API for the three
functions in the "pattern" above.