Reusing of KIO slave instances

[Andreas Hartmetz]

> On Thursday 19 November 2009 10:42:39 Dawit A. wrote:
> > On Thursday 19 November 2009 04:11:15 Sebastian Trueg wrote:
> > > Did I understand correctly from reading the thread: except for the
> > > changes in job.cpp you commited your patch? I.e. maxInstancesPerHost
> > > supprot is already implemented if an app uses the scheduler?
> >
> > Unfortunately I did not commit any portion of my patch yet for lack of
> > testing. Instead I opted to use the patch locally for an extended period
> > of time and see there are any negative side effect and so far I have had
> > none. I was being overly cautious because this portion of the code
> > affects every single application that accesses the network or the file
> > system.
> >
> > > So what I would test is the part of the patch that potentially leads to
> > > dead-locks in file_copy and friends?
> >
> > Yes, but in addition someone else besides me has to test whether or not
> > the default limits used for maxInstances and maxInstancesPerHost are
> > actually sufficient as well and generally test to see if there are any
> > problems such as performance degredation etc etc...
> >
> > > If so, yes, of course I will test that one.
> >
> > Great, then I will adapt these changes to trunk and post a patch later on
> > today.
> 
On Friday 20 November 2009 00:14:55 Dawit A. wrote:
> Here is the patch as promised...
> 
> Note that I have not even tested whether or not this patch compiles since I
>  do not yet have Qt 4.6 installed. I have made the best effort to ensure
>  that it is an exact replica of the one I have in the 4.3 branch which does
>  compile and work fine. Also when apply the patch you have to make sure you
>  do it from the top level of kdelibs since the it contains changes in three
>  separate folders within kdelibs.
> 
> Anyhow, here are a few notes about the the patch itself:
> 
> #1. With this patch the 'maxInstances' property which has been around since
> the creation of KIO or thereabouts will all of the sudden be enforced! This
> means that all protocol files will need to be checked to ensure they
>  contain this particular property with a sensible default or else only a
>  single instance of ioslave will be spawned for them by default! The patch
>  itself contains changes to protocol files included in kdelibs
>  (ftp/file/http), but not kdebase or other modules...
> 
> #2. The 'maxInstancesPerHost' property is optional and will not be enforced
>  if it is missing for a particular protocol. This was purposefully done for
>  the sake of backwards compatiability.
> 
> #3. I dealt with the deadlock conditions dfaure raised by simply refusing
>  to schedule a request whose other end also needs to be scheduled (e.g.
>  copying file from ftp to ftp or ftp to sftp) if an ioslave for the other
>  end cannot be reserved. This is a non-intrusive and much simpler solution
>  than otherwise suggested in those earlier discussions. It is however very
>  effective in combating deadlock conditions as far as I can tell...
> 
> If you want to test for deadlock conditions, then you can change the values
>  of the maxInstances/maxInstancesPerHost properties in any non-local (e.g.
>  ftp/sftp/http) protocol files and start copying files between two
>  different servers. If you make the value of those properties low enough,
>  you should reach a deadlock condition which should never happen with this
>  patch. To actually see deadlocks remove the patch to
>  kdelib/kio/scheduler.cpp, compile, and run the same tests again.
> 
> #4. The entire change needs to be stress tested under heavy request load to
> see if it works reliably. I have attempted to stress it by opening multiple
> table and visiting several complex sites, but some sort of automated stress
> testing would be nice...
> 
> Thanks...
> 
Hi,

(I took the liberty to reorder the messages above to end the top-posting)
I also have a patch sitting around that enforces per-host and per-protocol 
connection limits. I think that the current scheduler code leaves much to be 
desired, namely using appropriate data structures and modelling instead of 
linear list scans for just about everything. Most to all required features 
and/or constraints can actually be implemented much more directly with a 
different approach, mostly a QMap<int, Job*> with serial numbers (per host), 
and ~3 extra members in KIO::JobPrivate. Another QMap<int, PerHostData*> keeps 
jobs ordered between hosts.
Additionally the scheduler currently conflates limits enforcing with priorities 
which is a bad idea: high priority jobs don't observe any limits, low priority 
jobs scheduled with KIO::Scheduler::ScheduleJob() do observe per-protocol 
limits, IIRC. Priorities can be added to the design I described with virtually 
no effort by skewing serial numbers.
I'm slow to finish and commit the patch because I'm busy with my Diplom thesis 
(comparable but somewhat more work than Master thesis) :/
So Dawit, if you have something to commit *right now* and you coordinate on 
new API with me I'd say go ahead. The desktop files should be fixed better 
sooner than later and it will be interesting to have another implementation 
when investigating bugs. In the medium term I want to make the scheduler much 
better though; I've seen a lot of code in the KHTML loader that amounts to 
workarounds for KIO::Scheduler's deficiencies, especially lack of priority 
support and inefficiency with a large number of pending jobs as well as the 
tendency to spawn infinite ioslaves per host and per protocol.

Cheers,
Andreas