On Thu, Jan 10, 2013 at 2:38 AM, Gustavo Sverzut Barbieri <[email protected]> wrote: > On Tue, Jan 8, 2013 at 11:11 PM, Cedric BAIL <[email protected]> wrote: >> 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. > > Ops! We can fix that by doing an union with a ptr, enforces alignment.
Hum, dah ! Stupid me, I did that to avoid doing fragmentation and loosing some memory, but in fact, the mempool will align it anyway. So not going to win anything with this in fact. Will change it then. >> - 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. So we can't use Eina_Magic, Eina_Cow is going to be used in the critical path, it need to be fast and Eina_Magic will likely give us a 5% slow down. I will check, but if it does, then I will remove it. >> 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 :-/ I don't want to have a eina_cow_memory_read, it will make our source code massively unreadable and increase our source code size a lot. I really want the read access to be cost less and direct. > 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. I started by implementing a rollback as I though it would be cool. But it create more problem I think. Now your read state isn't matching your write pointer. That could lead to interesting bug that are hard to understand. It got removed, it also make the code more efficient and less complex obviously. > memset() doesn't copy memory contents, rather replaces reference (similar > to stringshare replace). memcpy does a memcpy from a reader point of view. It is a direct replacement for a memcpy. Ok, it does an optimisation because, it's a copy on write, but that's an optimisation. >> 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 *). Yes, but it is still highly likely that you will be doing read access on the const void *. By marking it as WARN_UNUSED, you just make sure that someone use the void * pointer, not that nobody use the const void * pointer. >> 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? Seriously, it replace existing memcpy where we do use memcpy. It does a memcpy from the user point of view of the API. Of course it is a COW so it doesn't do an actual copy, but it does look like one from the user perspective of that pointer. >> > 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? Call it multiple time, doesn't increase much the code, as you will be in an idler or a thread and will do that with timing constraint anyway. >> 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); Could be done and detected, will do that for developer/debug profile > - multiple writes to the same alloc memory at the same time (not just > write -> commit, write -> commit. more like write, write -> commit, commit) Could be done also. > - get write from previous write memory -> possible? what happens when the > parent write commits? I don't get your idea here or what you are trying to say. > 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... Indeed that's not the main goal of this API. The main goal is to reduce our memory use. We do have a lot of object that are stable and don't change much. By reusing the same pointer, we use less data. It should also improve our cache usage and the memory bandwidth we use (That's where we do have issue right now). If we are lucky and reduce our call to real memcpy, then that will be nice, but unexpected. I expect a small increase in expedite test as it is fully animated and only one object doesn't change, but most elementary and edje based apps should benefit from it. I hope to not impact expedite performance and maybe speed up some of elementary test (sadly we don't have a benchmark there). -- Cedric BAIL ------------------------------------------------------------------------------ Master Visual Studio, SharePoint, SQL, ASP.NET, C# 2012, HTML5, CSS, MVC, Windows 8 Apps, JavaScript and much more. Keep your skills current with LearnDevNow - 3,200 step-by-step video tutorials by Microsoft MVPs and experts. ON SALE this month only -- learn more at: http://p.sf.net/sfu/learnmore_122712 _______________________________________________ enlightenment-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
