Page MenuHomePhabricator

eo: add events to track the ownership status of an Eo object
Needs ReviewPublic

Authored by vitor.sousa on Apr 22 2019, 1:47 PM.

Details

Summary

Some user code may want to track an object ownership in regard to whether it is
kept by just one owner or shared between many owners.

This is specially true for code provided by bindings to other programming
languages, where different kinds of resource management may take place.

The event ownership,unique is triggered whenever the object refcount goes
from two to one, as a signal that it has just one owner from now on.

The event ownership,shared is triggered whenever the object refcount goes
from one to two, as a signal that it has multiple owners from now on.
It will not trigger when further increasing the refcount to any value beyond
two.

We also add benchmarks for sharing (i.e. increasing the refcount) and them
unsharing objects, in order to evaluate the performance impact of this patch.

Diff Detail

Repository
rEFL core/efl
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 11592
Build 8705: arc lint + arc unit
vitor.sousa created this revision.Apr 22 2019, 1:47 PM
vitor.sousa requested review of this revision.Apr 22 2019, 1:47 PM

An event fired every time any Eo object's refcount moves from 2 to 1 (or viceversa) won't have a significant performance impact?

Shouldn't we profile this?

I think this is a not so good idea in any case:

  • Whenever a user subscribes to this event, he will be in a hot-path, and this will 100% hamper the performance of everything.
  • Executing code in those event handlers can be a problem, and might end up in weird situations. (just imagine the event emission before the autounref, the object just tells you that its ownership is unique, just to kill itself)
  • Every time you emit this event, we will walk the *entire* list of subscribed event callbacks. (Another performance cirtical point)

I agree with the above comment. This is going to have serious performance impact. Could you explain what you are trying to do?

We can avoid the performance problems by creating a boolean if someone is registered in the event. The event won't get called most of the time because most objects have a parent anyway and the event gets called just for objects with C# wrappers.

Our problem is that c# wrapper owns a reference of the Eo object, so the Eo object must live as long as the reference. But, when the Eo is also owned by C code, e.g., when it has a parent, then the C# wrapper must also live as long as the Eo object because it may be a user class inheriting from it or it may contain callbacks/events defined in that wrapper in C# wrapper. We keep the C# alive as long as the Eo object has a shared ownership which would keep it alive anyway. And we unpin (if user sets to null, the wrapper can then get freed) when we know the C# is the only owner (unique).

As everyone here is concerned, it really gives performance problem a lot.
You can check the slowness by executing attached EFL# application easily.
(launching, scrolling, and resizing window .. everything is slow)

We might need to consider other ways.

As everyone here is concerned, it really gives performance problem a lot.
You can check the slowness by executing attached EFL# application easily.
(launching, scrolling, and resizing window .. everything is slow)

Thanks @woohyun for writing a custom targeted benchmark. This should be part of the goal to make this work nice and smooth! Along with expedite.

To have a trace of our discussion on IRC, we need benchmark to not get worse with this patch and the tests need to cover more case (efl_del() and efl_part()). With that we should be able to make better decision.

raster added a subscriber: raster.Apr 24 2019, 2:10 PM

what they said. this is going to be costly...

Results of expedite-cmp (I've patched it to aggregate the average values of the comparison files in the second column):

For 30 runs with this version of the patch: https://pste.eu/p/CshR.html

  • 2.9% average drop in the evas speed calculation
  • ~9% drop in most widget tests

For 20 runs with the version with the extra patch from devs/vitorsousa/eo_ownership_events adding a shield flag: https://pste.eu/p/O7rN.html

  • 0.5% average drop in the evas speed calculation
  • 2.5~5% drop in most widget tests

@lauromoura

If we don't have any other refactoring point on this patch, this looks a bit risky - as the results of your test are showing.
And, I am concerned that applications would be much slower whenever they create more objects while executing.

bu5hm4n requested changes to this revision.Apr 25 2019, 4:55 AM

(Request changes for now because of this, otherwise this shows up as needs review)

This revision now requires changes to proceed.Apr 25 2019, 4:55 AM
vitor.sousa planned changes to this revision.Apr 25 2019, 8:23 AM

Avoid calling OWNERSHIP_UNIQUE while invalidating
Add a boolean flag for performance optmization reasons

vitor.sousa planned changes to this revision.EditedApr 25 2019, 8:31 AM

Still needing a more in-depth analysis about the impact of this patch in 'efl_part'.
Still working in some optimization aspects.

expedite is not the right test for this. things like elementary - like genlist and things that create, destroy and move objects in and out of parents and have complex call trees - those are the things u need to look at

benchmarks and some updates

vitor.sousa edited the summary of this revision. (Show Details)Thu, May 23, 11:47 AM

What are the result benchmarking this?

We've tried measuring the eo benchmarks for it using the release build[1] on an dell xps 15 9560 running ubuntu 18.10.

There are some results in the link[2] below. The graph shows the ratio of the mean of 50 runs of the patched over the mean of 50 runs of upstream code. There was waaaay to much noise in lower number of objects to get significant results for these cases. The upstream code alone got wildy different results on each run.

One of the tests we added to go through the code path in this patch (shared_ownership_alternative) creates a number of objects with refcount 1, increment (making them shared) and decrement (making them unique). For this one only we got a rather constant 2% slower performance as we increase the number of objects.

[1] meson build-mono-release -Dbindings=cxx,mono -Dmono-beta=true -Dprefix=/opt/efl-e -Dinstall-eo-files=true --buildtype=release -Dc_args="-march=native -DNDEBUG=1"
[2] https://imgur.com/a/Q10LaGJ