Fish Trophy

jjelinek got a fish trophy!

10809 Performance optimization of AVL tree comparator functions

Review Request #1771 — Created April 18, 2019 and submitted

jjelinek
illumos-gate
general

This is a port from ZoL of
ee36c709c3d Performance optimization of AVL tree comparator functions

I copied the performance results from the ZoL commit message into the bug report.

zfs-test run

  • 4
  • 0
  • 0
  • 3
  • 7
Description From Last Updated
We should not just call into __builtin_expect everywhere. This should be rigged up in sys/ccompile.h as a macro like other ... rm rm
Should these be documented as part of libavl(3AVL) and avl(9F)? rm rm
I think we should probably consider keeping these as undocumented for the time being. rm rm
Rather than avl, should we use zfs here and in the userland header? rm rm
tsoome
  1. Ship It!
  2. 
      
igork
  1. Ship It!
  2. 
      
rm
  1. I didn't go through all of these, but I think the typing isn't right on almost all of these. In many cases the AVL_CMP macro is taking the raw types and always assigning it to an int, truncating it in the process. I'm not sure this is sound. I stopped auditing them all, because it seeemd true of almost all of them.

    1. Sorry, I realized just now that I misread the macro entirely so we're always doing two boolean comparisons and subtracting them. They don't have that problem, some of the other issues still need to be dealt with.

  2. We should not just call into __builtin_expect everywhere. This should be rigged up in sys/ccompile.h as a macro like other compiler specific flags to make sure that when it's not present, we basically just only have x output.
  3. usr/src/uts/common/fs/zfs/vdev_label.c (Diff revision 1)
     
     
    We're subtracting two uint64_t values, why is an int the right type for this?
  4. usr/src/uts/common/fs/zfs/vdev_label.c (Diff revision 1)
     
     
    This is truncating a uint64_t to an int, I don't believe that's right.
  5. usr/src/uts/common/fs/zfs/vdev_queue.c (Diff revision 1)
     
     
    This is converting off_t to an int, I don't believe that's right.
  6. usr/src/uts/common/sys/avl.h (Diff revision 1)
     
     
     
     
     
    Should these be documented as part of libavl(3AVL) and avl(9F)?
  7. 
      
jjelinek
rm
  1. Thanks for adding the guards. I have one or two other minor comments, but otherwise it looks good.

  2. usr/src/man/man3avl/AVL_CMP.3avl (Diff revision 2)
     
     
    I think we should probably consider keeping these as undocumented for the time being.
  3. Rather than avl, should we use zfs here and in the userland header?
  4. 
      
jjelinek
rm
  1. Ship It!
  2. 
      
jjelinek
Review request changed

Status: Closed (submitted)

Loading...