Re: [Engine-devel] Guid improvements

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 <mpastern(a)redhat.com <mailto:mpastern@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" <amureini(a)redhat.com <mailto:amureini@redhat.com>> >>>>>> To: "engine-devel" <engine-devel(a)ovirt.org <mailto:engine-devel@ovirt.org>>, "Barak Azulay" <bazulay(a)redhat.com <mailto:bazulay@redhat.com>> >>>>>> Cc: "Yair Zaslavsky" <yzaslavs(a)redhat.com <mailto:yzaslavs@redhat.com>>, "Michael Pasternak" <mpastern(a)redhat.com <mailto:mpastern@redhat.com>>, "Tal Nisan" >>>>>> <tnisan(a)redhat.com <mailto:tnisan@redhat.com>>, "Ayal Baron" <abaron(a)redhat.com <mailto:abaron@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... >>>>>> >>>>> _______________________________________________ >>>>> Engine-devel mailing list >>>>> Engine-devel(a)ovirt.org <mailto:Engine-devel@ovirt.org> >>>>> http://lists.ovirt.org/mailman/listinfo/engine-devel >>>> >>>> _______________________________________________ >>>> Engine-devel mailing list >>>> Engine-devel(a)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