Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multi-threaded Queries, Terminating Queries and old Query.lock() not necessary anymore #15

Closed
ssardina opened this issue Sep 30, 2018 · 17 comments

Comments

@ssardina
Copy link
Contributor

The JPL documentation includes a section on Multi-Threaded Queries that makes total sense. However, there seems to not be anymore the public static method Query.lock to be used in the applications.

All core query predicates (e.g., hasSolution) are synchronized static methods, but when combined with iteration, issues may arise if I understood well.

I went back even to the first commit of the repo and I couldn't find that lock() method, even though they are referred to in the source code, for example:

To ensure thread-safety, you should wrap sequential calls to
	 * this method in a synchronized block, using the static
	 * lock method to obtain the monitor.
	 * <pre>
	 * Query q = // obtain Query reference
	 * synchronized ( jpl.Query.lock() ){
	 *     while ( q.hasMoreElements() ){
	 *          Hashtable solution = q.nextSolution();
	 *          // process solution...
	 *     }
	 * }
@JanWielemaker
Copy link
Member

Not 100% sure. AFAIK, when JPL was initiated SWI-Prolog was single threaded and queries from multiple threads required a lock. Since a long time SWI and JPL support threads. The JPL creates
a number (configurable) of Prolog engines and pools queries to Prolog to one of these engines.

@anionic
Copy link
Contributor

anionic commented Oct 1, 2018

The doc section "Multi-Threaded Queries" may still make sense, but it is obsolete; Jan implemented an engine pool for JPL long ago. It looks like that document was updated for JPL7 down as far as "The Util class" but "Querying Prolog" onwards needs fixing - I'm on that - watch this space.

@ssardina
Copy link
Contributor Author

ssardina commented Oct 1, 2018

Hi both,

Thanks for your replies, highly appreciated!

I have been looking at the code today and as Jan suggested and confirmed by anionic (are you Paul S?) there is now an implementation of SWI prolog engine pools, and for what I understood each Java thread is linked to a different SWI engine.

The lock() method is not there anymore, though I would imagine it could still be useful/necessary even within the same engine (in the same thread) right?

Also, let me see if I understood correctly: by default in the C-code there are max 10 SWI engines and each Java thread will be attached to a different one. So this would imply that if I have say 11 Java threads wanting to do JPL-Prolog, it won't work? I ask this because I may have way more than 10 agents (each a thread) trying to do Prolog....

@anionic: let me know if you need help, keep me posted on the doc. :-)

Thanks!

Sebastian

@JanWielemaker
Copy link
Member

The lock() method is not there anymore, though I would imagine it could still be useful/necessary even within the same engine (in the same thread) right?

No. Two Java threads are never associated with the same Prolog engine as used to be the case when there was only one. Locking here will effectively make the whole pool design useless.

I don't know the code when JPL is out of Prolog engines. There are several things one can do, notably wait until one becomes available, create more or throw an error at the caller. Once upon a time the number of Prolog engines was limited, but it is now only limited by the OS.

@ssardina
Copy link
Contributor Author

ssardina commented Oct 1, 2018

Good point. So Query.lock() is totally not necessary, right, I get it.

I did a few testing and the pool of engines seems to re-use engines for threads that are finished. So there could never be more than 10 active threads using SWI, but once a thread finishes, it seems for what I have seen that the engine for that thread is re-used for another thread. I enabled some printout in JPL and the same engine seems to show up again in another thread...

Stil this may not solve my problem because I could have say 30 agents, all wanting Prolog at the same time, and they cannot just wait for another agent to die (may never die)..

So, Jan, are you saying that we can improve the JPL pool of engines to not have an upper bound (10 currently)? Or that there is no upper bound now already.. See this code in jpl.c

#define JPL_MAX_POOL_ENGINES 10    /* max pooled Prolog engines */
#define JPL_INITIAL_POOL_ENGINES 1 /* initially created ones */


@JanWielemaker
Copy link
Member

I don't know the code very well. I did little more than fix some compiler warnings and portability to it. I guess you can read the code yourself?

@anionic
Copy link
Contributor

anionic commented Oct 1, 2018

I believe that an engine is taken from the pool and attached to a thread when a Query is activated by that thread, and returned to the pool as soon as the Query is closed (explicitly or by solution exhaustion). If no pool engine is available, query activation delays until one is freed. Thus JPL_MAX_POOL_ENGINES doesn't limit the qty of threads which have called Prolog, only the qty with contemporaneously active queries. So your agents need not die, just close their queries nicely, and take their turn in calling Prolog. I think this is more cooperative than you inferred; is it good enough?

@ssardina
Copy link
Contributor Author

ssardina commented Oct 2, 2018

Hi both,

Thanks for your input!!

I had a careful look at the code and run many experiments with many threads etc to see what was happening. The situation seems to be exactly as @anionic has described with a subtle point:

There is a pool of engines, at this point 10 (specified in the C code) and an engine is attached to a Query when this is activated by its thread. The engine is then freed when the Query closes (simple queries like hasSolution or oneSolution close the query right away). One in the pool it is available for any other query in any other thread (or the same thread), and so on. So, YES, the JPL_MAX_POOL_ENGINES dictates how many concurrent opened queries can be at any point, and not how many threads: the engines are shared across threads.

There is only one subtle point: the first engine. This is created at the start and seems that once used and attached in a thread, it can only be re-used by queries in that thread. So the first engine does not seem to be available to other threads after that, even if the query that used that engine was closed.
@anionic , does this subtle point makes any sense to you?

