4:11 a.m.
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...
4:13 a.m.
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>
To: "engine-devel" <engine-devel(a)ovirt.org>, "Barak Azulay"
<bazulay(a)redhat.com>
Cc: "Yair Zaslavsky" <yzaslavs(a)redhat.com>, "Michael Pasternak"
<mpastern(a)redhat.com>, "Tal Nisan"
<tnisan(a)redhat.com>, "Ayal Baron" <abaron(a)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...
4:37 a.m.
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.
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>
> To: "engine-devel" <engine-devel(a)ovirt.org>, "Barak Azulay"
<bazulay(a)redhat.com>
> Cc: "Yair Zaslavsky" <yzaslavs(a)redhat.com>, "Michael
Pasternak" <mpastern(a)redhat.com>, "Tal Nisan"
> <tnisan(a)redhat.com>, "Ayal Baron" <abaron(a)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
http://lists.ovirt.org/mailman/listinfo/engine-devel
5:06 a.m.
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>
>> To: "engine-devel" <engine-devel(a)ovirt.org>, "Barak
Azulay" <bazulay(a)redhat.com>
>> Cc: "Yair Zaslavsky" <yzaslavs(a)redhat.com>, "Michael
Pasternak" <mpastern(a)redhat.com>, "Tal Nisan"
>> <tnisan(a)redhat.com>, "Ayal Baron" <abaron(a)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
> http://lists.ovirt.org/mailman/listinfo/engine-devel
_______________________________________________
Engine-devel mailing list
Engine-devel(a)ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-devel
--
Michael Pasternak
RedHat, ENG-Virtualization R&D
5:20 a.m.
I checked such a solution using JProfiler. Creating the GUID object takes much more CPU
cycles that checking the string in the Map.
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>
>>> To: "engine-devel" <engine-devel(a)ovirt.org>, "Barak
Azulay" <bazulay(a)redhat.com>
>>> Cc: "Yair Zaslavsky" <yzaslavs(a)redhat.com>, "Michael
Pasternak" <mpastern(a)redhat.com>, "Tal Nisan"
>>> <tnisan(a)redhat.com>, "Ayal Baron" <abaron(a)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
>> http://lists.ovirt.org/mailman/listinfo/engine-devel
>
> _______________________________________________
> Engine-devel mailing list
> Engine-devel(a)ovirt.org
> http://lists.ovirt.org/mailman/listinfo/engine-devel
>
--
Michael Pasternak
RedHat, ENG-Virtualization R&D
5:44 a.m.
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>
>>>> To: "engine-devel" <engine-devel(a)ovirt.org>, "Barak
Azulay" <bazulay(a)redhat.com>
>>>> Cc: "Yair Zaslavsky" <yzaslavs(a)redhat.com>, "Michael
Pasternak" <mpastern(a)redhat.com>, "Tal Nisan"
>>>> <tnisan(a)redhat.com>, "Ayal Baron"
<abaron(a)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
>>> http://lists.ovirt.org/mailman/listinfo/engine-devel
>>
>> _______________________________________________
>> Engine-devel mailing list
>> Engine-devel(a)ovirt.org
>> http://lists.ovirt.org/mailman/listinfo/engine-devel
>>
>
>
> --
>
> Michael Pasternak
> RedHat, ENG-Virtualization R&D
--
Michael Pasternak
RedHat, ENG-Virtualization R&D
5:45 a.m.
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...
On Sun, Jun 30, 2013 at 12:44 PM, Michael Pasternak <mpastern(a)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>
>>>>> To: "engine-devel" <engine-devel(a)ovirt.org>,
"Barak Azulay" <
bazulay(a)redhat.com>
>>>>> Cc: "Yair Zaslavsky" <yzaslavs(a)redhat.com>,
"Michael Pasternak" <
mpastern(a)redhat.com>, "Tal Nisan"
>>>>> <tnisan(a)redhat.com>, "Ayal Baron"
<abaron(a)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
>>>> http://lists.ovirt.org/mailman/listinfo/engine-devel
>>>
>>> _______________________________________________
>>> Engine-devel mailing list
>>> Engine-devel(a)ovirt.org
>>> http://lists.ovirt.org/mailman/listinfo/engine-devel
>>>
>>
>>
>> --
>>
>> Michael Pasternak
>> RedHat, ENG-Virtualization R&D
>
--
Michael Pasternak
RedHat, ENG-Virtualization R&D
6 a.m.
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
6:08 a.m.
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.
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(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
6:15 a.m.
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.
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(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
--
Michael Pasternak
RedHat, ENG-Virtualization R&D
6:23 a.m.
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(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
>
--
Michael Pasternak
RedHat, ENG-Virtualization R&D
6:23 a.m.
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(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
>>
>
>
--
Michael Pasternak
RedHat, ENG-Virtualization R&D
3:29 a.m.
----- Original Message -----
From: "Liran Zelkha" <liran.zelkha(a)gmail.com>
To: "Yair Zaslavsky" <yzaslavs(a)redhat.com>
Cc: "Allon Mureinik" <amureini(a)redhat.com>, "engine-devel"
<engine-devel(a)ovirt.org>
Sent: Sunday, June 30, 2013 11:37:26 AM
Subject: Re: [Engine-devel] Guid improvements
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.
I generally agree about the high cost of
sting<->uuid operations, but I'm not sure caching is the way to go, at least not
everywhere.
At least for the database, there is a much easier solution - stop using strings to
represent uuids.
Here's an example of how it can be done:
http://gerrit.ovirt.org/#/c/16281/
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. 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>
>> To: "engine-devel" <engine-devel(a)ovirt.org>, "Barak
Azulay"
>> <bazulay(a)redhat.com>
>> Cc: "Yair Zaslavsky" <yzaslavs(a)redhat.com>, "Michael
Pasternak"
>> <mpastern(a)redhat.com>, "Tal Nisan"
>> <tnisan(a)redhat.com>, "Ayal Baron" <abaron(a)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
> http://lists.ovirt.org/mailman/listinfo/engine-devel
3:36 a.m.
Awesome!!!
On Mon, Jul 1, 2013 at 10:29 AM, Allon Mureinik <amureini(a)redhat.com> wrote:
----- Original Message -----
> From: "Liran Zelkha" <liran.zelkha(a)gmail.com>
> To: "Yair Zaslavsky" <yzaslavs(a)redhat.com>
> Cc: "Allon Mureinik" <amureini(a)redhat.com>, "engine-devel"
<
engine-devel(a)ovirt.org>
> Sent: Sunday, June 30, 2013 11:37:26 AM
> Subject: Re: [Engine-devel] Guid improvements
>
> 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.
I generally agree about the high cost of sting<->uuid operations, but I'm
not sure caching is the way to go, at least not everywhere.
At least for the database, there is a much easier solution - stop using
strings to represent uuids.
Here's an example of how it can be done:
http://gerrit.ovirt.org/#/c/16281/
>
> 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. 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>
> >> To: "engine-devel" <engine-devel(a)ovirt.org>, "Barak
Azulay"
> >> <bazulay(a)redhat.com>
> >> Cc: "Yair Zaslavsky" <yzaslavs(a)redhat.com>, "Michael
Pasternak"
> >> <mpastern(a)redhat.com>, "Tal Nisan"
> >> <tnisan(a)redhat.com>, "Ayal Baron"
<abaron(a)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
> > http://lists.ovirt.org/mailman/listinfo/engine-devel
>
>
11:43 a.m.
----- Original Message -----
> > >
> > > 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. 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 >
> > > >> To: "engine-devel" <
engine-devel(a)ovirt.org >, "Barak Azulay"
> > > >> < bazulay(a)redhat.com >
> > > >> Cc: "Yair Zaslavsky" <
yzaslavs(a)redhat.com >, "Michael Pasternak"
> > > >> < mpastern(a)redhat.com >,
"Tal Nisan"
> > > >> < tnisan(a)redhat.com >,
"Ayal Baron" < abaron(a)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
> > > >
http://lists.ovirt.org/mailman/listinfo/engine-devel
> > >
> > >
From: "Liran Zelkha" <liran.zelkha(a)gmail.com>
To: "Allon Mureinik" <amureini(a)redhat.com>
Cc: "Yair Zaslavsky" <yzaslavs(a)redhat.com>, "engine-devel"
<engine-devel(a)ovirt.org>
Sent: Monday, July 1, 2013 10:36:06 AM
Subject: Re: [Engine-devel] Guid improvements
Awesome!!!
On Mon, Jul 1, 2013 at 10:29 AM, Allon Mureinik <
amureini(a)redhat.com >
wrote:
> ----- Original Message -----
> > From: "Liran Zelkha" < liran.zelkha(a)gmail.com >
> > To: "Yair Zaslavsky" < yzaslavs(a)redhat.com >
> > Cc: "Allon Mureinik" < amureini(a)redhat.com >,
"engine-devel" <
> > engine-devel(a)ovirt.org >
> > Sent: Sunday, June 30, 2013 11:37:26 AM
> > Subject: Re: [Engine-devel] Guid improvements
> >
> > 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.
> I generally agree about the high cost of sting<->uuid operations, but I'm
> not
> sure caching is the way to go, at least not everywhere.
> At least for the database, there is a much easier solution -
stop using
> strings to represent uuids.
> Here's an example of how it can be done:
> http://gerrit.ovirt.org/#/c/16281/
This patchset was merged [1].
Along with a couple of uber-neat improvements by Juan [2][3], I think we should see a real
boost in performance.
[1]
http://gerrit.ovirt.org/#/q/status:merged+project:ovirt-engine+branch:mas...
[2] http://gerrit.ovirt.org/#/c/16421/
[3] http://gerrit.ovirt.org/#/c/16467/
6:53 a.m.
On 06/30/2013 11:13 AM, Yair Zaslavsky wrote:
Well done, should have been done ages ago :)
Now, for the painful rebase of async_task_mgr changes :)
And for the total removal
of Guid wrapper usage of java.util.UUID
directly instead ;)
----- Original Message -----
> From: "Allon Mureinik" <amureini(a)redhat.com>
> To: "engine-devel" <engine-devel(a)ovirt.org>, "Barak Azulay"
<bazulay(a)redhat.com>
> Cc: "Yair Zaslavsky" <yzaslavs(a)redhat.com>, "Michael
Pasternak" <mpastern(a)redhat.com>, "Tal Nisan"
> <tnisan(a)redhat.com>, "Ayal Baron" <abaron(a)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...
>
4189
days inactive
4196
days old
15 comments
5 participants
participants (5)
-
Allon Mureinik -
Liran Zelkha -
Michael Pasternak -
Tal Nisan -
Yair Zaslavsky