Make QueueOptimizer easier to read. #5008
Conversation
| var i, ii = argsArray.length; | ||
| var state; | ||
| for (i = 0; i < ii; i++) { | ||
| var state = undefined; |
There was a problem hiding this comment.
Travis complains about this:
src/core/evaluator.js: line 2577, col 11, It's not necessary to initialize 'state' to 'undefined'.
|
I fixed the lint error. |
|
/botio test |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.22.172.223:8877/7e792041d5197eb/output.txt |
From: Bot.io (Linux)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.21.233.14:8877/f76d0e41bdde08d/output.txt |
From: Bot.io (Linux)FailedFull output at http://107.21.233.14:8877/f76d0e41bdde08d/output.txt Total script time: 0.11 mins |
From: Bot.io (Windows)FailedFull output at http://107.22.172.223:8877/7e792041d5197eb/output.txt Total script time: 2.80 mins
Image differences available at: http://107.22.172.223:8877/7e792041d5197eb/reftest-analyzer.html#web=eq.log |
|
The test failures are all due to infrastructure problems, AFAICT. |
|
@nnethercote Yes, they are. The bots will have to be kicked by @yurydelendik. |
| var iFirstPIIXO = curr - 1; | ||
|
|
||
| // Look for the quartets. | ||
| var i = iFirstSave + 4; |
There was a problem hiding this comment.
would curr + 1 be better?
There was a problem hiding this comment.
I think iFirstSave + 4 is clearer. |curr| is really non-descriptive, which is why I move away from it and start using descriptive names immediately.
|
i gave a shot at reviewing this. It is quite exhausting to stay concentrated :) The new variables help. it is really hard to spot various +2 and its good that j+2 now has a better name. There is still a lot of duplicate code, and I wonder if it could be somehow extracted. However the new code at least says a bit clearer what it does, thus making changes less risky. I am not sure about the use of [i + 0] which is made to make it look nice and symmetrical. As said, I think the code still does what it did before. |
|
|
||
| // At this point, i is the index of the first op past the last valid | ||
| // quartet. | ||
| var count = Math.min((i - iFirstSave) / 4, MAX_IMAGES_IN_BLOCK); |
There was a problem hiding this comment.
i like the >> 2 better, but i assume you want to make clear you deal with quartets right?
|
Given that most, if not all, of this code was written by @yurydelendik, I think that it would be a good idea if he reviewed these changes. |
|
Thanks for the review. This code is tricky, so I agree that waiting for Yury to get back is wise. Do you know how long he's away for? As for the remaining duplication... I feel that although the code for each addState() function is similar, it's not similar enough for there to be a factorisation that would improve the readability. I'll address the remaining minor comments on Monday. Thanks for the review. |
QueueOptimizer is really hard to read. Enough so that it's blocking my efforts to streamline the representation used for operator lists. This patch improves its readability in the following ways. - More descriptive variable names make the sequence checking much clearer, as do additional comments. - The addState() functions now return the index of the first op past the sequence, instead of setting context.currentOperation to the last op of the sequence. - The loop in optimize() is clearer. - The array modification in the fourth addState() function is much clearer -- we're just removing trios of ops. - All four |addState| functions are now more consistent with each other. I used some debug printfs to find documents where these optimizations are used and then checked that the number of optimized ops was the same before and after my changes.
|
I converted the a[i + 0] instances to a[i]. |
|
@yurydelendik: any progress here? |
|
I'll check this PR Monday |
|
/botio-linux preview |
From: Bot.io (Linux)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://107.21.233.14:8877/fcfae286da93039/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/fcfae286da93039/output.txt Total script time: 0.74 mins Published
|
|
/botio test |
From: Bot.io (Linux)ReceivedCommand cmd_test from @yurydelendik received. Current queue size: 0 Live output at: http://107.21.233.14:8877/02d17b767397132/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @yurydelendik received. Current queue size: 0 Live output at: http://107.22.172.223:8877/aa94809ecc73472/output.txt |
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/aa94809ecc73472/output.txt Total script time: 19.79 mins
|
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/02d17b767397132/output.txt Total script time: 22.69 mins
|
Make QueueOptimizer easier to read.
|
Thank you |
QueueOptimizer is really hard to read. Enough so that it's blocking my
efforts to streamline the representation used for operator lists.
This patch improves its readability in the following ways.
as do additional comments.
sequence, instead of setting context.currentOperation to the last op of
the sequence.
-- we're just removing trios of ops.
I used some debug printfs to find documents where these optimizations are
used and then checked that the number of optimized ops was the same before
and after my changes.