Page MenuHomePhabricator

elementary: add a factory that handle caching for you.
ClosedPublic

Authored by cedric on Dec 7 2018, 2:49 PM.

Details

Summary

This factory handle caching of one type of object and automatically empty the cache
when the application goes into pause.

Creating object is costly and time consuming, keeping a few on hands for when you next will need them help a lot.
This is what this factory caching infrastructure provide. It will create the object from the class defined on it,
set the parent and the model as needed for all items a Factory create. The View has to release the Item using the
release function of the Factory interface for all of this to work properly.

This is copying what Elm_Genlist was doing for you in the background and bring Efl interface to parity.

Depends on D7442

Diff Detail

Repository
rEFL core/efl
Branch
T7485-devs/cedric/caching_factory
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8436
cedric created this revision.Dec 7 2018, 2:49 PM
cedric requested review of this revision.Dec 7 2018, 2:49 PM
cedric updated this revision to Diff 17915.Dec 14 2018, 10:25 AM
cedric edited the summary of this revision. (Show Details)

At the risk of repeating myself... This patch seems to be introducing a new feature: a caching factory. Therefore, I would expect some explanation of at least:

  1. Why is this needed? i.e. what problem are you trying to solve?
  2. How are you solving it? i.e. a high-level explanation of how this feature works: what is exactly being cached, what new API is being introduced, how does it work (examples!), ...
  3. An evaluation of how your patch solved the problem, i.e. tests which were failing before and are working now, or, in this case, performance numbers before and after this patch.

I know this all sounds academic, but it is not. Without the above description I have no idea of what are you trying to achieve with this patch, so I cannot review it. The only thing I have is the title and a very short summary, and since English is not the primary language for most of us, such short texts are prone to confusion.
Sure, I can search for typos, memleaks and low-level programming errors, but I cannot say if the patch is right or wrong, or if it fits in the bigger picture.

Ideally, any non-trivial commit should include the above information in some way or another.

Sooo... sorry for being so pedantic. I believe all this is for the best :)

cedric updated this revision to Diff 18072.Dec 21 2018, 11:23 AM
cedric edited the summary of this revision. (Show Details)

Add more explanation on what this Factory is about.

Sorry, but I still don't fully understand how this works. Is it that users of this factory, instead of creating objects and then destroying them, they return the objects to the factory, which keeps them for future use?
If this is the case, then I think it is worth mentioning in the summary (and in the EO docs!).
Is Genlist the typical use of this new class? Then I also think it is worth mentioning this example because it shows very clearly where the cache is handy.

Finally... Is there any test for this that I missed? Or a benchmark?

src/lib/efl/interfaces/efl_cache_item.eo
2

Do you think that Efl.Cached.Item would be more descriptive?

src/lib/elementary/efl_ui_caching_factory.c
14

Isn't this commonly called Most Recently Used (MRU)?

173

*As all the objects in the cache have the factory as parent, there's no need to unparent them

src/lib/elementary/efl_ui_caching_factory.eo
4

The same extended description you added to the diff's summary could be written here, and then it would appear in the final documentation.

Sorry, but I still don't fully understand how this works. Is it that users of this factory, instead of creating objects and then destroying them, they return the objects to the factory, which keeps them for future use?

Oh, that's how Efl.Ui.Factory API is designed and how View that use Factory have to use it. Just that before the caching behavior was left over to each factory while with this class is it now generic (I by left to every factory, none where really implementing it).

If this is the case, then I think it is worth mentioning in the summary (and in the EO docs!).
Is Genlist the typical use of this new class? Then I also think it is worth mentioning this example because it shows very clearly where the cache is handy.

The Factory top class is itself designed for Genlist type of use case (covering gengrid, tree, whatever long list view).

Finally... Is there any test for this that I missed? Or a benchmark?

Nop, I am mostly implementing infrastructure and hoping that it match @SanghyeonLee and @felipealmeida needs. Basically refactoring and cleaning up concept that where existing in Elementary legacy code.

src/lib/efl/interfaces/efl_cache_item.eo
2

Maybe, I am personally not a fan of the namespace itself, but I have no better idea at this point. Will update the patch with your suggestion.

src/lib/elementary/efl_ui_caching_factory.c
14

Dah ! :-) Thanks ! I have a hard time finding the right name for things. If it was me, everything would be named bob...

cedric updated this revision to Diff 18128.Dec 27 2018, 4:06 PM

Rebase and include requested improvement.

Just some minor documentation comments.

However, now I have a fundamental doubt. Is this really a factory that removes the MRU item from the cache??? Caches usually remove the LRU item (the LEAST-recently used)...

src/lib/efl/interfaces/efl_cached_item.eo
10 ↗(On Diff #18128)

This needs docs too, otherwise we get a hole in the generated pages :)

src/lib/elementary/efl_ui_caching_factory.eo
15

Mehhhh... lemme do that for ya:

Efl Ui Factory that provides object caching.

This factory handles caching of one type of object and automatically empties the cache
 when the application goes into pause.

 Creating objects is costly and time consuming, keeping a few on hand for when you next will need them helps a lot.
 This is what this factory caching infrastructure provides. It will create the object from the class defined on it and
 set the parent and the model as needed for all created items. The View has to release the Item using the
 release function of the Factory interface for all of this to work properly.

 The cache might decide to flush itself when the application event pause is triggered.
