Page MenuHomePhabricator

ecore: fix that timers are not called in the order they were registered.
ClosedPublic

Authored by eagleeye on Jul 30 2018, 10:29 PM.

Details

Summary

Timers are not called in the order they were registered.
Because when current timer is deleted, getting next timer is called twice.

Test Plan

<error>
Timer1 expired after 0.001 seconds.
Timer3 expired after 0.001 seconds.
Timer5 expired after 0.001 seconds.
Timer7 expired after 0.001 seconds.
Timer2 expired after 0.001 seconds.
Timer6 expired after 0.001 seconds.
Timer4 expired after 0.001 seconds.
Timer8 expired after 0.001 seconds.
<correct>
Timer1 expired after 0.001 seconds.
Timer2 expired after 0.001 seconds.
Timer3 expired after 0.001 seconds.
Timer4 expired after 0.001 seconds.
Timer5 expired after 0.001 seconds.
Timer6 expired after 0.001 seconds.
Timer7 expired after 0.001 seconds.
Timer8 expired after 0.001 seconds.|

Diff Detail

Repository
rEFL core/efl
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
eagleeye created this revision.Jul 30 2018, 10:29 PM

It seems that this patch has no reviewers specified. If you are unsure who can review your patch, please check this wiki page and see if anyone can be added: https://phab.enlightenment.org/w/maintainers_reviewers/

eagleeye requested review of this revision.Jul 30 2018, 10:29 PM
eagleeye edited the test plan for this revision. (Show Details)Jul 30 2018, 11:24 PM
eagleeye added reviewers: Hermet, Jaehyun_Cho.
eagleeye edited the test plan for this revision. (Show Details)
eagleeye edited the test plan for this revision. (Show Details)
eagleeye edited the test plan for this revision. (Show Details)Jul 30 2018, 11:29 PM
zmike edited projects, added Restricted Project; removed efl.Jul 31 2018, 6:29 AM

I'm doing a deep review of this patch right now. Will report back soon...

zmike requested changes to this revision.Jul 31 2018, 8:07 AM

A few items:

  • You're definitely correct that timers do get called out of order now; this is a direct result of (more) regressions created in rEFL5dd52fd09b7d79c70b3134423a87aa6400a2d994
  • This issue is caused by only the first part (pd->loop_data->timer_current being changed) of _efl_loop_timer_util_loop_clear() and the remainder of that function is actually correct to be called here
  • This should have a unit test added to check timer ordering

So I think the patch should be changed to just copy in the second part of the function.

This revision now requires changes to proceed.Jul 31 2018, 8:07 AM

@zmike Could u please more clear this sentence? "So I think the patch should be changed to just copy in the second part of the function."

zmike added a comment.Jul 31 2018, 8:36 PM

The part starting with // Remove the timer from all possible pending list (~line 400) until the end of the function should be copied into _efl_loop_timer_efl_object_parent_set at ~line 468 replacing the _efl_loop_timer_util_loop_clear() call.

eagleeye updated this revision to Diff 16025.Aug 1 2018, 4:00 AM

fix patch and make test case.

eagleeye updated this revision to Diff 16026.Aug 1 2018, 4:04 AM

fix test case.

zmike added a comment.Aug 1 2018, 7:13 AM

Seems good other than one questionable addition. Awaiting reply/changes before merging.

src/lib/ecore/ecore_timer.c
567

Hold up, why was this added?

eagleeye added inline comments.Aug 1 2018, 10:42 PM
src/lib/ecore/ecore_timer.c
567

After timer is expired and timer is deleted by user callback, timer is added to list again.
Sure, it is removed when efl_unref is called. but I wanna prevent unnecessary process.

zmike accepted this revision.Aug 2 2018, 6:14 AM
zmike added inline comments.
src/lib/ecore/ecore_timer.c
567

Okay, I just wanted to make sure this didn't slip in accidentally!

This revision is now accepted and ready to land.Aug 2 2018, 6:14 AM
Closed by commit rEFLa92274f81184: ecore: fix that timers are not called in the order they were registered. (authored by Hosang Kim <hosang12.kim@samsung.com>, committed by zmike). · Explain WhyAug 2 2018, 6:17 AM
This revision was automatically updated to reflect the committed changes.
raster added a subscriber: raster.Aug 6 2018, 6:32 PM

You're definitely correct that timers do get called out of order now; this is a direct result of (more) regressions created in rEFL5dd52fd09b7d79c70b3134423a87aa6400a2d994

There never was a guarantee of order other than by time slot.if multiple timers go off at the same time lot, no order was implied or guaranteed.

This should have a unit test added to check timer ordering

I would say it doesn't because it never was a guarantee. Same with event callbacks. No guarantee of order called EXCEPT when you use priority to force ordering. making guarantees of ordering like this (without them being explicit) makes it harder to optimize in future.