Could you make your patch using trunk/ as the "base URL"?

Also, I think it should be possible to add a unit-test using a Duration
(or System.currentTimeMillis()) initialized in an "entry command" and
checked in a "finally command" to be less than TIME_SLICE.
...and checking that there's no "premature termination" of the loops
(see below).

Please note that I have no commit rights, so someone else will have to
review this and submit it.


http://gwt-code-reviews.appspot.com/1680804/diff/1/SchedulerImpl.java
File SchedulerImpl.java (right):

http://gwt-code-reviews.appspot.com/1680804/diff/1/SchedulerImpl.java#newcode176
SchedulerImpl.java:176: canceledTasks++;
Thinking a bit more about it: not only would this induce an unnecessary
additional iteration in the case all tasks are cancelled in the same
run, but it could lead to premature termination of the loop when you
have, e.g., two tasks in the list, the first returns false on the first
iteration while the second executes fast for a few runs: on the second
iteration, you'd increment canceledTasks for the first task, then
because the second task isn't cancelled and executes fast, there'll be a
third iteration where you'll increment canceledTasks yet again, for the
first task, which was already counted in the second iteration; so even
if we haven't yet reached the TIME_SLICE limit, there won't be a fourth
iteration; even worse actually: the 'for' loop will be terminated too
and the third iteration over the tasks will be aborted before the second
task can execute for the third time => premature termination.

canceledTasks should really count the number of "tombstones" *put* into
the list (that is, incremented at the time you set(i,null).

Alternatively, we could keep the canceledSomeTasks boolean and instead
introduce an executedSomeTasks boolean.

boolean canceledSomeTasks = false;

while (executedSomeTasks && ...TIME_SLICE) {
  boolean executedSomeTasks = false;
  for (int i = 0; i < length, i++) {
    Task t = tasks.get(i);
    if (t == null) {
      continue;
    }
    executedSomeTasks = true;
    ...
  }
  if (!executedSomeTasks) {
    // 'tasks' is full of 'null's, no need to iterate again
    break;
  }
}

http://gwt-code-reviews.appspot.com/1680804/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

Reply via email to