xfs: avoid repeated pointer dereferences
PATCH v2 8/10 xfs: avoid repeated pointer dereferences
In xlog_find_cycle_start() use a local variable for some repeated operations rather than constantly updating the memory location whose address is passed in.
---fs/xfs/xfs_log_recover.c | 83 ++++++++++++++++++++++++-----------------------
fs/xfs/xfs_log_recover.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-)
fs/xfs/xfs_log_recover.c +14 -12
@@ -354,26 +354,28 @@ xlog_find_cycle_start( {xfs_caddr_t offset;xfs_daddr_t mid_blk; +xfs_daddr_t end_blk;uint mid_cycle;int error; - mid_blk =BLK_AVG (first_blk, *last_blk); -while (mid_blk != first_blk && mid_blk != *last_blk) { +ASSERT (last_blk !=NULL ); + end_blk = *last_blk; + mid_blk =BLK_AVG (first_blk, end_blk); +while (mid_blk != first_blk && mid_blk != end_blk) { error =xlog_bread (log, mid_blk,1 , bp, &offset);if (error)return error; mid_cycle =xlog_get_cycle (offset); -if (mid_cycle == cycle) { - *last_blk = mid_blk; -/* last_half_cycle == mid_cycle */ - }else { - first_blk = mid_blk; -/* first_half_cycle == mid_cycle */ - } - mid_blk =BLK_AVG (first_blk, *last_blk); +if (mid_cycle == cycle) + end_blk = mid_blk;/* last_half_cycle == mid_cycle */ +else + first_blk = mid_blk;/* first_half_cycle == mid_cycle */ + mid_blk =BLK_AVG (first_blk, end_blk); } -ASSERT ((mid_blk == first_blk && mid_blk+1 == *last_blk) || - (mid_blk == *last_blk && mid_blk-1 == first_blk)); +ASSERT ((mid_blk == first_blk && mid_blk+1 == end_blk) || + (mid_blk == end_blk && mid_blk-1 == first_blk)); + + *last_blk = end_blk;return 0 ; }
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 3 lines · Alex Elder
In
xlog_find_cycle_start()use a local variable for some repeated operations rather than constantly updating the memory location whose address is passed in.
Won't the compiler optimise that out for us? i.e. does the dissassembly of the function look any better before and after this change?
Quoted 13 lines
Signed-off-by Alex Elder <[email protected]>---
fs/xfs/xfs_log_recover.c| 83 ++++++++++++++++++++++++-----------------------fs/xfs/xfs_log_recover.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-)Index:
b/fs/xfs/xfs_log_recover.c
===================================================================fs/xfs/xfs_log_recover.c +0
-0@@ -354,26 +354,28 @@ xlog_find_cycle_start( {
fs/xfs/xfs_log_recover.c xlog_find_cycle_start( · line 362
Quoted 10 lines
fs/xfs/xfs_log_recover.c +3
-2xfs_caddr_t offset;xfs_daddr_t mid_blk; +xfs_daddr_t end_blk;uint mid_cycle;int error; - mid_blk =BLK_AVG (first_blk, *last_blk); -while (mid_blk != first_blk && mid_blk != *last_blk) { +ASSERT (last_blk !=NULL ); + end_blk = *last_blk;
FWIW, there is no need for that ASSERT there - the machine will panic on the very next line, anyway....
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 7 lines · Dave Chinner
Quoted 3 lines · Alex Elder
In
xlog_find_cycle_start()use a local variable for some repeated operations rather than constantly updating the memory location whose address is passed in.Won't the compiler optimise that out for us? i.e. does the dissassembly of the function look any better before and after this change?
I doubt it *can* optimize that. Doing so would change the way the function interacts semantically with the pointed-to memory. Still, I performed a quick check to be sure (on an x86_64) and the compiled code really does de-reference the pointer for both reads from and writes to memory.
Quoted 19 lines
Quoted 19 lines
Signed-off-by Alex Elder <[email protected]>---
fs/xfs/xfs_log_recover.c| 83 ++++++++++++++++++++++++-----------------------fs/xfs/xfs_log_recover.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-)Index:
b/fs/xfs/xfs_log_recover.c
===================================================================fs/xfs/xfs_log_recover.c +1
-0@@ -354,26 +354,28 @@ xlog_find_cycle_start( {xfs_caddr_t offset;xfs_daddr_t mid_blk; +xfs_daddr_t end_blk;uint mid_cycle;int error;
Quoted 7 lines
fs/xfs/xfs_log_recover.c xlog_find_cycle_start( · line 362
Quoted 4 lines
fs/xfs/xfs_log_recover.c +2
-2- mid_blk =BLK_AVG (first_blk, *last_blk); -while (mid_blk != first_blk && mid_blk != *last_blk) { +ASSERT (last_blk !=NULL ); + end_blk = *last_blk;FWIW, there is no need for that ASSERT there - the machine will panic on the very next line, anyway....
Agreed. I make a habit of stating assumptions about passed-in arguments via assertions. Not a big deal, I'll take it out.
Still soliciting a reviewed-by on this one.
-Alex
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs