On Tue, Jan 8, 2013 at 11:11 PM, Cedric BAIL <cedric.b...@free.fr> wrote:
> Yop, > > On Wed, Jan 9, 2013 at 7:13 AM, Gustavo Sverzut Barbieri > <barbi...@profusion.mobi> 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. > Ops! We can fix that by doing an union with a ptr, enforces alignment. > - 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 ? > Of course not! Are you crazy? :-) Disabling magic check is something to do in very rare cases where you can test all your code before deployment, making sure there are no mistakes. Not for general use, as it depends on the whole package (libs + apps) to be tested. > 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. > It's more an instruction that the ptr you give come from that. If I read: bla(void *x) or bla(const void *x) we understand that any data is possible, I can even call with "abc"... but no, in your case the memory must have come from Eina_Cow! of course compilers won't be able to warn about incorrect usage, that is a pita :-/ Maybe with a more clear API design, we can move things around, such as define Eina_Cow_Memory to be an opaque pointer and you do eina_cow_memory_read() and write(), these return const void*/void*. But I'm still not sure, the current API is confusing :-/ write() doesn't write... returns a write-able memory. commit() doesn't commit, just gives back the write-able memory saying it will not be writeable anymore... people will wonder about a rollback() or similar as well. memset() doesn't copy memory contents, rather replaces reference (similar to stringshare replace). > 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. > I wondered about that reason, but IMO it's not valid. Just make the return WARN_UNUSED, and the type should also be enough (you return void*, from a given const void *). > 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. > >From the names I guessed that, but as I said in my original email, the basic API is still confusing and not good for users. Let's work on that and keep these great ideas for later. It's bad to have something partial now and a sucker API :-/ > 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. > And now you see why the name is completely off? I'm not even sure the idea should be that or just _dup() that returns a new COW memory. And some _replace() that does that + free the previous value? > > 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. > right one to try... but the only 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. > The GC stuff seems too much WIP, but that's the lesser part. The API is much more confusing than one would expect. Really, I read it many times and it makes little sense given on names and usage. It should be simpler and usage reviewed. > 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. > I did, that's the problem :-( Otherwise I'd just say "nice". > > > 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. > I see, the test is close to what is done by evas/edje, but the tests are pretty insufficient, and it's all merged together. First of all would be better to use one test per scenario you want, so when it fails it will give you the name. Second use the proper check tests for integer, then it will print values that differs and one can understand why it did fail. Third and more important we need more tests to stress the non-expected situations: - more commits than writes (should fail gracefully, with an error msg rather than crash); - multiple writes to the same alloc memory at the same time (not just write -> commit, write -> commit. more like write, write -> commit, commit) - get write from previous write memory -> possible? what happens when the parent write commits? It would also be nice to see how you plan to take advantage. It's easy to see you want to change the existing mempy() of prev = cur, keeping cur intact until it writes. But reality is that we already just do that if o->changed, that is we'll know something changed... in very rare circumstances we changed to the original value and it will be a noop (maybe we even check for that). It would also imply that we do very fine grained write->commit pair, that will add a cost... memcpy() is quite fast to be beaten by such small memory amount. Then I don't see if we save much... -- Gustavo Sverzut Barbieri http://profusion.mobi embedded systems -------------------------------------- MSN: barbi...@gmail.com Skype: gsbarbieri Mobile: +55 (19) 9225-2202 ------------------------------------------------------------------------------ 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 enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
