Page MenuHomePhabricator

alternative approach to D6791
Needs ReviewPublic

Authored by zmike on Aug 9 2018, 10:00 AM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None

Diff Detail

Repository
rEFL core/efl
Branch
master
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 7301
zmike created this revision.Aug 9 2018, 10:00 AM

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/

zmike requested review of this revision.Aug 9 2018, 10:00 AM

This is the total of the changes required to achieve the same sort of behavior as D6791 (although this version is consistently ~10% slower).

The claim that this version would be less code seems to definitively be inaccurate:

D6806

src/lib/ecore/ecore_private.h    |  2 ++
src/lib/ecore/ecore_timer.c      | 47 +++++++++++++++++++++++++++++++++++++++--------
src/lib/ecore/efl_loop.c         | 15 +++++++++++++++
src/lib/ecore/efl_loop.eo        |  8 ++++++++
src/lib/edje/edje_program.c      |  5 ++---
src/tests/elementary/elm_suite.c | 18 ++++++++++++++++--
src/tests/elementary/elm_suite.h |  4 ++++
7 files changed, 86 insertions(+), 13 deletions(-)

D6791

src/tests/elementary/elm_suite.c | 42 +++++++++++++++++++++++++++++++++++++-----
src/tests/elementary/elm_suite.h |  5 +++++
2 files changed, 42 insertions(+), 5 deletions(-)

Moreover, this version, while indeed providing a "more general" implementation that could be reused, has a number of significant issues:

  • requires modification to existing edje_cc functionality (even though the existing functionality makes no sense)
  • requires invasive modifications to timer implementation
  • adds more api which can (and likely will) be disastrously misused

By contrast, D6791 has only one issue:

  • uses multiple loop objects

Given that this is not a functionality which I would expect to be reused outside of unit tests, where it could trivially be broken out into a standalone function to hook into test suites as needed, it seems to me that D6806 is objectively worse than D6791. The lone downside of D6791 can easily be mitigated by adding a unit test of concentric loop objects to the efl_app suite, thus enforcing this behavior. Relatedly, if loop objects are not meant to be used outside of the main loop then it seems like you've wasted an awful lot of time creating the implementation for them instead of just writing a simple wrapper for existing code?

raster added a subscriber: raster.Aug 10 2018, 2:45 PM

see a above. i think it's simpler just done where i originally mentioned - i the time get's and select mechanism of ecore. someone also has one this "let's run time at a different speed" as an LD_PRELOAD:

https://github.com/vi/timeskew

doesn't do poll though...

src/lib/ecore/ecore_private.h
167

make it a global, not a loop struct memeber

279

this func not needed.

src/lib/ecore/ecore_timer.c
127

this whole function seems unnecessary if there is a single speed factor for the process which is the sane way to go.

144

as above - unnecessary

259

I wouldn't do this here. i'd do this in the timer core that evaluates the next sleep time and timers that should expire then, so this bit of code would not be needed.

492

this change is not needed.

680

this also not needed if done as an env var as suggested for a whole process.

src/lib/ecore/efl_loop.c
318

make this a single global, not loop struct var (same mount of code, just different var)

444

api's not needed if ecore init just grabs an env var and sets a global.

src/lib/ecore/efl_loop.eo
85

not needed.

src/lib/edje/edje_program.c
8

why do we have to change anything in edje if we are silently updating time and the speed it runs at inside ecore?

420

same as above

src/tests/elementary/elm_suite.c
101

all of this below could be a putenv("ECORE_TIME_SCALE=0.0002"); or something similar

I took some time to consider and explore your idea since I had been having some difficulty reconciling what you were proposing with the actual functionality required. I'll ignore your inlined comments, since they're not actually related to this patch, and are effectively proposing an entirely different approach from the one in this patch.

After consideration, there are a number of issues with your proposed approach:

  1. As I have said, I am strongly opposed to providing any such mechanism in ecore/efl itself, as this will almost certainly be abused in some way at a later time. This is a stopping point for me, and I would rather have longer test runtimes than do anything related to adding such functionality.
  2. I have tested the exact method that you have proposed, modifying the return of time_get(). I have also tested the timeskew project that you cited. Neither of these approaches have the same result that I have been trying to achieve, namely reducing the runtime of elm_suite. The simplest way to verify this would be for you to try it yourself:
    • clone timeskew
    • make in timeskew directory
    • TIMING_ENABLED=1 TIMESKEW="500 1" LD_PRELOAD=timeskew/libtimeskew.so.0.0.0 tests/elementary/.libs/elm_suite
    • The resulting runtime for the suite is slightly reduced...maybe? It's such a small change that it could just as easily be within the margin of error.
    • I even manually added a poll() wrapper and there was no change

If you can provide a viable alternative to D6791 then I'm willing to consider and investigate it, but "using multiple loop objects" does not seem like something which should be considered a blocker; this is an entirely valid use of the provided API. An untested use, perhaps, but test cases are easy to write and verify.