Minor compression heuristic cleanups

PATCH In review David Sterba 2 participants 5 messages Dec 12, 2017 Btrfs file system

Share & tools

David Sterba

author maintainer

2017-12-12 20:57 UTC #0 lore raw

cover Minor compression heuristic cleanups

The callback pointers for radix_sort are not needed, we don't plan to export the function now. The compiler is smart enough to replace the indirect calls with direct ones, so there's no change in the resulting asm code.

David Sterba (3):
btrfs: heuristic: open code get_num callback of radix sort
btrfs: heuristic: open code copy_call callback of radix sort
btrfs: heuristic: call get4bits directly

 fs/btrfs/compression.c | 42 +++++++++++-------------------------------
 1 file changed, 11 insertions(+), 31 deletions(-)

Signature · 1 line

2.15.1

David Sterba

author maintainer

2017-12-12 20:57 UTC #1 lore raw

PATCH 1/3 btrfs: heuristic: open code get_num callback of radix sort

The callback is trivial and we don't need the abstraction for our purposes. Let's open code it and also make the array types explicit.

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/compression.c | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

fs/btrfs/compression.c +9 ~~-16~~

index 28c3940062b7..37a69d4b04ce 100644
@@ -1318,12 +1318,6 @@ static void copy_cell(void *dst, int dest_i, void *src, int src_i)
 	dstv[dest_i] = srcv[src_i];
 }
 
-static u64 get_num(const void *a, int i)
-{
-	struct bucket_item *av = (struct bucket_item *)a;
-	return av[i].count;
-}
-
 /*
  * Use 4 bits as radix base
  * Use 16 u32 counters for calculating new possition in buf array

@@ -1332,12 +1326,11 @@ static u64 get_num(const void *a, int i)
  * @array_buf - buffer array to store sorting results
  *              must be equal in size to @array
  * @num       - array size
- * @get_num   - function to extract number from array
  * @copy_cell - function to copy data from array to array_buf and vice versa
  * @get4bits  - function to get 4 bits from number at specified offset
  */
