Parallel Get/Put links #2316

stellarspot · 2019-08-14T17:08:33Z

This is the draft pull request which arises from discussion #1502 "Stop using SetLink for search results".
The used idea is that pattern matcher creates results in parallel and other links read them in parallel.

The aim of this pull request is to show how this can work with introduced ParallelLink which puts its results into queue and PutLink reads them.

In the current design both ParallelLink and PutLink work on the same thread. Making pattern matcher parallel can be considered as the second step if the current design with queue looks appropriate.

Here is a simple sample how it works:

(Inheritance (Concept "ball-1") (Concept "green"))
(Inheritance (Concept "ball-2") (Concept "red"))
(Inheritance (Concept "ball-3") (Concept "green"))
(Inheritance (Concept "ball-4") (Concept "red"))

(cog-execute!
 (ParallelGet
  (Variable "$BALL")
  (Inheritance
   (Variable "$BALL")
   (Concept "green"))
  (SetNode "S") ))

(cog-execute!
 (Put
  (Variable "$BALL")
  (Inheritance
   (Variable "$BALL")
   (Concept "selected-balls"))
  (SetNode "S")))

ParallelGetLink uses SetNode as input. Set node contains queue which used to exchange results between atoms.

ParallelGetLink puts its results to queue and generates MemberLinks:

(MemberLink
   (ConceptNode "ball-1")
   (SetNode "S"))
(MemberLink
   (ConceptNode "ball-3")
   (SetNode "S"))

PutLink uses SetNode as argument and reads results form the queue and produces:

   (InheritanceLink
      (ConceptNode "ball-1")
      (ConceptNode "selected-balls") )
   (InheritanceLink
      (ConceptNode "ball-3")
      (ConceptNode "selected-balls"))

linas · 2019-08-14T17:49:43Z

Wow! OK! I think I like this!

Rather than creating a ParallelGet, it might be better to just re-use the existing GetLink, and allow it to have an optional 3rd argument.
The cog-utils repo has implementations of a thread-safe queue and and a thread-safe stack; it would be better to use that, instead of inventing something new.
It might be better to have QueueValue be a special case of StreamValue? That is because StreamValue already has some basic infrastructure for time-varying values; and QueueValue extends that.

Some more comments to follow for specific lines of code.

linas

Some minor commentary about specific lines of code ...

linas · 2019-08-14T17:51:49Z

opencog/atoms/core/PutLink.cc

+			// If the argument is SetNode, then process atoms from queue
+			// stored in SetNode value
+			AtomSpace* as = getAtomSpace();
+			ValuePtr value = args->getValue(QueueValue::QUEUE_VALUE_KEY);


Change this to DEFAULT_QUEUE_VALUE_KEY. We should also probably do the same thing for DEFAULT_TRUTH_VALUE_KEY also (in some other pull req).

linas · 2019-08-14T17:56:36Z

opencog/atoms/value/QueueValue.h

+ */
+
+template <typename T>
+class ClosableQueue


There already exists code for something that is very similar, in cogutils, and I would rather that got used instead ...

linas · 2019-08-14T17:58:57Z

opencog/atoms/value/QueueValue.h

