Page MenuHomePhabricator

edje/optimization: keep a style hash for fast retrival of styles
ClosedPublic

Authored by smohanty on Aug 11 2019, 9:00 PM.

Details

Summary

As edje mostly deals with style string. to get the style data each time
it linearly search through list to find out the style which is not very
cache friendly so keep a hash to do first lookup with less impact on cache.

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 11 2019, 9:00 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 11 2019, 9:00 PM
ali.alzyod added inline comments.Aug 12 2019, 12:02 AM
src/lib/edje/edje_cache.c
382

I am not sure, could stl->name be NULL ?

src/lib/edje/edje_textblock_styles.c
310–311

This does not compile

smohanty updated this revision to Diff 24028.Aug 12 2019, 12:22 AM
smohanty marked 2 inline comments as done.

fixed build error.

smohanty added inline comments.Aug 12 2019, 12:23 AM
src/lib/edje/edje_cache.c
382

well i put that not to get a review comment :) just followed the coding style below. will update the patch

src/lib/edje/edje_textblock_styles.c
310–311

my bad .. will update the patch .

smohanty marked 2 inline comments as done.Aug 12 2019, 12:23 AM
ali.alzyod added inline comments.Aug 12 2019, 1:08 AM
src/lib/edje/edje_cache.c
382

I did understand the comment :)
I added this comment because in edje_textblock_styles.c file:

EINA_LIST_FOREACH(ed->file->styles, l, stl)
 {
    if ((stl->name) &&
    (stl->name == style || !strcmp(stl->name, style))) break;
     stl = NULL;
 }
smohanty marked an inline comment as done.Aug 12 2019, 2:01 AM
smohanty added inline comments.
src/lib/edje/edje_cache.c
382

This goes through the styles list and check if the style name matches with the style_name we are requesting and if it finds then it returns the Edje_Style Data pointer .
As you see for each style we query it search the entire style list in the file .(which is slow)

By keeping a hash (style_name, Edje_Style*) we can query the Data using stylle_name , hence changed this code to simple hash_find()

smohanty added inline comments.Aug 12 2019, 2:03 AM
src/lib/edje/edje_cache.c
382

sorry ignore this comment . I miss read your comment and replied something else :)

raster requested changes to this revision.Aug 12 2019, 6:28 AM

i see no code to free the hash when edf is freed... :) also yes - doesn't handle building the hash if stl->name is null properly :) in theory the incoming edj file could be malformed with null style names... so handle the case.

This revision now requires changes to proceed.Aug 12 2019, 6:28 AM
smohanty updated this revision to Diff 24045.Aug 12 2019, 4:18 PM

free style_hash on file_close().

bu5hm4n requested changes to this revision.Aug 18 2019, 6:16 AM
bu5hm4n added a subscriber: bu5hm4n.
bu5hm4n added inline comments.
src/lib/edje/edje_cache.c
383

I think you need to call eina_hash_set here. If stl->name is twice the same, the hash will be working in a undefined manner.

This revision now requires changes to proceed.Aug 18 2019, 6:16 AM
smohanty marked an inline comment as done.Aug 18 2019, 5:28 PM
smohanty added inline comments.
src/lib/edje/edje_cache.c
383

The whole idea of using eina_hash_direct_add() is not to store the style_name in the hash.and if at all the edje comes with 2 style with same name .. maybe both will be inserted into the hash and during search the first one will be returned. So the proper check and fix should be in the edje_cc not here.

smohanty marked an inline comment as done.Aug 18 2019, 5:29 PM
cedric added inline comments.Aug 18 2019, 5:34 PM
src/lib/edje/edje_cache.c
383

You can not assume that a file has been correctly compiled as a malign user could use that to corrupt an application.

smohanty added inline comments.Aug 18 2019, 5:56 PM
src/lib/edje/edje_cache.c
383

Am not sure what the eina_hash will do in case if am trying to add the same key with different data . If it replaces then the last edje_style will be the style , if it doesn't add the data then the first one will be there .. So i think this won't be a problem because we are only using this hash for lookup not managing the lifetime of the style data.

let me know if am missing something.

smohanty requested review of this revision.Aug 18 2019, 8:18 PM
smohanty marked an inline comment as done.
bu5hm4n requested changes to this revision.Aug 18 2019, 10:48 PM
bu5hm4n added inline comments.
src/lib/edje/edje_cache.c
383
This revision now requires changes to proceed.Aug 18 2019, 10:48 PM

@bu5hm4n ,

I would appreciate if you give little bit more technical  information how it will result into undefined behaviour so that we can understand the problem (As not every one is an expert of Eina_Hash Internals). 

and if we agree that the current patch may result into undefined behaviour then maybe we will also fix the below code as well (in the same function).

 edf->color_hash = eina_hash_string_small_new(NULL);
 EINA_LIST_FOREACH(edf->color_classes, l, cc)
   if (cc->name)
     eina_hash_direct_add(edf->color_hash, cc->name, cc);

 edf->text_hash = eina_hash_string_small_new(NULL);
 EINA_LIST_FOREACH(edf->text_classes, l, tc)
    if (tc->name)
       eina_hash_direct_add(edf->text_hash, tc->name, tc);

 edf->size_hash = eina_hash_string_small_new(NULL);
 EINA_LIST_FOREACH(edf->size_classes, l, sc)
   if (sc->name)
     eina_hash_direct_add(edf->size_hash, sc->name, sc);

I do not know the internals of eina_hash i am just going with the documentation here. There have been annoying issues in the past with this, as debugging this is quite ... hard.
I totally agree, we also need to fix the cases you linked.

I see two possibilities fixing it, either check with eina_hash_find if the key is already in the hash, or use eina_hash_set (which does copy the key again).

I do not know the internals of eina_hash i am just going with the documentation here. There have been annoying issues in the past with this, as debugging this is quite ... hard.
I totally agree, we also need to fix the cases you linked.

I see two possibilities fixing it, either check with eina_hash_find if the key is already in the hash, or use eina_hash_set (which does copy the key again).

Well If you read the document carefully

  • Failure to use sufficient uniqueness will result in unexpected results when inserting data pointers accessed with @ref eina_hash_find(), and removed with @ref eina_hash_del().**

As far as i understood its talking about some special use case.

There is only 3 scenario when hash has already same key

  1. it will not insert the data and return
  2. it will replace the data .

3, it will keep the old data and add the new data.
As long as eina_hash_find() gives a data against the key my use case will work.

maybe @raster or @cedric can comment more on it.

I see two possibilities fixing it, either check with eina_hash_find if the key is already in the hash, or use eina_hash_set (which does copy the key again).

I don't like any of the solution you mentioned .

Collision handling in hash is normal (passing same pointer will be just a collision case) so my guess is that it will just append the data and in hash_find() it will just find the first data and return.

Collision handling in hash is normal (passing same pointer will be just a collision case) so my guess is that it will just append the data and in hash_find() it will just find the first data and return.

The behavior of Eina_Hash should be considered to return a randomly selected answer with the right key in the set of all the inserted item with the same key. As we use an RBTree internally, we do have random reorder of the item depending on what get inserted in the tree. If you think this behavior is fine, then I am fine with it. From my understanding, it could just give a different visual output and should not lead to any potential crash.

smohanty requested review of this revision.Aug 19 2019, 6:30 PM

Collision handling in hash is normal (passing same pointer will be just a collision case) so my guess is that it will just append the data and in hash_find() it will just find the first data and return.

The behavior of Eina_Hash should be considered to return a randomly selected answer with the right key in the set of all the inserted item with the same key. As we use an RBTree internally, we do have random reorder of the item depending on what get inserted in the tree. If you think this behavior is fine, then I am fine with it. From my understanding, it could just give a different visual output and should not lead to any potential crash.

As we are creating this Hash for a Edje_File once . and don't modify it again whatever data the Eina_hash returns for a duplicate key it will always return the same . Its the same behaviour with current code (returns the first Style matches the style string ).
So I think this patch is not going to introduce any new behaviour than what is currently exists.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 20 2019, 10:40 AM
Closed by commit rEFL10501b170fcf: edje/optimization: keep a style hash for fast retrival of styles (authored by subhransu mohanty <sub.mohanty@samsung.com>, committed by cedric). · Explain Why
This revision was automatically updated to reflect the committed changes.