RFC: KIO::Scheduler changes... [PATCH included]

[Dawit A.]

Does anybody object to me committing this patch with the exception of the 
job.cpp changes ? That way the current behavior is not changed and only those 
use KIO::Scheduler::scheduleJob are affected ? This would allow the webkit part 
users to test the changes and provide feedback. Otherwise, this will simply 
bit rot and be forgotten...

On Sunday 30 August 2009 20:38:15 Dawit A. wrote:
> New patch that combines all the individual patches I posted before (to be
> applied from kdelibs level) with the following additional changes:
> 
> 1.) Improve the scheduler patch by putting the enforcing of the ioslave
>  limits into two functions (createSlave and findIdleSlave). Still limits
>  are only enforces when jobs are scheduled.
> 
> 2.) Fixed a logic error in modification to
> KIO::Scheduler::assignJobToSlave(...).
> 
> 3.) Modification of the "maxInstances" and "maxInstancesPerHost" entries
>  for the file, http and ftp ioslaves to make it easier to test the change
>  itself. Note the I picked these values arbitrarily. They are simply limits
>  I chose would be sufficient and are not something set in stone...
> 
> Now if someone else could apply these patches and provide feedback that
>  would be great...
> 
> On Friday 28 August 2009 17:25:54 Dawit A. wrote:
> > This is a follow up on the the recent discussions about what
> > KIO::Scheduler does by default and how we can go about improving the
> > current default behavior to resolve some long standing issues in KDE as
> > well as limit the number new ioslaves (read: new connections) that get
> > spawned. For further details please read the the threads linked below or
> > the explanation at the bottom of this email.
> >
> > The patches provided here are my attempt to address the current short
> > comings of the scheduler and are based on a patch David posted in [1]
> > with the following additions/modifications:
> >
> > 1.) Add support for optional host based limits to the .protocol files
> > (maxInstancesPerHost) and if set use them in KIO::Scheduler::scheduleJob.
> >
> > 2.) Modify the job.cpp class to ensure all redirected requests are re-
> > scheduled.
> >
> > 3.) Remove scheduled jobs if they are to be used in connection-oriented
> > scheduling mode, KIO::Scheduler::assignJobToSlave.
> >
> > -------------------------------------------------------------------------
> >-- ------------------------------------------------------------------
> >
> > What follows is a brief explanation of how jobs are currently handled by
> > the scheduler and how they will be handled when the attached patches are
> > applied for those that do not want to be bothered to read the other
> > threads linked below.
> >
> > First, for the benefit of those not familiar with the inner workings of
> > KIO, here is roughly how a job is handled by KIO internally:
> >
> > 1.) A call to KIO::get, KIO::post, KIO::mimetype comes in...
> >
> > 2.) A job is created and of it is of type SimpleJob, it is handed over to
> > KIO::Scheduler...
> >
> > 3.) KIO::Scheduler simply checks to see if there is an idle ioslave that
> > can be reused to handle the request. If one is found, it is reused.
> >
> > 4.) If none was found, the scheduler will always attempt to create a
> > brand new ioslave. No limits on the number of ioslaves it will create.
> >
> > The issue with this is that the scheduler by default does not have any
> > limits on the number ioslaves (read: new connections) that it spawns.
> > Instead it forces you to call an additional function,
> > KIO::Scheduler::scheduleJob, if you want to limit the number ioslave
> > instances.
> >
> > In practise the problems with this default behavior only affects users
> > under certain circumstances where servers actually put a limit on the
> > number of connections they allow from a given address. This is specially
> > true for more than a few ftp servers out there. In fact, there is a long
> > standing FTP related bug vs Konqueror caused by this very issue. However,
> > just because users do not directly see the side effects does not mean
> > there is no problem with what the scheduler does by default. Creating too
> > many connections by spawning many ioslaves is a costly affair to both to
> > the client and server machines since it will unnecessarily result in the
> > creation of many sockets. Hence, we end up using too many resources.
> >
> > Okay so what does this attach patch do ?
> >
> > 1.) Schedule all simple jobs created through KIO::<get/post/..> by
> > default.
> >
> > 2.) Add an optional "maxInstancesPerHost" property to the *.protocol
> > files that is honored when jobs are scheduled which is now the default.
> > KIO::scheduleJob currently honors the existing "maxInstances" property
> > set in each *.protocol file as a 'soft limit'. However, if #1 is made the
> > default, then this will cause a bottle neck at the scheduler since the
> > "maxInstances" property is per protocol (ftp, http). For example, for the
> > ioslaves included in kdelibs, these value is set to 4, 3, and 2 in file,
> > http and ftp protocol files respectively. Having only this many instances
> > of ioslaves KDE wide is not sufficient. Hence the new property. After
> > that it is only a matter of setting both
> > "maxInstances=" and "maxInstancesPerHost=" proerties to some sane default
> > values so that the scheduler can do a better job of scheduling requests
> > per host.
> >
> > For example, fot the http protocol the "maxInstances" property can be
> > raised to "15", 5x as much as its original value, and
> > "maxInstancesPerHost" set to "3". This means that there cannot be more
> > than 15 instances of the http ioslave at any given time and no more than
> > 3 connection to any given host. All jobs will be queued until this
> > criteria is fulfilled...
> >
> > What issues remain ?
> >
> > 1.) Unfortunately the patch to job.cpp will break applications that might
> > want to have direct control over the number of ioslaves (read:
> > connections) , e.g. a download manager. The change to scheduling jobs by
> > default will make direct control impossible since all jobs will be
> > queued. So, an additional function is required in KIO::Scheduler to
> > remove the scheduled job from the queue or such applications need to call
> > KIO::Scheduler::doJob again and we can add the remove from queue check
> > there ??? Somehow the last one seems very inefficient to me, doJob(...),
> > scheduleJob(...) and then doJob(...) again...
> >
> > 2.) Should the other scheduling modes, besides "scheduled mode" be forced
> > to honor the "maxInstances" property ? My own view is no since the
> > default does and the other modes are meant to give the users of KIO
> > complete control to schedule their jobs how they see fit.
> >
> > Anyhow, comments, suggestions, improvements and everything in between is
> > welcome...
> >
> > [1] http://lists.kde.org/?t=124531188100002&r=1&w=2
> > [2] http://lists.kde.org/?t=125042610900001&r=1&w=2
>