Page MenuHomePhabricator

edje/cache: Refactor _edje_cache_file_coll_open()
ClosedPublic

Authored by smohanty on Aug 22 2019, 5:08 PM.

Details

Summary

This function does lot of things

  • can be called only to load the file (by passing coll as null)
  • can be called to load both file and open the collection from the file.
  • handles the file_cache logic
  • handles fixing the collection after reading from file.

this patch is targeting to split the responsibility to
smaller function for easy maintenance and code readability.

future patch to follow for splitting the file opening and collection
opening to two different function.

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.
smohanty created this revision.Aug 22 2019, 5:08 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/

smohanty requested review of this revision.Aug 22 2019, 5:08 PM
smohanty updated this revision to Diff 24412.Aug 22 2019, 5:28 PM

unused var removed.

smohanty updated this revision to Diff 24413.Aug 22 2019, 5:37 PM

edje/optimization: replace eina_list with eina_array.

zmike requested changes to this revision.Aug 23 2019, 7:16 AM
zmike added a subscriber: zmike.

I think the list -> array should be broken out into a separate patch (using git-phab). Also some other minor nits. Seems fine otherwise though.

For a future optimization, it might be possible to convert _edje_file_cache to a hash. One idea would be if we add something like a global edje file counter to tag newly-accessed files in the cache (with a struct member) and then use this tag to sort cache members into an array and prune the least-recently-used ones in _edje_cache_file_clean() since that's the only place where it needs to be a list and it likely is not triggered very frequently.

src/lib/edje/edje_cache.c
163

Why not just add this to eina now?

566

Is there a reason why this can't just be done during edje_init?

580

I think trash_pop would be a little more descriptive for this?

638–639

Is this implying that the function should potentially be renamed or...?

This revision now requires changes to proceed.Aug 23 2019, 7:16 AM
smohanty added inline comments.Aug 23 2019, 8:01 AM
src/lib/edje/edje_cache.c
163

Well Eina means public Api ,test and docs :) .. so this patch will be blocked for long time :)

566

In current code we delete the hash in _edje_cache_file_unref() function.
Although I don't see any reason why it can't move to edje_init() maybe next patch ?

580

I don't have any reservation on the name. As we may not find the file inside the trash . though of keeping same name as trash_find() (same as cache_find() ) :)

If trash_pop sounds better will update the patch.

638–639

we read the collection from edje file in _edje_file_coll_open() on first request and then we cache it there and just return the edc object. and this collection_fix() should be done for the first time when we read from the eet file (after that we update the eet->checked flag). So ideally this function should move inside edje_file_coll_open().
I just added this comment to get some feedback. if every one agrees will move this function to _edje_file_coll_open().

In D9715#180358, @zmike wrote:

I think the list -> array should be broken out into a separate patch (using git-phab). Also some other minor nits. Seems fine otherwise though.

Actually I created a separate patch for it . As it based on the first patch , when I did arc diff it just combined it to one phab ID :(. if you know how to create separate phab id please let me know.

For a future optimization, it might be possible to convert _edje_file_cache to a hash. One idea would be if we add something like a global edje file counter to tag newly-accessed files in the cache (with a struct member) and then use this tag to sort cache members into an array and prune the least-recently-used ones in _edje_cache_file_clean() since that's the only place where it needs to be a list and it likely is not triggered very frequently.

Yes, the first step is to split it into smaller function so that it is easy to understand . then we can see how to improve further . now edje functions are so big that kind of scary to look at it :)

zmike added a comment.Aug 23 2019, 8:23 AM
In D9715#180358, @zmike wrote:

I think the list -> array should be broken out into a separate patch (using git-phab). Also some other minor nits. Seems fine otherwise though.

Actually I created a separate patch for it . As it based on the first patch , when I did arc diff it just combined it to one phab ID :(. if you know how to create separate phab id please let me know.

There's docs here https://phab.enlightenment.org/w/arcanist/

For a future optimization, it might be possible to convert _edje_file_cache to a hash. One idea would be if we add something like a global edje file counter to tag newly-accessed files in the cache (with a struct member) and then use this tag to sort cache members into an array and prune the least-recently-used ones in _edje_cache_file_clean() since that's the only place where it needs to be a list and it likely is not triggered very frequently.

Yes, the first step is to split it into smaller function so that it is easy to understand . then we can see how to improve further . now edje functions are so big that kind of scary to look at it :)

Yes, that's very true.

src/lib/edje/edje_cache.c
163

We're not currently in a feature freeze?

566

Yeah, I meant in a future patch; sorry, should have been more clear.

580

I don't have a strong opinion, pop is just the verb we have used for recovering items from a trash.

638–639

Ahhh okay, I see. Then yes, i think it can be moved for improved clarity.

smohanty updated this revision to Diff 24476.Aug 25 2019, 6:14 PM

updated review comment.

smohanty marked 4 inline comments as done.Aug 25 2019, 6:18 PM
smohanty added inline comments.
src/lib/edje/edje_cache.c
163

have raised a patch for eina_array_find. once thats get landed will add the patch (changing from list to array).

566

cool

580

have update the patch renaming to trash_pop()

638–639

updated the patch by moving collection_fix() to edje_file_coll_open()

zmike accepted this revision.Aug 26 2019, 5:08 AM
This revision is now accepted and ready to land.Aug 26 2019, 5:08 AM
Closed by commit rEFL02b63b826006: edje/cache: Refactor _edje_cache_file_coll_open() (authored by subhransu mohanty <sub.mohanty@samsung.com>, committed by zmike). · Explain WhyAug 26 2019, 5:31 AM
This revision was automatically updated to reflect the committed changes.