linas
commented
5 years ago I think I like this. But first some general commentary, just so that things are clear.
-
methods to copy atomsThere is one and only one valid place where it is acceptable to copy an atom: this is during atom-table insertion, so that the AtomTable can have it's own private copy. It is currently "illegal" to make copies of atoms; it should never be done. (There might exist bizarro corner cases, but I cannot think of any) The goal is to always have only one copy, ever, of any atom. So please to NOT add a_copier() -
The atom factory should only be called only once, ever, and that is when an atom is inserted into the atomspace. It is used for only one purpose: to create the one-and-only, single, final version of the atom. The factory should never-ever be called from anywhere else.
-
Some atom constructors are very expensive. For example, the
PatternLinkdoes a HUGE amount of calculation. That's OK, because that calculation is only done once, ever, when the PatternLink is inserted into the atomspace. We want to be careful, and not accidentally cause a PatternLink costrutor to run more than once. -
The optimization problem is to move atom-style-data as fast as possible from the user-API (scheme, python, haskell) into the atomspace, where the factory runs once, and returns. By "atom-style-data", I mean: a type, a node-string, a link-handle-seq and maybe a TV. I don't care how it is moved into the atomspace, as long as it's fast. Today, the easiest, fastest way to do this is to create an "empty shell of an atom", with createNode/createLink, and pass it around as needed. Simple, easy, uniform. But it's just an empty shell, until the factory runs and creates "the real one" (the one-and-only, true, globally-unique real one).
-
One performance optimzation might be to make the shell "even emptier", somehow. Maybe by not keeping the incoming set for the non-final atoms? So that any atom that is not in the atomspace also does not have an incoming set! Huh. I like that idea...
OK that's the story of factories. The stories of validators are different.
-
The idea of validators is still kind-of an experimental prototype, and I am not yet convinced that it is a good idea. Maybe it is, and maybe there is a better way. It kind-of-works, mostly, but its ugly and it has problems that I don't know how to solve, yet. The experiment is described here:
atom_types.scriptlines 66 to 107 -
Those six special link types are tightly coupled to the validators, to force validation.
-
Some atoms have factories that differ from validators. For example,
ARITHMETIC_LINKhas a factory, because there is a C++ class calledArithmeticLink. However, (see atom_types.script line 591) is also aNUMERIC_LINKand aNUMERIC_OUTPUT_LINK, and so those will trigger validators that are completely unlrelated to the C++ class. Those validators come from a different place.
So I think I like your proposal. Some point by point comments:
- move validator call from
DEFINE_LINK_FACTORYDEFINE_NODE_FACTORYto theClassFactory::factorymethod
Sure, fine, don't care.
- stop replacing factories by
validating_factoryinClassServer::addValidatormethod, just call proper validator instead inClassFactory::factory
Uh sure, I guess ?? I'm not sure what this solves.
- reuse
ClassServer::spliceFactorymethod to splice not only factories but validators as well
Yes. I think you discovered this in your unit test. I wrote splceFactory(), and although it is only 20 lines long, it took me two or three very long days to write it; I tried half a dozen other approaches, some of which were extremely complex, and took many hundreds of lines of impossible code. I was very happy when I finally found a simple way to create spliceFactory. By the end, I was so disgusted and tired, that I did not want to do it again (i.e. I did not feel like creating spliceValidator) . But your unit test discovered the bug that results from my laziness.
- as side effect it should fix unit tests from PR https://github.com/opencog/atomspace/pull/2023#issue-249267032
OK
- add additional
ClassServer::_copiercollection to keep methods to copy atoms - add additional method
copyAtomto use it inAtomTable::addmethod for atoms copying
Avoid copies, per above.
- replace
Handle (*)(const Handle& )factory method signature by method with vararg like inValueFactory
Careful, I had to fix a bug in ValueFactory just a few weeks ago. It works, I like the idea, but the use of the static is ... interesting, non-obvious, subtle, and error prone.
- in
createNodeandcreateLinkreplace new atom creation by arguments forwarding
Uhh, sure I guess. I don't understand what that means. Keep in mind, most atoms should be "empty shells", unborn seeds, just passing through, temporary holders for a type, a node-string and a handle-seq. it should be simple and fast to create them, and then let them die as soon as they've moved thier atom-style-date from the API to the atomspace.
vsbogd
ngeiswei
This issue is to discuss plan of removing unnecessary double atoms creation which were discussed at issue #1965, especially in comments:
At the moment
createNodeandcreateLinkfunctions create instance of unspecific instance ofNodeorLinkclass and passes them intoClassServer::factorywhich creates another atoms with proper class.Suggested plan:
DEFINE_LINK_FACTORYDEFINE_NODE_FACTORYto theClassFactory::factorymethodvalidating_factoryinClassServer::addValidatormethod, just call proper validator instead inClassFactory::factoryClassServer::spliceFactorymethod to splice not only factories but validators as wellClassServer::_copiercollection to keep methods to copy atomscopyAtomto use it inAtomTable::addmethod for atoms copyingHandle (*)(const Handle& )factory method signature by method with vararg like inValueFactorycreateNodeandcreateLinkreplace new atom creation by arguments forwarding