+{
+protected:
+	std::string _name;
+	HandleClosableQueuePtr _handle_queue;


Rather than doing it for Handle, doing it for Value would be better.

linas · 2019-08-14T17:59:40Z

opencog/atoms/value/QueueValue.h

+ */
+
+class QueueValue
+	: public Value


Should probably inherit from StreamValue instead ?

Oh, hang on, maybe it would be better to inherit from LinkValue?

Or maybe StreamValue and LinkValue should be merged into one ? Or something like that? Yes, that would be a different, distinct discussion. So, for now, maybe this is OK as it is.

linas · 2019-08-14T18:05:57Z

opencog/atoms/value/QueueValue.h

+
+	virtual ~QueueValue() {}
+
+	HandleClosableQueuePtr get_queue() const { return _handle_queue; }


Rather than doing this, maybe it would be better to just add push and pop directly to the API?

linas · 2019-08-14T18:20:14Z

One last remark: parallelizing the pattern matcher seems like it should be "easy" but might be quite difficult. There is a very obvious place to parallelize: the top-level search-starting-point loop. This is in InitiateSearch and it finds all of the places from which the search can be started. Replacing the foreach with a parallel_foreach seems like it would be very easy, and then it should "just plain work". Well, I tried that once, and it didn't work. I don't know why. It seemed like there were some snakes down in there, waiting to bite.

vsbogd · 2019-08-15T08:09:26Z

opencog/atoms/core/PutLink.cc

-			try
+			// If the argument is SetNode, then process atoms from queue
+			// stored in SetNode value
+			AtomSpace* as = getAtomSpace();


Is atomspace required here? It is not used in code below

vsbogd · 2019-08-15T08:13:26Z

opencog/atoms/atom_types/atom_types.script

@@ -388,6 +389,7 @@ SATISFYING_LINK <- PATTERN_LINK
 GET_LINK <- SATISFYING_LINK        // Finds all groundings, returns them
 QUERY_LINK <- SATISFYING_LINK      // Finds all groundings, substitutes.
 BIND_LINK <- QUERY_LINK            // Finds all groundings, substitutes.
+PARALLEL_GET_LINK <- SATISFYING_LINK  // Finds all groundings, puts them to queue


Should it be inherited from PATTERN_LINK instead?

vsbogd · 2019-08-15T08:29:18Z

opencog/atoms/pattern/ParallelGetLink.cc

+		HandleSeq handle_seq;
+		handle_seq.push_back(h);
+		handle_seq.push_back(_target);
+		Handle member_link = createLink(handle_seq, MEMBER_LINK);


Why don't add results to the atomspace in the ParallelSatisfier instead? Am I right that current implementation collects results as SetLink and then puts them into atomspace as MemberLink? It would be simpler to put them as MemberLink without using a Set as intermediate state.

linas · 2019-12-26T15:53:26Z

Closing. interesting prototype.

If you just want plain-old parallel GetLink, I think you can achieve this by looking at

atomspace/opencog/query/InitiateSearchCB.cc

Lines 402 to 417 in 812580d

    
           // TODO: This is kind-of the main entry point into the CPU-cycle 
        
           // sucking part of the pattern search.  It might be worth 
        
           // parallelizing at this point. That is, ***if*** the _search_set 
        
           // is large, or the pattern is large/complex, then it might be 
        
           // worth it to create N threads, and N copies of PatternMatchEngine 
        
           // and run one search per thread.  Maybe. CAUTION: this is not 
        
           // always the bottleneck, and so adding heavy-weight thread 
        
           // initialization here might hurt some users.  See the benchmark 
        
           // `nano-en.scm` in the benchmark get repo, for example. 
        
           // Note also: parallelizing will require adding locks to some 
        
           // portions of the callback, e.g. the `grounding()` callback, 
        
           // so that two parallel reports don't clobber each other. 
        
           PatternMatchEngine pme(pmc); 
        
           pme.set_pattern(*_variables, *_pattern); 
        
           for (const Handle& h : _search_set)

and converting that loop into a parallel-for-each loop. That parallelization is distinct from the other idea(s) in this prototype, which should be pursued as a distinct, unrelated project. (The other idea in this prototype being that search results can be queued into a QueueValue, making it easy to pipeline producers and consumers (the pattern matcher being the producer, and some PutLink-type thing being a consumer.)

linas · 2019-12-26T15:58:14Z

As usual, issue #1645 tracks the idea of parallelizing the pattern matcher, and issue #1502 tracks an earlier, related idea for QueueValue

stellarspot added 11 commits August 7, 2019 12:13

Add initial version of ParallelGetLink

72e2d4c

Remove unused satset from ParallelGetLink

62ad46c

Handle SetNode in PutLink

29719ed

Add initial version of ParallelSatisfier

82e990f

Add initial implementation of ProducerConsumerValue

076c8b2

Use Closable Queue in QueueValue

2daac23

Rename CONTROL_KEY to QUEUE_VALUE_KEY

c8fafe1

Handle SetNode with QueueValue in PutLink

8b086fc

Add ParallelGetLinkUTest

50fbc7b

Add ParallelGet get/put test

325c67f

Update ParallelGetLink link comments

b89c25f

linas reviewed Aug 14, 2019

View reviewed changes

vsbogd reviewed Aug 15, 2019

View reviewed changes

linas mentioned this pull request Dec 26, 2019

Localize use of PatternMatchEngine #2453

Merged

linas added enhancement pattern-matcher labels Dec 26, 2019

linas mentioned this pull request Dec 26, 2019

Design a Publish/Subscribe System (aka Futures) #1750

Closed

linas closed this Jan 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parallel Get/Put links #2316

Parallel Get/Put links #2316

stellarspot commented Aug 14, 2019

linas commented Aug 14, 2019

linas left a comment

linas Aug 14, 2019

linas Aug 14, 2019

linas Aug 14, 2019

linas Aug 14, 2019

linas Aug 14, 2019

linas Aug 14, 2019

linas commented Aug 14, 2019

vsbogd Aug 15, 2019

vsbogd Aug 15, 2019

vsbogd Aug 15, 2019

linas commented Dec 26, 2019

linas commented Dec 26, 2019


		virtual ~QueueValue() {}

		HandleClosableQueuePtr get_queue() const { return _handle_queue; }

Parallel Get/Put links #2316

Parallel Get/Put links #2316

Conversation

stellarspot commented Aug 14, 2019

linas commented Aug 14, 2019

linas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linas commented Aug 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linas commented Dec 26, 2019

linas commented Dec 26, 2019