On Tue, Jul 30, 2013 at 3:57 PM, Daniel Shahaf <danie...@elego.de> wrote:
> Stefan Fuhrmann wrote on Tue, Jul 30, 2013 at 01:04:43 +0200: > > On Mon, Jul 29, 2013 at 9:06 PM, Daniel Shahaf <danie...@elego.de> > wrote: > > > > > danie...@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 in > > > a '--enable-maintainer-mode' '-C' '--enable-optimize' build. For some > > > reason my non-maintainer build fails. Can someone try the following on > > > a release build, please, and report what value they get for klen? > > > > > > % ./libtool --mode=execute gdb -silent -ex r -args subversion/svn/svn > > > update > > > (gdb) b update_internal > > > (gdb) r > > > (gdb) b apr_hash_get > > > Breakpoint 2 at 0x7ffff6b25ac0: file tables/apr_hash.c, line 342. > > > (gdb) c > > > Continuing. > > > > > > Breakpoint 2, apr_hash_get (ht=0x65c698, key=0x7ffff7bcf86c, klen=-1) > > > at tables/apr_hash.c:342 > > > 342 { > > > (gdb) up > > > #1 0x00007ffff7bccd69 in update_internal (result_rev=0x7fffffffe260, > > > conflicted_paths=<value optimized out>, > > > local_abspath=0x6987e0 "/home/danielsh/src/svn/t1", > > > anchor_abspath=0x698a60 "/home/danielsh/src/svn/t1", > > > revision=0x7fffffffe170, depth=svn_depth_unknown, > depth_is_sticky=0, > > > ignore_externals=0, allow_unver_obstructions=0, > adds_as_modification=1, > > > timestamp_sleep=0x7fffffffe26c, notify_summary=1, ctx=0x65d810, > > > pool=0x6986a8) at subversion/libsvn_client/update.c:240 > > > 240 ? svn_hash_gets(ctx->config, > > > SVN_CONFIG_CATEGORY_CONFIG) > > > (gdb) > > > > > > I would expect 6, since SVN_CONFIG_CATEGORY_CONFIG is known at > > > compile-time. > > > > > > > A two things have been broken here: > > > > Thanks for fixing this. Collective review of your commits: > > r1508221: should update the docstring of SVN_HAS_DUNDER_BUILTINS in > configure.ac to remove mention of __builtin_choose_expr > > r1508221: should update the AC_COMPILE_IFELSE call in the > cross-compilation branch to remove __builtin_choose_expr Fixed in r1508458. r1508222: the #ifndef seems to be the wrong solution to whatever problem > 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 > earlier?)" > #endif > No go. This is a public API header. People will not define SVN_HAS_DUNDER_BUILTINS in their applications. > and adding a > #define SVN_HAS_DUNDER_BUILTINS 0 > to subversion_config_private.hw? > Done in r1508460, 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). > 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. So, feel free to do some search/replace magic to handle the remaining files. 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. > > > * the ac macro (fixed in r1508221) > > * the definition / #include order (detected as of r1508222, fixed in > > r1508225) > > > > Now, I see the optimization work on my machine again. > > > > Great, thanks for finishing what I'd started! > Thanks for the review! -- Stefan^2.
