Sure. I agree. I'd be happy to show you the results in the profiler, so we can make a correct decision.
On Jun 30, 2013, at 1:23 PM, Michael Pasternak wrote: > 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
