[v2,1/7] test/hash: fix bucket size in hash perf test

Message ID 1537550255-252066-2-git-send-email-yipeng1.wang@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series hash: add extendable bucket and partial key hashing |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Wang, Yipeng1 Sept. 21, 2018, 5:17 p.m. UTC
  The bucket size was changed from 4 to 8 but the corresponding
perf test was not changed accordingly.

Fixes: 58017c98ed53 ("hash: add vectorized comparison")
Cc: stable@dpdk.org

Signed-off-by: Yipeng Wang <yipeng1.wang@intel.com>
---
 test/test/test_hash_perf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

Comments

Bruce Richardson Sept. 26, 2018, 10:04 a.m. UTC | #1
On Fri, Sep 21, 2018 at 10:17:29AM -0700, Yipeng Wang wrote:
> The bucket size was changed from 4 to 8 but the corresponding
> perf test was not changed accordingly.
> 

Can you perhaps give a little detail on what actual problems this caused.
Did it just mean that we used up too much memory in the test because we
thought there were more buckets than there were, or something else?

> Fixes: 58017c98ed53 ("hash: add vectorized comparison")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Yipeng Wang <yipeng1.wang@intel.com>
> ---
>  test/test/test_hash_perf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/test/test/test_hash_perf.c b/test/test/test_hash_perf.c
> index 33dcb9f..9ed7125 100644
> --- a/test/test/test_hash_perf.c
> +++ b/test/test/test_hash_perf.c
> @@ -20,7 +20,7 @@
>  #define MAX_ENTRIES (1 << 19)
>  #define KEYS_TO_ADD (MAX_ENTRIES * 3 / 4) /* 75% table utilization */
>  #define NUM_LOOKUPS (KEYS_TO_ADD * 5) /* Loop among keys added, several times */
> -#define BUCKET_SIZE 4
> +#define BUCKET_SIZE 8
>  #define NUM_BUCKETS (MAX_ENTRIES / BUCKET_SIZE)
>  #define MAX_KEYSIZE 64
>  #define NUM_KEYSIZES 10
> -- 
> 2.7.4
>
  
Wang, Yipeng1 Sept. 27, 2018, 3:39 a.m. UTC | #2
Hi Bruce,

In the test, the bucket size and number of buckets are used
to map to the underneath rte_hash structure. They are used
to test performance of two scenarios: keys in primary
buckets only and keys in both primary and secondary buckets.

Although there is no functional issue with bucket size set
to 4, it mismatches the underneath rte_hash structure (i.e. 8),
which may affect code readability and future extension.

I added this description into the commit message.

Thanks
Yipeng

>-----Original Message-----
>
>Can you perhaps give a little detail on what actual problems this caused.
>Did it just mean that we used up too much memory in the test because we
>thought there were more buckets than there were, or something else?
>
  
Honnappa Nagarahalli Sept. 27, 2018, 4:23 a.m. UTC | #3
> -----Original Message-----
> From: Yipeng Wang <yipeng1.wang@intel.com>
> Sent: Friday, September 21, 2018 12:17 PM
> To: bruce.richardson@intel.com
> Cc: dev@dpdk.org; yipeng1.wang@intel.com; michel@digirati.com.br;
> Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> Subject: [PATCH v2 1/7] test/hash: fix bucket size in hash perf test
> 
> The bucket size was changed from 4 to 8 but the corresponding perf test was
> not changed accordingly.
> 
> Fixes: 58017c98ed53 ("hash: add vectorized comparison")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Yipeng Wang <yipeng1.wang@intel.com>
> ---
>  test/test/test_hash_perf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/test/test/test_hash_perf.c b/test/test/test_hash_perf.c index
> 33dcb9f..9ed7125 100644
> --- a/test/test/test_hash_perf.c
> +++ b/test/test/test_hash_perf.c
> @@ -20,7 +20,7 @@
>  #define MAX_ENTRIES (1 << 19)
>  #define KEYS_TO_ADD (MAX_ENTRIES * 3 / 4) /* 75% table utilization */
> #define NUM_LOOKUPS (KEYS_TO_ADD * 5) /* Loop among keys added,
> several times */ -#define BUCKET_SIZE 4
> +#define BUCKET_SIZE 8
May be we should add a comment to warn that it should be same as ' RTE_HASH_BUCKET_ENTRIES'?

>  #define NUM_BUCKETS (MAX_ENTRIES / BUCKET_SIZE)  #define
> MAX_KEYSIZE 64  #define NUM_KEYSIZES 10
> --
> 2.7.4
  
Wang, Yipeng1 Sept. 29, 2018, 12:31 a.m. UTC | #4
>-----Original Message-----
>From: Honnappa Nagarahalli [mailto:Honnappa.Nagarahalli@arm.com]
>> several times */ -#define BUCKET_SIZE 4
>> +#define BUCKET_SIZE 8
>May be we should add a comment to warn that it should be same as ' RTE_HASH_BUCKET_ENTRIES'?
>
[Wang, Yipeng] Done in V4, Thanks for the comment!
  

Patch

diff --git a/test/test/test_hash_perf.c b/test/test/test_hash_perf.c
index 33dcb9f..9ed7125 100644
--- a/test/test/test_hash_perf.c
+++ b/test/test/test_hash_perf.c
@@ -20,7 +20,7 @@ 
 #define MAX_ENTRIES (1 << 19)
 #define KEYS_TO_ADD (MAX_ENTRIES * 3 / 4) /* 75% table utilization */
 #define NUM_LOOKUPS (KEYS_TO_ADD * 5) /* Loop among keys added, several times */
-#define BUCKET_SIZE 4
+#define BUCKET_SIZE 8
 #define NUM_BUCKETS (MAX_ENTRIES / BUCKET_SIZE)
 #define MAX_KEYSIZE 64
 #define NUM_KEYSIZES 10