Yop, On Wed, Jan 9, 2013 at 7:13 AM, Gustavo Sverzut Barbieri <[email protected]> wrote: > Quick review: > > - Eina_Cow_Ptr should be before given memory to user. > - It would enforce alignment (as user memory could be not aligned). > Eina_Cow_Ptr itself is always aligned because it contains pointers, so it > will not impact what follows.
Eina_Cow_Ptr doesn't have a pointer in it. > - It would be less likely to suffer from out-of-bounds writes... that > are VERY common bug :-P > - Please use a macro to get Eina_Cow_Ptr from memory. Indeed. > - Eina_Cow_Ptr should have magic header... will consume more memory, but > it's a valuable way to avoid errors with users giving invalid pointers Hum, as Eina_Magic can be disable. Sounds good to. Does the release profile disable Eina_Magic, btw ? > - initial data should be marked as READ ONLY to valgrind or some other > means Indeed. > To avoid falling into the same problems as we did with stringshare, please > typedef const void Eina_Cow_Memory and use it. Avoiding confusion where > simple pointers are expected and where actual cow-memory is. Hum, I don't really see the point here. We will always put this const void * memory into a pointer to an actual structure and access it directly for all read only access. There will never be an instance of Eina_Cow_Memory anywhere in the code that use Eina_Cow. > eina_cow_write() shouldn't take pointer to data, why it does that? just use > the returned value. It does, because it may change the pointer to data. This is to reduce the chance of a mistake where you do access in the same function to the read only pointer and write some data to the returned pointer. I believe that to reduce the chance of a bug, that pointer need to be changed. > inside eina_cow_write(), if we memcpy(), it's always *data. Remove that > confusing src as it's useless :-) indeed. > eina_cow_commit() is pretty undefined what's the behavior based on the > name... the contents adds it to GC? so commit is "mark as unused"? What if > it's not ref->togc? leaks? Confusing! If it's already ref->togc, then it is already handed to the GC, so no need to add it again to the GC list. The purpose of this function is that maybe we can make the GC run from another thread. In that case you need to delay GC on that write pointer until we do "commit" it, but that would also means that it needs to be removed from the GC list when eina_cow_write is called. That's not done yet, as I want to first experiment without thread, but the API permit it later. > eina_cow_commit() exit early if (ref->togc), makes it simpler. indeed. > eina_cow_memcpy() free dst and make dst = src? really? unexpected from name. Copy on Write means that as long as you don't modify the pointer data, you can share the same pointer. That's the purpose of this cow. > eina_cow_gc() just handles the last element, logic is confusing :-( Yes, it handles the last one as this is the one that as likely been less changed and less chance to change in the near futur. So it should the right one to try. > While I understand the purpose of having eina_cow, I found the API to be > very confusing and misleading. It also seems to be incomplete, yet tries to > to more than the basics without getting the basics right. The basics are right for our use case. It does everything we need for evas and edje. Please tell us what is incomplete. > Could we get back and model that after some existing concepts, doing the > simple things first, then adding complex stuff as the hash/gc and reuse? > A good start is: add(), del(), alloc(), free() and write(). Get these right > then follow to extensions such as memcpy() and commit(). Why remove what already works, does the job and match our needs ? Please take the time to look at the actual provided code and understand it's purpose correctly. > It would be nice to have some tests to validate what you're trying to > provide and serve as examples as well. There is a test in eina test suite... Use case is Evas_Object_Protected_Data and Edje_Real_Part mostly (There is some other place in Evas and Elm that could benefit from it). Idea is that this object are mostly read only with very few write. Many of them do have the same value. So for not impacting speed the read only path should remain almost as it is and only the write path should change. The yet to be decided is if we do call the gc from a thread or from an idler in the main loop. -- Cedric BAIL ------------------------------------------------------------------------------ Master Java SE, Java EE, Eclipse, Spring, Hibernate, JavaScript, jQuery and much more. Keep your Java skills current with LearnJavaNow - 200+ hours of step-by-step video tutorials by Java experts. SALE $49.99 this month only -- learn more at: http://p.sf.net/sfu/learnmore_122612 _______________________________________________ enlightenment-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
