Re: [Engine-devel] Guid improvements
- Original Message - From: Liran Zelkha liran.zel...@gmail.com To: Allon Mureinik amure...@redhat.com Cc: Yair Zaslavsky yzasl...@redhat.com, engine-devel engine-devel@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 amure...@redhat.com wrote: - Original Message - From: Liran Zelkha liran.zel...@gmail.com To: Yair Zaslavsky yzasl...@redhat.com Cc: Allon Mureinik amure...@redhat.com , engine-devel engine-devel@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:master+topic:guid-database-improvements,n,z [2] http://gerrit.ovirt.org/#/c/16421/ [3] http://gerrit.ovirt.org/#/c/16467/ 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 amure...@redhat.com To: engine-devel engine-devel@ovirt.org , Barak Azulay bazu...@redhat.com Cc: Yair Zaslavsky yzasl...@redhat.com , Michael Pasternak mpast...@redhat.com , Tal Nisan tni...@redhat.com , Ayal Baron 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 http://lists.ovirt.org/mailman/listinfo/engine-devel ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
Re: [Engine-devel] Guid improvements
Awesome!!! On Mon, Jul 1, 2013 at 10:29 AM, Allon Mureinik amure...@redhat.com wrote: - Original Message - From: Liran Zelkha liran.zel...@gmail.com To: Yair Zaslavsky yzasl...@redhat.com Cc: Allon Mureinik amure...@redhat.com, engine-devel engine-devel@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 amure...@redhat.com To: engine-devel engine-devel@ovirt.org, Barak Azulay bazu...@redhat.com Cc: Yair Zaslavsky yzasl...@redhat.com, Michael Pasternak mpast...@redhat.com, Tal Nisan tni...@redhat.com, Ayal Baron 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 http://lists.ovirt.org/mailman/listinfo/engine-devel ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
[Engine-devel] 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 http://lists.ovirt.org/mailman/listinfo/engine-devel
Re: [Engine-devel] Guid improvements
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 To: engine-devel engine-devel@ovirt.org, Barak Azulay bazu...@redhat.com Cc: Yair Zaslavsky yzasl...@redhat.com, Michael Pasternak mpast...@redhat.com, Tal Nisan tni...@redhat.com, Ayal Baron 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 http://lists.ovirt.org/mailman/listinfo/engine-devel
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. 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 amure...@redhat.com To: engine-devel engine-devel@ovirt.org, Barak Azulay bazu...@redhat.com Cc: Yair Zaslavsky yzasl...@redhat.com, Michael Pasternak mpast...@redhat.com, Tal Nisan tni...@redhat.com, Ayal Baron 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 http://lists.ovirt.org/mailman/listinfo/engine-devel ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
Re: [Engine-devel] Guid improvements
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 amure...@redhat.com To: engine-devel engine-devel@ovirt.org, Barak Azulay bazu...@redhat.com Cc: Yair Zaslavsky yzasl...@redhat.com, Michael Pasternak mpast...@redhat.com, Tal Nisan tni...@redhat.com, Ayal Baron 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 http://lists.ovirt.org/mailman/listinfo/engine-devel ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel -- Michael Pasternak RedHat, ENG-Virtualization RD ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
Re: [Engine-devel] Guid improvements
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 To: engine-devel engine-devel@ovirt.org, Barak Azulay bazu...@redhat.com Cc: Yair Zaslavsky yzasl...@redhat.com, Michael Pasternak mpast...@redhat.com, Tal Nisan tni...@redhat.com, Ayal Baron 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 http://lists.ovirt.org/mailman/listinfo/engine-devel ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel -- Michael Pasternak RedHat, ENG-Virtualization RD -- Michael Pasternak RedHat, ENG-Virtualization RD ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
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 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]
Re: [Engine-devel] Guid improvements
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 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
Re: [Engine-devel] Guid improvements
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 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]
Re: [Engine-devel] Guid improvements
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
Re: [Engine-devel] Guid improvements
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
Re: [Engine-devel] Guid improvements
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 amure...@redhat.com To: engine-devel engine-devel@ovirt.org, Barak Azulay bazu...@redhat.com Cc: Yair Zaslavsky yzasl...@redhat.com, Michael Pasternak mpast...@redhat.com, Tal Nisan tni...@redhat.com, Ayal Baron 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 http://lists.ovirt.org/mailman/listinfo/engine-devel
