Arvados

Comments on 24b0da5

I think it's worth adding a comment near my $tasks_this_level ... [a] explaining that the reason we do this is to increase the per-task memory limit when ntasks<nslots, and [b] acknowledging this artificially reduces concurrency in the case where level L starts with few tasks but then adds more tasks at level L. Renaming $tasks_this_level to $max_slots_this_level might help here too (as named, it can go out of sync with reality, but as $max_slots_this_level it is accurate throughout its scope).

If $tasks_this_level < @node we should raise it to @node. (Same memory benefit, but greater max concurrency.)

Undecided whether it's worth asking for a log message when we detect the "artificially reduces concurrency" situation (something like @jobstep_todo > 0 && $tasks_this_level < @slot ?).

Is this still OK? @slot > @freeslot used to mean "any tasks are running" but now it can be true when no tasks are running, so I wonder if this can cause us to get stuck in THISROUND when no tasks are running. I suspect it should change to $tasks_this_level > @freeslot...?

  while (!@freeslot
         ||
         (@slot > @freeslot && $todo_ptr+1 > $#jobstep_todo))

(I think the part after || here is trying to say "don't walk off the end of the todo list and proclaim end-of-this-round while there are any tasks still running which might give us more todo's -- either by queueing more tasks, or by failing".)

Tom Clegg wrote:

I think it's worth adding a comment near my $tasks_this_level ... [a] explaining that the reason we do this is to increase the per-task memory limit when ntasks<nslots, and [b] acknowledging this artificially reduces concurrency in the case where level L starts with few tasks but then adds more tasks at level L. Renaming $tasks_this_level to $max_slots_this_level might help here too (as named, it can go out of sync with reality, but as $max_slots_this_level it is accurate throughout its scope).

Given the value actually in the variable, and other changes you suggested, $max_slots_this_level isn't quite right either. Renamed to $initial_tasks_this_level.

Undecided whether it's worth asking for a log message when we detect the "artificially reduces concurrency" situation (something like @jobstep_todo > 0 && $tasks_this_level < @slot ?).

Since you're undecided, I took the cheap option and added the number of slots we're using to the "start level" log message. It's not quite as explicit, but the reader should be able to notice the situation without any extra logic on our part.

Is this still OK? @slot > @freeslot used to mean "any tasks are running" but now it can be true when no tasks are running, so I wonder if this can cause us to get stuck in THISROUND when no tasks are running. I suspect it should change to $tasks_this_level > @freeslot...?

I think you're right there's likely a bug here. It now compares against the initial size of @freeslot instead.

Implemented all your other proposals as written, and we're now at 55bc432. Thanks.

55bc432 LGTM, thanks