32

Missing doc. Probably the setter's doc above belongs here.

42

Missing doc. Probably the setter's doc above belongs here.

cedric updated this revision to Diff 18158.Dec 28 2018, 11:32 AM

Rebase and include review fixes.

I have no further comments regarding docs.
However, I am still curious, is this really a factory that removes the MRU item from the cache??? Caches usually remove the LRU item (the LEAST-recently used)...

However, I am still curious, is this really a factory that removes the MRU item from the cache??? Caches usually remove the LRU item (the LEAST-recently used)...

Hum, I am very confused now :-D It is adding at the tail of the list and pulling it out in the front, but I think it should actually be an MRU and push new object on the front of the list. Every object are identical and there is no reason to walk the list in one direction or another. Direct access to the top of the list sounds best in all case to me.

zmike added a subscriber: zmike.Jan 2 2019, 10:11 AM

This is a really minor nit, but shouldn't the naming here be efl.item.cached and efl.ui.factory.caching?

In D7443#132165, @zmike wrote:

This is a really minor nit, but shouldn't the naming here be efl.item.cached and efl.ui.factory.caching?

There is an ongoing discussion, which I think has been mostly settled, to follow the pattern of ListView, GridView and so on. So following that pattern, I am adopting that, which might not be best in all case. I think Factory is better if we follow Efl.Ui.CachingFactory, but Efl.Item.Cached seems better. Arguably the later one is standing up in its own namespace, so that might be a problem there.

@SanghyeonLee, @felipealmeida could you guys share some of your opinion on this patch?

zmike added a comment.Jan 2 2019, 10:35 AM

CachingFactory seems good.

In D7443#132170, @zmike wrote:

CachingFactory seems good.

Efl.Ui.Caching_Factory ?

We always have splitted the names with '_'

zmike added a comment.Jan 2 2019, 10:42 AM
In D7443#132170, @zmike wrote:

CachingFactory seems good.

Efl.Ui.Caching_Factory ?

We always have splitted the names with '_'

Saying "always" in this context is a bit misleading since none of the interfaces API is released/stable API. CamelCase plus underscore is pretty gross, we should consider reducing this usage in things which can potentially propagate to bindings and make those bindings gross.

In D7443#132175, @zmike wrote:
In D7443#132170, @zmike wrote:

CachingFactory seems good.

Efl.Ui.Caching_Factory ?

We always have splitted the names with '_'

Saying "always" in this context is a bit misleading since none of the interfaces API is released/stable API. CamelCase plus underscore is pretty gross, we should consider reducing this usage in things which can potentially propagate to bindings and make those bindings gross.

Sorry for throwing confusion here. In .Eo we will have Efl.Ui.Caching_Factory, in C we will have efl_ui_caching_factory, in C# Efl.Ui.CachingFactory. Every language can adapt to there preferred case.

In D7443#132175, @zmike wrote:
In D7443#132170, @zmike wrote:

CachingFactory seems good.

Efl.Ui.Caching_Factory ?

We always have splitted the names with '_'

Saying "always" in this context is a bit misleading since none of the interfaces API is released/stable API. CamelCase plus underscore is pretty gross, we should consider reducing this usage in things which can potentially propagate to bindings and make those bindings gross.

Sorry for throwing confusion here. In .Eo we will have Efl.Ui.Caching_Factory, in C we will have efl_ui_caching_factory, in C# Efl.Ui.CachingFactory. Every language can adapt to there preferred case.

@cedric Perfect! :)
@zmike ... even if its gross, we are consistent with that right now :)

zmike added a comment.Jan 2 2019, 10:57 AM

Alrighty, as long as we're moving forward!

Finally we arrived somewhere: you say all objects are equal, therefore this whole discussion is pointless : )

To avoid repeating this discussion in the future, I would remove all mention of MRU in the code comments, even in the variable name, because it is misleading.
Maybe call the variable cache and turn the comment into:

/// Simple list of ready-to-use objects. They are all equal so it does not matter from which
/// end of the list objects are added and removed.
cedric added a comment.Jan 4 2019, 6:14 PM

Finally we arrived somewhere: you say all objects are equal, therefore this whole discussion is pointless : )

Well, I wouldn't have realized they were all equal without that pointless discussion :-)

To avoid repeating this discussion in the future, I would remove all mention of MRU in the code comments, even in the variable name, because it is misleading.
Maybe call the variable cache and turn the comment into:

/// Simple list of ready-to-use objects. They are all equal so it does not matter from which
/// end of the list objects are added and removed.

Ok, will do that. I am having some trouble uploading the patch today (kind of the case at the end of the day, so might not push it for a few days and if there is no other comment by Wednesday will push it upstream).

Actually doing MRU is better than LRU, because it is more likely to still be in cache (L1/L2 or L3). But this should not be guaranteed by the API and should be a implementation detail.

felipealmeida accepted this revision.Jan 5 2019, 3:18 PM
This revision is now accepted and ready to land.Jan 5 2019, 3:18 PM

My comments from D7443#132437 have not been addressed yet, so please do not land this yet :)

src/lib/efl/interfaces/efl_cached_item.eo
10 ↗(On Diff #18128)

Also: Are these bytes? it should be specified.

This revision was automatically updated to reflect the committed changes.