[Engine-devel] Guid improvements

Liran Zelkha liran.zelkha at gmail.com
Sun Jun 30 10:23:39 UTC 2013 

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

More information about the Engine-devel mailing list