I am curious that @JanWielemaker said one could have as many SWI engines as wanted, modulo OS limitations... This means we could potentially get rid of JPL_MAX_POOL_ENGINES...

@JanWielemaker
Copy link
Member

The first one may be special because the first Prolog engine is still special and, unlike other engines, cannot be destroyed. I think that also means you should make sure that the Java thread that created this engine stays alive. There is no need for this Java/Prolog couple to do any work though and if they are just sitting there they should be fairly cheap.
I guess JPL_MAX_POOL_ENGINES should be a dynamic setting that can either be changed on the fly or specified as startup option. Removing can be an option, but if you have a workload where the memory usage of Prolog queries is huge you may not run too many concurrently.

P.s. Note that engines cannot communicate thread_local predicates or global variables. That means that you can only use these from Java in a single query. If you fire two consecutive queries you may be talking to two different engines.

I'm not really into developing JPL, but happy to merge pull requests.

@ssardina
Copy link
Contributor Author

ssardina commented Oct 3, 2018

All good points Jan: may not be smart to just remove JPL_MAX_POOL_ENGINES, but would be good to allow setting it (either at start of application or dynamically).

I am interested in JPL and improving it, but I want to be careful as I am only now learning its internal and it is pretty sophisticated code in some parts. The pool framework works very well and is powerful, even for my many agents (28+) because as @anionic has stated, engines are re-usable if we make sure we close the query (this happens 95% of the times as we use hasSolution, oneSolution or allSolutions anyway).

So thanks for all this thread exchange, I now have a much more clear view. I will document it in a README and maybe you can merge that as documentation. Will probably clean up the source to remove the old reference to Query.lock() which not applicable anymore.

Sebastian
PS: issue #13 is much more critical at this point I think. I have a workaround but is it the best?

@ssardina
Copy link
Contributor Author

ssardina commented Oct 3, 2018

This post should be closed as Query.lock() is not necessary anymore due to the implementation of a re-usable pool of Prolog engines. What needs to be done are:

  1. Update internal source code to remove the references to Query.lock()
  2. Update JPL external documentation, particularly the Multi-Threaded section
  3. Document somehere (In a readme) how the pool of engines work so that the user knows how to use JPL well.

@ssardina ssardina closed this as completed Oct 3, 2018
@ssardina
Copy link
Contributor Author

ssardina commented Oct 3, 2018

Actually, I may keep it open until we complete at least steps 1-3 above.

Here is a first take on point 2:

https://github.com/ssardina-research/packages-jpl/wiki/Multi-Threaded-Queries

@anionic : what do you think to replace the text on the JPL doc? Also, note that the section "Terminating Queries" should also be updated/removed. For example, there is no Query.rewind() anymore, there is only Query.close()

@ssardina ssardina reopened this Oct 3, 2018
@anionic
Copy link
Contributor

anionic commented Oct 3, 2018

I've uploaded my rewrite of "Java API - overview" to org.jpl7, then found your "Multi-Threaded-Queries" page, which I like; undecided whether to add that as a separate page and keep the overview terse?

If you are willing to experiment, it would be interesting to know what actually happens if a thread tries interleaved solution retrieval, or passing an active query to another thread...

@ssardina
Copy link
Contributor Author

ssardina commented Oct 4, 2018

Thanks @anionic. I had a quick look at yours and I like it too, it is more succinct. I would probably just add a link for discussion and more details if you want to my page.

I will develop more my text there including what you suggested to cover all subtleties so a user of JPL does not have to guess. I have done a bit of what you said and the result is probably not what one would expect: if you pass a query object to another thread, then that new thread will complain it has the engine not attached. But I still need to investigate that more to write something informative and true.

I suggest then, if you agree, you add a link to my link as more details/information.

@ssardina ssardina changed the title Query.lock() gone? Multi-threaded Queries, Terminating Queries and old Query.lock() not necessary anymore Oct 4, 2018
@ssardina
Copy link
Contributor Author

ssardina commented Oct 4, 2018

Hi @anionic . I have much developed the explanation on Multithreaded, including termination, nested retrieval of solutions in a thread (error!), and even passing around query objects (active or non active) to various threads as you suggested.

Now that I understand it more clearly JPL is really quite powerful as is. If one uses the basic functionalities in a simple way (one-shot queries in the same thread), then one does not need to understand much. If one uses iterator type queries and multi-threading, then one has to be more careful and understand the few limitations.

https://github.com/ssardina-research/packages-jpl/wiki/Multi-Threaded-Queries

I also explain the different type of queries one can use with JPL here:

https://github.com/ssardina-research/packages-jpl/wiki/Types-of-Queries:-One-shot-vs-Iterative

Cheers,

Sebastian
PS: as I am understanding the code, I am cleaning a few bits. For example, there are two very important methods in Query called get1() (linked to hasMoreSolutions() check) and get2() (linked to nextSlution()) that took me sometime to understand in detail partly because of their names. I think they should be called something like: fetchNextSolution() and getCurrentSolutionBindings()

@ssardina
Copy link
Contributor Author

ssardina commented Oct 4, 2018

All three points above have been done (@anionic did the external doc already). I think this issue is now understood and addressed. There was no reference to Query.lock() in the actual source, only in the doc.

@ssardina ssardina closed this as completed Oct 4, 2018
@JanWielemaker
Copy link
Member

Looks correct to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants