This is downright horrible for code maintainability. We have a type for revision numbers.
It's bad enough we don't have a stable type for revision numbers across platforms. But now you're abusing it and using 32-bit integers in some places for them and 64-bit values in others. Now that you're doing this I'm now of the opinion that I should veto the code that limits them to 32-bits at the RA layer. Because all it's done is encourage us to do things like this. On 3/7/14, 2:11 PM, stef...@apache.org wrote: > Author: stefan2 > Date: Fri Mar 7 22:11:24 2014 > New Revision: 1575428 > > URL: http://svn.apache.org/r1575428 > Log: > In FSFS, remove padding from cache key structs to simplify handling. > Also, document why we want them to be 16 bytes whenever we can help it. > > * subversion/libsvn_fs_fs/fs.h > (pair_cache_key_t): Widen revision type to eliminate padding on > platforms with sizeof(long) != 8. Document the > struct size rationale. > (window_cache_key_t): Document the struct size rationale. > > Modified: > subversion/trunk/subversion/libsvn_fs_fs/fs.h > > Modified: subversion/trunk/subversion/libsvn_fs_fs/fs.h > URL: > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs.h?rev=1575428&r1=1575427&r2=1575428&view=diff > ============================================================================== > --- subversion/trunk/subversion/libsvn_fs_fs/fs.h (original) > +++ subversion/trunk/subversion/libsvn_fs_fs/fs.h Fri Mar 7 22:11:24 2014 > @@ -254,20 +254,27 @@ typedef struct fs_fs_dag_cache_t fs_fs_d > > /* Key type for all caches that use revision + offset / counter as key. > > - NOTE: always initialize this using calloc() or '= {0};'! This is used > - as a cache key and the padding bytes on 32 bit archs should be zero for > - cache effectiveness. */ > + Note: Cache keys should be 16 bytes for best performance and there > + should be no padding. */ > typedef struct pair_cache_key_t > { > - svn_revnum_t revision; > + /* The object's revision. Use the 64 data type to prevent padding. */ > + apr_int64_t revision; > > + /* Sub-address: item index, revprop generation, packed flag, etc. */ > apr_int64_t second; > } pair_cache_key_t; > > -/* Key type that identifies a txdelta window. */ > +/* Key type that identifies a txdelta window. > + > + Note: Cache keys should be 16 bytes for best performance and there > + should be no padding. */ > typedef struct window_cache_key_t > { > - /* Revision that contains the representation */ > + /* Revision that contains the representation. > + We limit this to 32 bit because it revision numbers are practically > + limited to 32 bits by the RA layer, ATM, and we want to keep this > + struct 16 bytes long. */ > apr_uint32_t revision; > > /* Window number within that representation */ > >
