It really doesn't make that much of a difference. APR_HASH_KEY_STRING
simply tells the hash function to watch for NUL rather than to
traverse N characters. Because N doesn't have to be tracked, you might
even argue the NUL-test is faster.
IOW, passing APR_HASH_KEY_STRING does NOT perform a strlen(). So yes:
a lot of this work isn't saving much at all.
Just look at apr/tables/apr_hash.c:hashfunc_default()
Cheers,
-g
(tho we'll leave it to Stefan and Daniel to decide whether to blow
more of their time on this "issue" or just back it all out)
On Tue, Jul 30, 2013 at 12:03 PM, Bert Huijben <bert_at_qqmail.nl> wrote:
> build/ac-macros/svn-macros.m4 configure.ac subversion/include/svn_hash.h
> MIME-Version: 1.0
> Content-Type: text/plain; charset="utf-8"
> Content-Transfer-Encoding: quoted-printable
>
> If this really makes a measurable difference for the average user
> (which I doubt it even is for our use) this belongs in Apr.
>
> Until then I think we are wasting a lot of development time in
> optimizing a non measurable piece of our code, with
> unconfirmed 'improvements' that time after time cause new problems.
>
> Bert From: Daniel Shahaf
> Sent: =E2=80=8E30/=E2=80=8E07/=E2=80=8E2013 16:47
> To: Stefan Fuhrmann
> Cc: Subversion Development; commits_at_subversion.apache.org
> Subject: Re: svn commit: r1508170 - in /subversion/trunk:
> build/ac-macros/svn-macros.m4 configure.ac subversion/include/svn_hash.h
> Stefan Fuhrmann wrote on Tue, Jul 30, 2013 at 16:30:00 +0200:
>> On Tue, Jul 30, 2013 at 3:57 PM, Daniel Shahaf <danielsh_at_elego.de> wrote:
>>=20
>> > Stefan Fuhrmann wrote on Tue, Jul 30, 2013 at 01:04:43 +0200:
>> > > On Mon, Jul 29, 2013 at 9:06 PM, Daniel Shahaf <danielsh_at_elego.de>
>> > wrote:
>> > >
>> > > > danielsh_at_apache.org wrote on Mon, Jul 29, 2013 at 18:35:26 -0000:
>> > > > > Author: danielsh
>> > > > > Date: Mon Jul 29 18:35:25 2013
>> > > > > New Revision: 1508170
>> > > > >
>> > > > > URL: http://svn.apache.org/r1508170
>> > > > > Log:
>> > > > > Follow-up to r1507366: svn_hash_gets: compute the length of
>> > > > > string literal keys (common case) at compile-time, without
>> > > > > multiply-evaluating dynamically-computed keys.
>> > > > >
>> > > > > Review by: philip
>> > > > > breser
>> > > >
>> > > > Oddly enough, the optimization doesn't seem to kick in for me
> ...
>> > > >
>> > >
>> > > A two things have been broken here:
>> > >
>> >
>> > Thanks for fixing this. Collective review of your commits:
>> >
>> > r1508222: the #ifndef seems to be the wrong solution to whatever proble=
> m
>> > it is trying to fix. If it's trying to generate a compiler warning, it
>> > should use the #warning preprocessor directive; if it's trying to
>> > silence one about use of an undefined macro, how about doing
>> > #ifndef SVN_HAS_DUNDER_BUILTINS
>> > #error "Build broken (do you need to include svn_private_config.h ear=
> lier?)"
>> > #endif
>> >
>>=20
>> No go. This is a public API header. People will not define
>> SVN_HAS_DUNDER_BUILTINS in their applications.
>>=20
>
> It would be nice if we can propagate the fix to C API consumers too.
>
> How can we do that? Do we just document the macro (say, in an #ifdef
> DOXYGEN block) along with an example of how to detect the setting for
> it in various build systems? Or do try to sniff it at compile time?
>
>> > r1508225: changing the include order //only in places that currently
>> > include svn_hash.h// seems like a bad idea; we should change it
>> > _everywhere_ (since other places may grow svn_hash.h includes in the
>> > future).
>> >
>>=20
>> Well, I got kind of fed up after manually patching
>> ~180 files (about the same number of files left to
>> check). Even the commit took longer than 4 minutes.
>>=20
>> So, feel free to do some search/replace magic to
>> handle the remaining files.
>
> What makes you think I'm not as lazy as you are? :-)
>
> Index: build/ac-macros/compiler.m4
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- build/ac-macros/compiler.m4=09(revision 1508471)
> +++ build/ac-macros/compiler.m4=09(working copy)
> @@ -74,6 +74,7 @@ AC_DEFUN([SVN_CC_MODE_SETUP],
> =20
> CNOWARNFLAGS=3D"$CFLAGS"
> CFLAGS=3D"$CFLAGS_KEEP"
> + CFLAGS=3D"-DSVN__CORE $CFLAGS"
> =20
> AC_SUBST(CMODEFLAGS)
> AC_SUBST(CNOWARNFLAGS)
> Index: build/generator/gen_win.py
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- build/generator/gen_win.py=09(revision 1508471)
> +++ build/generator/gen_win.py=09(working copy)
> @@ -714,7 +714,8 @@ class WinGeneratorBase(gen_win_dependencies.GenDep
> fakedefines =3D ["WIN32","_WINDOWS","alloca=3D_alloca",
> "_CRT_SECURE_NO_DEPRECATE=3D",
> "_CRT_NONSTDC_NO_DEPRECATE=3D",
> - "_CRT_SECURE_NO_WARNINGS=3D"]
> + "_CRT_SECURE_NO_WARNINGS=3D",
> + "SVN__CORE"]
> =20
> if cfg =3D=3D 'Debug':
> fakedefines.extend(["_DEBUG","SVN_DEBUG"])
> Index: subversion/include/svn_hash.h
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- subversion/include/svn_hash.h=09(revision 1508471)
> +++ subversion/include/svn_hash.h=09(working copy)
> @@ -248,7 +248,11 @@ svn_hash_from_cstring_keys(apr_hash_t **hash,
> * below will usually generate a compiler warning if your definition is
> * being encountered _after_ #including this header.=20
> */
> -#define SVN_HAS_DUNDER_BUILTINS 0
> +# ifdef SVN__CORE
> +# error "Build broken (should you include svn_private_config.h earlier?)"
> +# else
> +# define SVN_HAS_DUNDER_BUILTINS 0
> +# endif
> #endif
> =20
> /** Shortcut for apr_hash_get() with a const char * key.
>
> Untested, but I'll add it to my queue for next week.
>
>> > Also, let's take this opportunity to document our preferred order of
>> > #include's in HACKING (The relative order of: stdlib, apr, mod_dav, bdb=
> ,
>> > serf, sqlite, svn_*.h, svn_*_private.h, ./*.h, ../libsvn_{fs|ra}/*.h,
>> > subversion_config_private.h, and APR_WANT_FOO) --- I always end
>> > up copying the order from some existing file.
Received on 2013-07-30 21:56:37 CEST