-static void radix_sort(void *array, void *array_buf, int num,
-		       u64 (*get_num)(const void *, int i),
+static void radix_sort(struct bucket_item *array, struct bucket_item *array_buf,
+		       int num,
 		       void (*copy_cell)(void *dest, int dest_i,
 					 void* src, int src_i),
 		       u8 (*get4bits)(u64 num, int shift))

@@ -1355,9 +1348,9 @@ static void radix_sort(void *array, void *array_buf, int num,
 	 * Try avoid useless loop iterations for small numbers stored in big
 	 * counters.  Example: 48 33 4 ... in 64bit array
 	 */
-	max_num = get_num(array, 0);
+	max_num = array[0].count;
 	for (i = 1; i < num; i++) {
-		buf_num = get_num(array, i);
+		buf_num = array[i].count;
 		if (buf_num > max_num)
 			max_num = buf_num;
 	}

@@ -1370,7 +1363,7 @@ static void radix_sort(void *array, void *array_buf, int num,
 		memset(counters, 0, sizeof(counters));
 
 		for (i = 0; i < num; i++) {
-			buf_num = get_num(array, i);
+			buf_num = array[i].count;
 			addr = get4bits(buf_num, shift);
 			counters[addr]++;
 		}

@@ -1379,7 +1372,7 @@ static void radix_sort(void *array, void *array_buf, int num,
 			counters[i] += counters[i - 1];
 
 		for (i = num - 1; i >= 0; i--) {
-			buf_num = get_num(array, i);
+			buf_num = array[i].count;
 			addr = get4bits(buf_num, shift);
 			counters[addr]--;
 			new_addr = counters[addr];

@@ -1397,7 +1390,7 @@ static void radix_sort(void *array, void *array_buf, int num,
 		memset(counters, 0, sizeof(counters));
 
 		for (i = 0; i < num; i ++) {
-			buf_num = get_num(array_buf, i);
+			buf_num = array_buf[i].count;
 			addr = get4bits(buf_num, shift);
 			counters[addr]++;
 		}

@@ -1406,7 +1399,7 @@ static void radix_sort(void *array, void *array_buf, int num,
 			counters[i] += counters[i - 1];
 
 		for (i = num - 1; i >= 0; i--) {
-			buf_num = get_num(array_buf, i);
+			buf_num = array_buf[i].count;
 			addr = get4bits(buf_num, shift);
 			counters[addr]--;
 			new_addr = counters[addr];

@@ -1444,7 +1437,7 @@ static int byte_core_set_size(struct heuristic_ws *ws)
 	struct bucket_item *bucket = ws->bucket;
 
 	/* Sort in reverse order */
-	radix_sort(ws->bucket, ws->bucket_b, BUCKET_SIZE, get_num, copy_cell,
+	radix_sort(ws->bucket, ws->bucket_b, BUCKET_SIZE, copy_cell,
 			get4bits);
 
 	for (i = 0; i < BYTE_CORE_SET_LOW; i++)

Signature · 1 line

2.15.1

David Sterba

author maintainer

2017-12-12 20:57 UTC #2 lore raw

PATCH 2/3 btrfs: heuristic: open code copy_call callback of radix sort

In reply to David Sterba

The callback is trivial and we don't need the abstraction for our purposes. Let's open code it.

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/compression.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

fs/btrfs/compression.c +3 ~~-14~~

index 37a69d4b04ce..935acabc0ea7 100644
@@ -1311,13 +1311,6 @@ static u8 get4bits(u64 num, int shift) {
 	return low4bits;
 }
 
-static void copy_cell(void *dst, int dest_i, void *src, int src_i)
-{
-	struct bucket_item *dstv = (struct bucket_item *)dst;
-	struct bucket_item *srcv = (struct bucket_item *)src;
-	dstv[dest_i] = srcv[src_i];
-}
-
 /*
  * Use 4 bits as radix base
  * Use 16 u32 counters for calculating new possition in buf array

@@ -1326,13 +1319,10 @@ static void copy_cell(void *dst, int dest_i, void *src, int src_i)
  * @array_buf - buffer array to store sorting results
  *              must be equal in size to @array
  * @num       - array size
- * @copy_cell - function to copy data from array to array_buf and vice versa
  * @get4bits  - function to get 4 bits from number at specified offset
  */
 static void radix_sort(struct bucket_item *array, struct bucket_item *array_buf,
 		       int num,
-		       void (*copy_cell)(void *dest, int dest_i,
-					 void* src, int src_i),
 		       u8 (*get4bits)(u64 num, int shift))
 {
 	u64 max_num;

@@ -1376,7 +1366,7 @@ static void radix_sort(struct bucket_item *array, struct bucket_item *array_buf,
 			addr = get4bits(buf_num, shift);
 			counters[addr]--;
 			new_addr = counters[addr];
-			copy_cell(array_buf, new_addr, array, i);
+			array_buf[new_addr] = array[i];
 		}
 
 		shift += RADIX_BASE;

@@ -1403,7 +1393,7 @@ static void radix_sort(struct bucket_item *array, struct bucket_item *array_buf,
 			addr = get4bits(buf_num, shift);
 			counters[addr]--;
 			new_addr = counters[addr];
-			copy_cell(array, new_addr, array_buf, i);
+			array[new_addr] = array_buf[i];
 		}
 
 		shift += RADIX_BASE;

@@ -1437,8 +1427,7 @@ static int byte_core_set_size(struct heuristic_ws *ws)
 	struct bucket_item *bucket = ws->bucket;
 
 	/* Sort in reverse order */
-	radix_sort(ws->bucket, ws->bucket_b, BUCKET_SIZE, copy_cell,
-			get4bits);
+	radix_sort(ws->bucket, ws->bucket_b, BUCKET_SIZE, get4bits);
 
 	for (i = 0; i < BYTE_CORE_SET_LOW; i++)
 		coreset_sum += bucket[i].count;

Signature · 1 line

2.15.1

David Sterba

author maintainer

2017-12-12 20:57 UTC #3 lore raw

PATCH 3/3 btrfs: heuristic: call get4bits directly

In reply to David Sterba

As it's a single instance and local to the file, we don't need to pass it as an argument.

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/compression.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

fs/btrfs/compression.c +2 -4

index 935acabc0ea7..208334aa6c6e 100644
@@ -1319,11 +1319,9 @@ static u8 get4bits(u64 num, int shift) {
  * @array_buf - buffer array to store sorting results
  *              must be equal in size to @array
  * @num       - array size
- * @get4bits  - function to get 4 bits from number at specified offset
  */
 static void radix_sort(struct bucket_item *array, struct bucket_item *array_buf,
-		       int num,
-		       u8 (*get4bits)(u64 num, int shift))
+		       int num)
 {
 	u64 max_num;
 	u64 buf_num;

@@ -1427,7 +1425,7 @@ static int byte_core_set_size(struct heuristic_ws *ws)
 	struct bucket_item *bucket = ws->bucket;
 
 	/* Sort in reverse order */
-	radix_sort(ws->bucket, ws->bucket_b, BUCKET_SIZE, get4bits);
+	radix_sort(ws->bucket, ws->bucket_b, BUCKET_SIZE);
 
 	for (i = 0; i < BYTE_CORE_SET_LOW; i++)
 		coreset_sum += bucket[i].count;

Signature · 1 line

2.15.1

Timofey Titovets

2017-12-12 21:52 UTC #4 lore raw

In reply to David Sterba

2017-12-12 23:55 GMT+03:00 David Sterba <[email protected]>:

Quoted 10 lines

The callback pointers for radix_sort are not needed, we don't plan to export the function now. The compiler is smart enough to replace the indirect calls with direct ones, so there's no change in the resulting asm code.
David Sterba (3):
btrfs: heuristic: open code get_num callback of radix sort
btrfs: heuristic: open code copy_call callback of radix sort
btrfs: heuristic: call get4bits directly

Quoted 6 lines

 fs/btrfs/compression.c | 42 +++++++++++-------------------------------
 1 file changed, 11 insertions(+), 31 deletions(-)

--
2.15.1

Thanks!
On whole series:
Reviewed-by: Timofey Titovets

Signature · 2 lines

Have a nice day,
Timofey.