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:: 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 >