On 06/30/2013 01:15 PM, Michael Pasternak wrote: > On 06/30/2013 01:08 PM, Liran Zelkha wrote: >> Why synchronization? No need for it. Worst case scenario a put (which should >> be much less common then get) will occur twice on the same key. > > why assuming a best & not worst scenario? don't forget that every new > insertion requires collision resolution > which is triggers .equals() on the GUID.
Liran, don't get me wrong, i'm not against the caching in general, obviously reads > writes so actually i'm all with you, just we're going to significantly enlarge a memory footprint so i just want to make sure we're on a right track for the worst scenario where engine runs for ages and hashmap reaches it's load factor. > >> >> On Jun 30, 2013, at 1:00 PM, Michael Pasternak wrote: >> >>> On 06/30/2013 12:45 PM, Liran Zelkha wrote: >>>> All is true. But average UUID.fromString execution is 1675us, and >>>> HashMap.put is 78us - so the benefit is clear when we're talking on >100K >>>> executions for 10minutes... >>> >>> even with synchronization? what about ConcurrentHashMap? >>> >>>> >>>> >>>> On Sun, Jun 30, 2013 at 12:44 PM, Michael Pasternak <mpast...@redhat.com >>>> <mailto:mpast...@redhat.com>> wrote: >>>> >>>> On 06/30/2013 12:20 PM, Liran Zelkha wrote: >>>>> I checked such a solution using JProfiler. Creating the GUID object takes >>>>> much more CPU cycles that checking the string in the Map. >>>> >>>> of course it is, but what is the size of map that you checked?, check >>>> on worst scenario, i.e map >>>> is full of all possible guids, >>>> >>>> also problem a bit different,java map has a load factor (which is >>>> usually 0.75), >>>> when ratio increases beyond the load factor, occurs proses called >>>> re-hash so that the hash >>>> table will double amount of buckets. what can produce a cpu spikes >>>> (though it should not happen too often), >>>> to avoid this the initial capacity should be greater than the maximum >>>> number of entries / the >>>> load factor, and this is a huge map.... >>>> >>>> so basically this is a tradeoff between time and space costs against >>>> the new guid generation. >>>> >>>>> >>>>> On Jun 30, 2013, at 12:06 PM, Michael Pasternak wrote: >>>>> >>>>>> On 06/30/2013 11:37 AM, Liran Zelkha wrote: >>>>>>> Great news. >>>>>>> Allon - please note that GUID is being recreated from String by both DB >>>>>>> calls and by data received from VDSM. It is VERY useful to cache Guid >>>>>>> String-->Guid, and not >>>> just Empty GUID. >>>>>>> >>>>>>> However, as the Guid class runs in GWT as well, you can't use >>>>>>> Infinispan and you're limited in the HashMap implementations you can >>>>>>> use. >>>>>>> Personally, I don't think it's a memory leak, as GUID number in the >>>>>>> system are finite and not too large. >>>>>> >>>>>> it's large, it's 128-bit random number, it's not about memory leaking, >>>>>> but cpu cost, >>>>>> as you'll face a lot of rehash'ings in the map, >>>>>> >>>>>> i'm not even sure that using indexing in the map can help, worth >>>>>> checking. >>>>>> >>>>>> What do you think? Want to add it to your patch? >>>>>> >>>>>> >>>>>>> >>>>>>> On Jun 30, 2013, at 11:13 AM, Yair Zaslavsky wrote: >>>>>>> >>>>>>>> Well done, should have been done ages ago :) >>>>>>>> Now, for the painful rebase of async_task_mgr changes :) >>>>>>>> >>>>>>>> >>>>>>>> ----- Original Message ----- >>>>>>>>> From: "Allon Mureinik" <amure...@redhat.com >>>>>>>>> <mailto:amure...@redhat.com>> >>>>>>>>> To: "engine-devel" <engine-devel@ovirt.org >>>>>>>>> <mailto:engine-devel@ovirt.org>>, "Barak Azulay" <bazu...@redhat.com >>>>>>>>> <mailto:bazu...@redhat.com>> >>>>>>>>> Cc: "Yair Zaslavsky" <yzasl...@redhat.com >>>>>>>>> <mailto:yzasl...@redhat.com>>, "Michael Pasternak" >>>>>>>>> <mpast...@redhat.com <mailto:mpast...@redhat.com>>, "Tal Nisan" >>>>>>>>> <tni...@redhat.com <mailto:tni...@redhat.com>>, "Ayal Baron" >>>>>>>>> <aba...@redhat.com <mailto:aba...@redhat.com>> >>>>>>>>> Sent: Sunday, June 30, 2013 11:11:30 AM >>>>>>>>> Subject: Guid improvements >>>>>>>>> >>>>>>>>> Hi all, >>>>>>>>> >>>>>>>>> I just merged a couple of improvements to the [N]Guid class [1] to >>>>>>>>> improve >>>>>>>>> it's performance both CPU-wise and memory-wise, based on a set of >>>>>>>>> benchmarks >>>>>>>>> presented by Liran. >>>>>>>>> >>>>>>>>> What this patchset achieves: >>>>>>>>> 1. Clean up the code, so it's easier to understand and use >>>>>>>>> 2. Eliminate the inflation in the memory foot print caused by the >>>>>>>>> getValue() >>>>>>>>> method >>>>>>>>> 3. Eliminate all the heavy calls to UUID.fromString when creating a >>>>>>>>> new/empty >>>>>>>>> Guid instance as a default value >>>>>>>>> 4. Note that the cleanups proposed in (1) will have minor performance >>>>>>>>> benefits (e.g., eliminating useless conditional statements), but I >>>>>>>>> doubt >>>>>>>>> this would be anything to write home about. >>>>>>>>> >>>>>>>>> From a developer's perspective, here's what changed: >>>>>>>>> 1. No more NGuid, just Guid. Both static methods to create a Guid >>>>>>>>> from String >>>>>>>>> still exist, and are named createGuidFromString and >>>>>>>>> createGuidFromStringDefaultEmpty. >>>>>>>>> 2. [N]Guid.getValue() was removed, it's no longer needed after (1) was >>>>>>>>> implemented >>>>>>>>> 3. The Guid() constructor was made private, as it forced a redundant >>>>>>>>> call to >>>>>>>>> UUID.fromString(String). If you need an empty Guid instance, just use >>>>>>>>> Guid.Empty >>>>>>>>> 4. The Guid.EMPTY_GUID_VALUE string constant was removed, as it was >>>>>>>>> used for >>>>>>>>> redundant calls to UUID.fromString. If you really, REALLY, need it, >>>>>>>>> just >>>>>>>>> call Guid.Empty.getValue() for a UUID or Guid.Empty.toString() for a >>>>>>>>> String. >>>>>>>>> 5. All sorts of ways to transform Strings to Guids were removed. If >>>>>>>>> you have >>>>>>>>> a literal you trust, just use new Guid(String). If you suspect it may >>>>>>>>> be >>>>>>>>> null, use Guid.createGuidFromString[DefaultEmpty] >>>>>>>>> 6. NewGuid is now called newGuid. We're in Java, not C# :-) >>>>>>>>> >>>>>>>>> >>>>>>>>> Many thanks to everyone who reviewed this patchset. >>>>>>>>> You guys rock! >>>>>>>>> >>>>>>>>> >>>>>>>>> Regards, >>>>>>>>> Allon >>>>>>>>> >>>>>>>>> >>>>>>>>> [1] >>>>>>>>> http://gerrit.ovirt.org/#/q/project:ovirt-engine+branch:master+topic:guid-cleanup,n,z >>>>>>>>> >>>>>>>> _______________________________________________ >>>>>>>> Engine-devel mailing list >>>>>>>> Engine-devel@ovirt.org <mailto:Engine-devel@ovirt.org> >>>>>>>> http://lists.ovirt.org/mailman/listinfo/engine-devel >>>>>>> >>>>>>> _______________________________________________ >>>>>>> Engine-devel mailing list >>>>>>> Engine-devel@ovirt.org <mailto:Engine-devel@ovirt.org> >>>>>>> http://lists.ovirt.org/mailman/listinfo/engine-devel >>>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> >>>>>> Michael Pasternak >>>>>> RedHat, ENG-Virtualization R&D >>>>> >>>> >>>> >>>> -- >>>> >>>> Michael Pasternak >>>> RedHat, ENG-Virtualization R&D >>>> >>>> >>> >>> >>> -- >>> >>> Michael Pasternak >>> RedHat, ENG-Virtualization R&D >> > > -- Michael Pasternak RedHat, ENG-Virtualization R&D _______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
