On Sat, Mar 8, 2014 at 12:25 AM, Ben Reser <ben_at_reser.org> wrote:
> On 3/7/14, 3:11 PM, Stefan Fuhrmann wrote:
> > Two comments:
> >
> > * The keys are being used one-way, i.e. any type > input type will do
>
> *nod* I realize it's safe. But the problem is that we know we have a type
> that
> isn't stable for revnum's across platforms. Say we do Subversion 2.0 and
> we
> want to fix it. If we're doing stuff like this we then have to go around
> and
> find all the places where we used some other type to represent a revision
> number.
>
That is true and hard to avoid: Ultimately, we need to
map to some on-disk representation that may not
allow for open addressing.
My idea here is to enable Subversion to be built using
a C++ compiler and add some magic template header
(I posted such a thing 4 years ago or so - it's not difficult)
that prevents implicit assignment between different
integer types - even if they are the same fundamental
C type. That header would only be activated using
some configure option and not affect production code.
Can we at least have these private types for this sort of thing:
> typedef apr_int32_t svn__revnum32_t
> typedef apr_int64_t svn__revnum64_t
>
Good idea. I think, we should define those and make
the FS2 API use svn__revnum64_t consistently. It is
still a fixed / limited size value but clearly "big enough"
and "grep-able".
> If we can avoid the svn__revnum32_t that'd be nice since the conversion
> from
> svn_revnum_t to svn__revnum64_t is always safe (at least until someone
> shows up
> with a platform with 128-bit longs), while svn_revnum_t to svn__revnum32_t
> is
> not. Of course in practice all conversions would always be safe in our
> code.
>
> With the private types it'll be easy to find these cases in the future. It
> also self documents when things that aren't necessarily obviously revision
> numbers.
>
IIRC, I use 32 bit revnums in a few places in FSX, ATM.
But that code is either very much in the flux or even
destined to be removed at some point this year (FSFS
leftovers).
Don't bother fixing types and conversions over there
right now unless you think there is ordinary bug. I will
go over the code about 2 weeks before 1.9-beta and
replicate the relevant changes from FSFS to FSX.
> > * Padding is horrible because it inserts sections of random data
> > into a *hash* key. Valgrind might find these instances, if run on
> Windows.
>
> Unless you initialize the keys before filling them, but I'm guessing you
> want
> to avoid that for performance reasons.
Performance is not the problem here as the compiler
*should* remove redundant initialization. The actual
performance issue is <=16 byte structs (can be used
for cache addressing as is) vs. other keys (require a
MD5 calculation just to figure out what cache array
index to use).
Concerning { 0 } initialization, it is very hard to verify
that we actually do it everywhere. Default initializers
had been frowned upon on this list on several occasions
over the last couple of years or so. *I* am happy to use
them but they are easy to omit and a maintenance hazard.
> Are you saying that Valgrind will
> complain about uninitialized memory because of the padding on 64-bit
> Windows
> (since it's going to pad due to the 64-bit word size but long is still
> 32-bits)?
>
If there was valgrind on Windows (which is the next
problem with verification), it would spot the first place
where either the padding or any copy of it gets read.
In the case of cache keys, that would be trivially the
place when we use the struct to calculate the cache-
internal bucket index.
-- Stefan^2.
Received on 2014-03-08 14:10:51 CET