fred
fred copied to clipboard
freenet
Lazy start RequestStarter's
This saves a few threads at the cost of some complexity. Its main purpose is to make it easier to simulate really big networks for one-request-at-a-time simulations, e.g. for extensive unit tests for routing (not yet implemented but possible). It is off by default.
I would like to get my simulation changes merged eventually, and doing it in pieces makes sense. However it may be that this is not appropriate for Freenet proper. On a thread-constrained node it saves up to 8 threads when some types of request are not running locally (CHK, SSK * bulk, realtime * request, insert).
Thoughts?
On a thread-constrained node it saves up to 8 threads when some types of request are not running locally.
That practically amounts to "never" on a connected node. I don't think we need this additional complexity in RequestStarter for a case that never happens on a normal node. Subclassing RequestStarter for your simulations might make more sense.
There are 8 threads: {CHK, SSK} * {request, insert} * {bulk, realtime}.
FProxy uses realtime requests, downloads (including built-in stuff) use bulk requests, uploads (including built-in i.e. ARKs) use bulk inserts. So for example realtime inserts are used very rarely. So it probably saves 2 threads on an average node - if it's turned on (it's off by default). And more on idle nodes.
Subclassing RequestStarter might be possible with classloader hacks. Do we want classloader hacks in our unit tests? Also because of concurrency issues there would still need to be significant refactoring of the run method.
As I said in my original explanation, I want my simulations to be merged, because that will allow easily testing changes to routing, including JUnit tests run with test.extensive=true which could be run as part of the release process and/or continuous integration. That will be another, somewhat larger, pull request, but I'm trying to post its dependencies separately for ease of review.
I have dug a bit into this, and summarised my findings in this bug: https://bugs.freenetproject.org/view.php?id=6827 This branch does not only lazy start, but it also exits after the queue is empty. Unfortunately that means that ordinary cooldown can result in the RequestStarter not waking up when it should. That's an existing bug, but it only adds an extra 1000ms latency to a request that's just gone to sleep for 30 minutes. If it was fixed, then we could have a fully asynchronous RequestStarter, either with wait() (rather than polling every 1000ms), or without needing a dedicated thread at all. Fixing this properly requires being able to test it, especially given all the trouble we've had with cooldown causing end-user issues in the past.
That practically amounts to "never" on a connected node. I don't think we need this additional complexity in RequestStarter for a case that never happens on a normal node. Subclassing RequestStarter for your simulations might make more sense.
Toad had sounded rather fearful on IRC that this concern could become a mergeblocker, so I'd like to give a thought to help easing the decision: Over the years, I had sometimes been afraid that our work is not focusing enough on doing proper science. Some of it was more code-and-fix than maths, simulations and measurements. Thus I'm very grateful that toad has switched focus now; and also that he is doing this for free, and adapting his university studies to allow him to have this as a subject. That's quite a service :) So I'd say: Let's please be open to merging this even if it isn't 1000% clean. There often are cases where unit tests would require very complex changes to the non-test classes to be completely isolated, and then it is just easier to have some test code in the regular codebase itself. If that means we can have proper simulations soon, and use them in unit tests, let's just trust toad with his judgements that doing it in a more clean way would be too much work.
The benefits of this pull request to the project as a whole void my previous objections to having this merged.
There are some whitespace issues with this changeset. As soon as those are addressed, I won't object to having this merged.
