/* I think the above is really just doing this:
AppTask<?> task;
while( (task = taskQueue.poll()) != null ) {
if (!task.isCancelled()) {
task.invoke();
}
}
//...but it's hard to say for sure. It's so twisted
//up that I don't trust my eyes. -pspeed
*/
The original code made my eyes bleed as well ;)
Here's a series of straightforward transformations so you can easily check that semantics is preserved:
Original code:
AppTask<?> task = taskQueue.poll();
toploop: do {
if (task == null) break;
while (task.isCancelled()) {
task = taskQueue.poll();
if (task == null) break toploop;
}
task.invoke();
} while (((task = taskQueue.poll()) != null));
Move the assignment from the final line into the loop:
AppTask<?> task = taskQueue.poll();
toploop: do { if (task == null) break;
while (task.isCancelled()) {
task = taskQueue.poll();
if (task == null) break toploop;
}
task.invoke(); task = taskQueue.poll();
} while (task != null);
Now it's easy to see that the outer loop is indeed just
AppTask<?> task;
toploop: while ((task = taskQueue.poll()) != null) {
while (task.isCancelled()) {
task = taskQueue.poll();
if (task == null) break toploop;
}
task.invoke();
};
which is indeed equivalent to
AppTask<?> task;
while ((taskQueue.poll()) != null) {
if (! task.isCancelled()) {
task.invoke();
}
};
The problem is that I don’t just change things like this without some thorough testing. I agree with your analysis but I have not had time to setup the tests to verify.
I’ve been bitten by theory versus practice one too many times to fix code I don’t use personally that is otherwise working fine. And without understanding why the original author wrote it in such a convoluted way in the first place (because you really have to work at it that in this case), I felt weird changing it.
Yeeh, I would have the same reservations in your place @pspeed. It would be really nice to know why it was written like that and if there is a real reason or just someone trying to be clever. I guess a version history search could find who the author is but that relies on them being around and contactable.
One thing I will say is that I’m not a comment fanatic but anything written like that really needs a comment explaining why!
@zarch said:
Yeeh, I would have the same reservations in your place @pspeed. It would be really nice to know why it was written like that and if there is a real reason or just someone trying to be clever. I guess a version history search could find who the author is but that relies on them being around and contactable.
One thing I will say is that I'm not a comment fanatic but anything written like that really needs a comment explaining why!
Dafuq is that code part… (just looked into it O.o
But I agree it seems to do exaclty what you said it does. #
(Also i don’t see the big problem in chanign this , at least its not like we change a stable version, and if its changed wrong we will know in a few days for sure.
→ Never be afraid to refactor code, it might break things, but it will make everything easier in the future. (And jme is not like a nuclear power plant control code or so, so worst case when we creak it , nothing!)
a) I don’t use the code and sort of have a personal rule about fixing working code in code I don’t use… but especially so in the case of:
b) the code seems to go out of its way to be as strange as possible… and I keep thinking there should be a reason. I know it’s possible (even likely) that there is no reason but it’s using features most Java developers wouldn’t even know existed. Of course, in the classes in my own code where I have things like two lines of code that must be in a specific order even though they look innocent enough, I leave half-page comments as to why.
Someday I (or someone) will get curious enough to check the history of that file.
I tracked down the history of that piece of code and found out it pretty much transformed into a scary beast over many revisions and code changes. Then I pulled it out from jME2 with some changes that made it slightly more complex …
The author is credited as “sunsett” in SVN. He is actually Matt Hicks, known as darkfrog on these forums. The original code I pulled out was this:
[java]public void execute() {
GameTask<?> task = queue.poll();
do {
if (task == null) return;
while (task.isCancelled()) {
task = queue.poll();
if (task == null) return;
}
task.invoke();
} while ((executeAll.get()) && ((task = queue.poll()) != null));
}[/java]
I added the loop label and removed usage of executeAll variable …
9 times out of 10 a loop label is a really bad smell. In nearly all cases I’ve seen it used it was unnecessary and made code more complicated… this one included.
So, would be Java developers. If you catch yourself doing a goto (loop label) then rethink what you are doing.
The thing is that you don’t want a task to execute and then execute another task that is added within the former task directly afterwards, the task should be executed in the next update loop. Thats why the loop became so complicated I guess.
Re bad smells: Loop labels are a somewhat bad smell, yes.
However, there was a worse one: the test for loop exit existed in three different places.
I tend to (re)write loops so that the exit test is in exactly one place.
That makes the loop far easier to understand.
Not just for humans (which would be important enough) but also for optimizers (which usually want to optimize code that follows the loop, and finding the commonalities of three loop-exiting code paths is always harder than simply assuming a single loop-exiting code path).
I’m fairly sure that the loop as it currently exists won’t give the behaviour you want anyway @normen.
[java]AppTask task = taskQueue.poll();
toploop: do {
if (task == null) break;
while (task.isCancelled()) {
task = taskQueue.poll();
if (task == null) break toploop;
}
task.invoke();
} while (((task = taskQueue.poll()) != null));
[/java]
If taskQueue contains t1, t2.
t1 executes, adds t3.
t2 is polled and executed
t3 is polled and executed
Loop exits
By my reading the queue always continues until it is drained.
In order to give the behaviour you are looking for you would need either something like an array list and remember the size when you go in to stop processing at or a “startTime” flag in the task object that gets queried to stop the execution.
Edit to add: Or two active queues and you flip them each frame - so one gets new tasks and one is used to process existing tasks. Personally I’d like it if enqueued tasks executed the following frame too but it is a semi-breaking change.
@toolforger said:
I just pulled the update.
Nicely done, @Momoko_Fan.
Re bad smells: Loop labels are a somewhat bad smell, yes.
However, there was a worse one: the test for loop exit existed in three different places.
I tend to (re)write loops so that the exit test is in exactly one place.
That makes the loop far easier to understand.
Not just for humans (which would be important enough) but also for optimizers (which usually want to optimize code that follows the loop, and finding the commonalities of three loop-exiting code paths is always harder than simply assuming a single loop-exiting code path).
Not only that but I'm fairly sure the loop was actually doing an un-needed double-check each run through.
[java]toploop: do {
if (task == null) break;
//Snip
} while (((task = taskQueue.poll()) != null));
[/java]
It was checking if task was null both on the (while) and then immediately after the start of the loop as an if. If it used a while() loop instead of a do() loop that could have been simpler!
@normen said:
The thing is that you don't want a task to execute and then execute another task that is added within the former task directly afterwards, the task should be executed in the next update loop. Thats why the loop became so complicated I guess.
Then it became complicated for no reason because it never ever did that. All queued tasks execute to completion even if queued during a task... in all versions of this loop.
Edit: and by the way that behavior is impossible anyway without two queues.
@pspeed said:
Then it became complicated for no reason
I think you found out that much already ^^ I was trying to find out why as draining a queue isn't exactly wizardry.
The old game task queue had some mechanism in place to execute only one task at a time, maybe that got mixed with some code to separate the tasks. I think I remember there were issues with people complaining the queue "doesn't work" because of this "blocking".
