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

[Dawit A.]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: scheduler.patch
Type: text/x-patch
Size: 17866 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20090830/bb179c6f/attachment.bin>