{"thread":{"id":"26236","subject":"[RFD] My thoughts about implementing gitweb output caching","startedAt":"2011-01-07T23:42:34Z","lastAt":"2011-01-08T11:44:04Z","messageCount":5,"participants":["Jakub Narebski","Jonathan Nieder","Nicolas Pitre"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"159182","messageId":"201101080042.36156.jnareb@gmail.com","threadId":"26236","inReplyTo":null,"subject":"[RFD] My thoughts about implementing gitweb output caching","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-01-07T23:42:34Z","receivedAt":"2011-01-07T23:42:34Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Those are my thoughts on gitweb output caching, brought by working on\nmy rewrite of gitweb output caching, based on ideas (and some code)\nfrom John 'Warthog9' Hawley (J.H.) series, and on reviewing J.H. code.\n\nI'd apreciate very much comments on this\n\nTable of contents:\n~~~~~~~~~~~~~~~~~~\n 1. Splitting gitweb (and infractructure for it)\n 2. require Gitweb::Cache vs do 'cache.pl'\n 3. cache key (href(-replay => 1,...) vs $cgi->self_url())\n 4. caching, error handling, progress info and persistent environments\n    (blocking progress info via parameter, or referer, or ...)\n 5. capturing and saving STDOUT by redirection vs select(FH) or print $out\n 6. being able to use other caching engines, and capturing to scalar\n    capturing to memory vs capturing to file, \n 7. saving in text or binary; printing data from cache in binary mode\n 8. atomic file write and locking\n 9. background cache generation\n10. HTTP aware output caching, HTTP aware caching engine (c.f. CHI)\n\n\n\n1. Splitting gitweb (and infractructure for it)\n\nBecause gitweb is getting quite large (7341 lines, in byte size next\nto largest in git.git repository after gitk-git/gitk), and caching is\nmeant to be optional, therefore (most of) code related to gitweb\ncaching should be put in a separate file.\n\nThis means that adding support for caching to gitweb should bring\ninfrastructure that will be required for splitting gitweb for easier\nmaintenance and extending.  Which is a good side effect of adding\ncaching support to gitweb.\n\nOne question remains: current infrastructure supports having gitweb\nmodules (or \"modules\", see the next item) in 'lib/' subdirectory\nalongside gitweb.cgi, and in directories in PERL5LIB or PERLLIB.\nThere was request to support installing gitweb modules in a separate\ndirectory, but that would require changes to \"gitweb: Prepare for\nsplitting gitweb\" patch (but it is doable).  Is there wider interest\nin supporting such feature?\n\n\n2. require Gitweb::Cache vs do 'cache.pl'\n\nOne of the differences between original J.H. code and my rewrites of\nit is that in J.H. patches caching code is in 'cache.pm' file and must\nbe loaded using \"do 'cache.pm'\", while in my rewrite caching is put\ninto proper Perl module and can be loaded using something like\n\"require Gitweb::Cache\".\n\n(Digression: the name 'cache.pm' is bad because of two reasons.\nFirst, the *.pm extension sugests that it is a proper Perl module,\nwith a separate namespace, that can be loaded using \"require\".\nSecond, it is good practice to restrict all lowercase package names to\npragma-like modules, like 'strict', 'warnings' or 'namespace::clean'.\nThat is why in \"minimal fixup\" response to J.H. patches I renamed\nit from cache.pm to cache.pl (or rather lib/cache.pl).)\n\nThe main advantage of 'cache.pl' solution is that you don't need to\nseparate gitweb parts from caching parts, and you don't need to\ncarefully design API for interaction between caching layer and\napplication layer.  For example in J.H.'s patches cache.pm uses\nvariables and subroutines from gitweb.perl, e.g. $my_url or %actions,\nand die_error.  This allow for fast no-design development... leading\nto (somewhat at least) spaghetti code.\n\nBut this is also main disadvantage of this method.  Without proper\nseparation (proper encapsulation) of caching layer testing the caching\nengine separately would be difficult, if at all possible (perhaps with\nmocking...).  In my rewrite caching engine has a separate set of\ntests checking its behavior; in J.H. series it doesn't.\n\nCurrently 'cache.pl' is loaded in \"minimal fixup\" version using the\nfollowing code\n\n  if (!exists $INC{'cache.pl'}) {\n    my $return = do 'cache.pl';\n    die $@ if $@;\n    die \"Couldn't read 'cache.pl': $!\" if (!defined $return);\n  }\n\nthough nowadays I think that \"require 'cache.pl';\" could be enough.\n\nRemoving some of entaglements is quite easy: instead of calculating key\nbased on $my_url or $cgi in cache_fetch, calculate it in caller i.e. in\ngitweb; instead of passing $action and using $actions{$action}->(), just\npass code reference i.e. $actions{$action} as parameter; pass options in\na hash or hashref.  Removing other dependencise is harder: to remove\ndependency on die_error you need either 'on_error' handler, or extract\nerror handling in separate package and use it (which is harder than it\nlooks, at least with current error handling, but see below).\n\nThere is also additional advantage in putting caching layer into\nseparate Perl package: there is possibility of (re)using it outside of\ngitweb.  But this is rather minor issue...\n\n\n3. cache key (href(-replay => 1,...) vs $cgi->self_url())\n\nThe key used for gitweb caching, to find apropriate cache entry, is\nbased on URL of current page.  The question is if some URLs are to be\nconsidered equivalent, and what class of URL do we consider equivalent,\ni.e. having the same key.\n\nSimplest solution is to use $cgi->self_url() (note that what J.H. v8\nuses, i.e.: \"$my_url?\". $ENV{'QUERY_STRING'}, where $my_url is just\n$cgi->url() is not enough - it doesn't take path_info into account).\n\nAlternate solution, which I used in my rewrite, is to come up with\n\"canonical\" URL, e.g. href(-replay => 1, -full => 1, -path_info => 0);\nwith this solution using path_info vs query parameters or reordering\nquery parameters still gives the same key.\n\n\n4. caching, error handling, progress info and persistent environments\n   (blocking progress info via parameter, or referer, or ...)\n\nThis needs an intro:\n\nJ.H. gitweb caching series introduced non-JavaScript HTML-only progress\nindicator, to notify user that there is data being (re)generated.  It is\nbased on the trick that HTML redirection via http-equiv=\"Refresh\" meta\nheader is done only after connection is closed / HTML page is rendered\nin full.  So by not printing final '</html>' and closing connection, and\nby printing e.g. '.' every few seconds to prevent closing connection, we\ncan have simple progress indicator.\n\nOther solution, presented in one of first versions of my rewrite, would\nbe to use Ajax based progress indicator.  The problem with this solution\nis that you need to guess if web browser (client) supports JavaScript\nfrom server side; though there are tricks to do that (see\n'blame_incremental' support).\n\nJ.H. patches up and including v7, and my rewrite up and including v6,\nexcluded error pages from caching.  I think that the original resoning\nbehind choosing to do it this way was that A.), each of specific error\npages is usually accessed only once, so caching them would only take up\nspace bloating cache, but what is more important B.) that you can't\ncache errors from caching engine.\n\nError handling in gitweb is done using die_error() subroutine (renamed\nfrom error_page() very, very early in gitweb history).  It is used\nsimilarly to how \"die\" could be used in an ordinary application; in\nplace of for example\n\n  open my $fh, '<', $filename\n  \tor die \"Can't open $filename: $!\"\n\ngitweb uses\n\n  open my $fd, \"-|\", git_cmd(), \"ls-tree\", $base, \"--\", $path\n  \tor die_error(500, \"Open git-ls-tree failed\");\n\nBecause gitweb is not a simple web application, and error can occur\nquite deep in the call stack, having to check if each subroutine ended\nin an error is impractical: it is much easier to rely on the fact that\ncalling die_error() ends current request.\n\n\nDescription of problem:\n\nThe problem is interaction with HTML-only progress indicator, and\nnon-cached pages.  When the \"Generating...\" page starts to be send to\nweb browser (in my rewrite there was initial delay; if non-cached page\nstarted to be generated within this initial delay then \"Generating...\"\npage was not displayed at all), it means that page we wait for has to be\ngenerated in background process.  When the page is finished,\n\"Generating...\" page would force redirection... but if page it was\nwaiting for is not saved in cache, the same thing would repeat: again\nrunning generating data in background, again \"Generating...\" page.\n\nSo what's the solution?  One would be to actually cache error pages.\nThis is not easy with die_error() both printing error page (sending it\nto web browser after turning off capturing), and ending request,\noriginally by the way of exit(0) (note: ModPerl::Registry redefines it\nto end request and not worker process), later with nonlocal goto to\nEND_GITWEB or (in one of my unapplied patches) a bit later to\nEND_REQUEST label.  The proposed solution is to actually separate the\ncreating and throwing error from generating an error page; this should\nallow easy caching of error pages -- gitweb would use die/eval to throw\nand catch exceptions (similar to what SVN::Web does).\n\nPossible improvement, but an additional complication, would be to make\ncached error pages shorl lived, perhaps cleaning them up after using\nthem.  This might be done via HTTP awareness, see below.\n\nAnother possible solution I have just thought about is to ensure somehow\nthat page (re)loaded from \"Generating...\" page has progress info\ndisabled.  But that means that error page would get generated twice,\nsecond time without progress indicator.\n\nThis is cerrtainly an issue that needs some thought.\n\n\n5. capturing and saving STDOUT by redirection vs select(FH) or print $out\n\nOne of the goals in designing capturing mechanism for caching engine to\nbe used in gitweb output caching is that when caching is disabled gitweb\nbehaviour should not change at all (or at least the change should be\nminimal).  That means not collecting output to print it or save it to\ncache at the end, as this would change bahvior from current streaming,\nprint as you go behavior... and make 'blame_data' part of\n'blame_incremental' useless.\n\nThe solution with changing every 'print LIST' to 'print $out LIST' was\ndiscarded because it would mean changes in almost all of gitweb,\nresulting in very large patch.\n\nThe solution finally chosen is to redirect STDOUT to a file (reopen to a\nfile), while duplicating it to save it for later restoring.  This is the\nsame solution that Capture::Tiny uses.\n\nAlternate solution is to use select(FH) to change default filehandle for\n'print' and 'printf'... but this seems less elegant solution.\n\n\n6. being able to use other caching engines, and capturing to scalar\n   capturing to memory vs capturing to file, \n\nThe standard API to access cache supported by various Perl caching\nengines (Cache::Cache, Cache, CHI, Cache::FastMmap, Cache::Memcached) is\nbased around $cache->get($key) and $cache->set($key, $data) methods:\n\n  my $data = $cache->get($key);\n\n  if (!defined $data) {\n    $data = compute_data($key);\n    $cache->set($key, $data);\n  }\n\nSome caching engines support single instruction \"get and set\" equivalent\nto above code, by providing code callback to generate data if it is not\nin cache, though it is not as standardized as ->get/->set methods API.\nCHI for example has $cache->compute($key, $code) method.\n\nNote that $data might be a reference to a more complicated Perl\nstructure (ir might be arrayref or hashref) -- the usual example is to\ncache results of a database query.  This means that $data must be\nserialized before writing to cache, though some cache implementations\nallow to cache raw, unserialized data.  This is complicated even more by\nthe fact that some caching engines wrap $data in a cache entry object\nwhich stores entry-specific options, e.g. expiration time for cache\nentry, so we need serializing even if $data is scalar.\n\nMore important issue is that (with sole exception of old Cache engine)\ncaching is done for data stored in memory.  For caching captured output,\nwhile using file-based cache, this means that when generating data we\nhave to capture output to variable (e.g. by capturing it to in-memory\nfile, or capturing to temporary file and slurping it to variable / to\nmemory), and then dump (spew) this variable to cache entry file (perhaps\nwith serialization as intermediate step).  For when data exist in file\nbased cache this means slurping contents of cache entry file to variable\n/ to memory (perhaps with deserialization in between), then dumping\n(spewing) this content to STDOUT.\n\nThis means that to support existing caching engines (Cache::Cache, CHI,\nCache::FastMmap, Cache::Memcached, Cache::Ref, etc.) we need to support\ncapturing output to scalar, and dumping data to STDOUT.\n\nIf caching driver gitweb uses is file based, if data being cached is\nunstructured and by itself does not require serialization, and if\ncapturing output is based on redirecting STDOUT to a file (perhaps\nin-memory file), then this intermediate step of in-memory representation\n(of storing captured output in variable) seems wasteful.  Why not\ncapture directly to cache entry file, or dump / copy contents of cache\nentry file directly to STDOUT?  The problem is to design a good API that\nallows separation of caching, capturing, cache management and gitweb\nitself.\n\nIn my rewrite I have chosen to model API after\n\n  my $data = $cache->compute($key, $code);\n\nThe difference is that instead of returning scalar value (from cache or\ncalculated by $code), ->compute_fh() method returns a pair of filehandle\n(opened for reading) and filename:\n\n  my ($fh, $filename) = $cache->compute_fh($key, $code_fh);\n\nThe $code_fh is coderef, which instead of returning value to be cached,\nis passed filehandle opened for writing and filename to write to.\n\n\n7. saving in text or binary; printing data from cache in binary mode\n\nI'd like to repeat what I have written a few times earlier: if we take\ncare that PerlIO ':utf8' or ':raw' layers are applied correctly during\ncapture, then the data saved to cache can be treated as binary.  This\nmeans that we can read cache entry file in ':raw' mode and dump it to\nSTDOUT which is also in ':raw' (binary mode), regardless whether output\nwas done in text or binary mode.\n\nThis means that there is no need to special case in the caching layer\nactions which output or can write binary (':raw') output.\n\n\n8. atomic file write and locking\n\nThere is yet another are where original J.H. patches and my rewrite\ndiffers, namely in using atomic file write and in locking.\n\nIn my code caching engine writes data to temporary file (File::Temp\nwithout locking, <entry>.tmp with locking), then it gets [atomically]\nrenamed to final destination.  This means that if there is data in cache\n(possibly stale), we can read it *without locking*.\n\nLocking in my rewrite is needed only to chose one of clients as the one\nthat would be (re)generating data in cache, which is done via non\nblocking writer (exclusive) lock, and to notify other clients when data\nis ready (written to cache), via blocking reader (shared) lock.\n\nSidenote: CHI has different mechanism to avoid cache miss stampedes\n(though it doesn't protect against generic concurrent access when there\nis no data in cache), namely the 'expires_variance' mechanism, see CHI\ndocumentation, e.g. http://p3rl.org/CHI.  It is meant to protect against\nstampede at the end of cache expiration time, via \"smearing\" expiration\ntime: it allows items to expire a little earlier than the stated\nexpiration time.\n\n\n9. background cache generation\n\nIn my rewrite caching engine first checks if forking a background\nprocess is needed (it is configurable), and only then fork() is used.\n\nThere are two slightly different situations where background cache\ngeneration can be used.  First is when there is stale (but not too\nstale) data to be served.  In this case caching engine runs process\ngenerating data in background, and foreground process (all foreground\nprocesses: the one responsible for regenerating data (got lock) and\nothers) returns stale data.  Foreground process doesn't, and shouldn't,\nwait for background process.  This means that background process has to\nbe detached as to not generate zombie processes.  I do this in my\nrewrite.\n\nSecond is if there is no stale data to serve (or data is too stale), but\nwe have progress indicator.  In this case the foreground process is\nresponsible for rendering progress indicator, and background process is\nresponsible for generating data.  In this case foreground process waits\nfor data to be generated (unless progress info subroutine exits), so\nstrictly spaking we don't need to detach background process in this\ncase.\n\n\n10. HTTP aware output caching, HTTP aware caching engine (c.f. CHI)\n\nIn one of additional patches sent in last version (v7) of my (shortened)\nrewrite I have make use of the fact that what gitweb caches is HTTP\nresponse.\n\nOne of things that were done in this patch was adding 'Content-Length'\nheader if it was not present already.  In the case of this header, it\ncan be added when printing data from cache, or it can be added when\nsaving data to cache.  In the case of adding it when saving data to\ncache it can be done by caching engine itself, or it can be done by\noutput caching \"middleware\".  (In the case of patch in question it is\nadded during printing by output caching middleware.)\n\nOther such header is 'Expires' header (or 'Cache-Control: max-age=').\nIt can be used when printing data from cache to reflect _current_\nexpiration time (which depends on current load in the case of adaptive\ncache lifetime), like it is done in my patch.  On the other hand it can\nbe used by caching engine to store per-entry expiration time: current\ncode only supports global (per-cache) expiration time.\n\nThere are however two complications with 'Expires' header.  First, it\nshould never be in the past - this might happen when we are serving\nstale data while waiting for data in cache to be regenerated,... or if\ngitweb set some expire time, like when it knows that the data would\nnever change, because link uses SHA-1 identifier.  Second, when gitweb\nsets expire time then outpot cahcing middleware should probably rewrite\nit to be maximum of gitweb's expire at the time of generation and cache\nentry expire... or gitweb can be modified to use Cache-Control: max-age\ninstead.\n\nPerhaps we could also add 'Last-Modified' header, setting it to the date\nof cache generation; maybe even add support for If-Modified-Since\nconditional request, though I am not sure where in the layer it should\nbe added: perhaps yet another middleware between gitweb and output\ncaching?\n\nWith output caching gitweb can also support 'Range' requests, which\nmeans that it would support resumable download.  This would mean hat we\nwould be able to resume downloading of snapshot (or in the future\nbundle)... if we cannot do this now.  This would require some more code\nto be added.\n\nWe might also want to compress body (payload) of HTTP response, and add\n'Content-Encoding: gzip' header... perhaps supporting transparent\ndecompression in the case when web browser doesn't support required\nencoding.  In this case we must do compression and adding of header when\nsaving data to cache, and must do transparent decompression (if needed)\non sending data to browser.\n\nAnother HTTP header that could be added is 'X-Sendfile' or\n'X-Accel-Redirect', though I think that it needs to be done in the\noutput caching middleware (i.e. cache_output subroutine in\nGitwebCache::CacheOutput in my rewrite).\n\n-- \nJakub Narębski\nPoland\n"},{"id":"159189","messageId":"20110108002643.GD15495@burratino","threadId":"26236","inReplyTo":"201101080042.36156.jnareb@gmail.com","subject":"Re: [RFD] My thoughts about implementing gitweb output caching","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-01-08T00:26:43Z","receivedAt":"2011-01-08T00:26:43Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nThanks for these design notes.  A few uninformed reactions.\n\nJakub Narebski wrote:\n\n> There was request to support installing gitweb modules in a separate\n> directory, but that would require changes to \"gitweb: Prepare for\n> splitting gitweb\" patch (but it is doable).  Is there wider interest\n> in supporting such feature?\n\nIf you are referring to my suggestion, I see no reason to wait on\nthat.  The lib/ dir can be made configurable later.\n\n> Simplest solution is to use $cgi->self_url() (note that what J.H. v8\n> uses, i.e.: \"$my_url?\". $ENV{'QUERY_STRING'}, where $my_url is just\n> $cgi->url() is not enough - it doesn't take path_info into account).\n>\n> Alternate solution, which I used in my rewrite, is to come up with\n> \"canonical\" URL, e.g. href(-replay => 1, -full => 1, -path_info => 0);\n> with this solution using path_info vs query parameters or reordering\n> query parameters still gives the same key.\n\nIt is easy to miss dependencies on parts of the URL that are being\nfuzzed out.  For example, the <base href...> tag is only inserted with\npath_info.  Maybe it would be less risky to first use self_url(), then\ncanonicalize it in a separate patch?\n\n> J.H. patches up and including v7, and my rewrite up and including v6,\n> excluded error pages from caching.  I think that the original resoning\n> behind choosing to do it this way was that A.), each of specific error\n> pages is usually accessed only once, so caching them would only take up\n> space bloating cache, but what is more important B.) that you can't\n> cache errors from caching engine.\n\nPerhaps there is a user experience reason?  If I receive an error page\ndue to a problem with my repository, all else being equal, I would\nprefer that the next time I reload it is fixed.  By comparison, having\nto reload multiple times to forget an obsolete non-error response\nwould be less aggravating and perhaps expected.\n\nBut the benefit from caching e.g. a response from a broken link would\noutweigh that.\n\n> Second is if there is no stale data to serve (or data is too stale), but\n> we have progress indicator.  In this case the foreground process is\n> responsible for rendering progress indicator, and background process is\n> responsible for generating data.  In this case foreground process waits\n> for data to be generated (unless progress info subroutine exits), so\n> strictly spaking we don't need to detach background process in this\n> case.\n\nWhat happens when the client gets tired of waiting and closes the\nconnection?\n\n> With output caching gitweb can also support 'Range' requests, which\n> means that it would support resumable download.  This would mean hat we\n> would be able to resume downloading of snapshot (or in the future\n> bundle)... if we cannot do this now.  This would require some more code\n> to be added.\n\nExciting stuff.\n\nTeaching gitweb to generate bundles sounds like a recipe for high server\nloads, though.  I suspect manual (or by cronjob) generation would work\nbetter, with a possible exception of very frequently cloned and\ninfrequently pushed-to repos like linus's linux-2.6.\n\nJonathan\n"},{"id":"159198","messageId":"alpine.LFD.2.00.1101072142550.22191@xanadu.home","threadId":"26236","inReplyTo":"20110108002643.GD15495@burratino","subject":"Re: [RFD] My thoughts about implementing gitweb output caching","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2011-01-08T02:46:30Z","receivedAt":"2011-01-08T02:46:30Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Fri, 7 Jan 2011, Jonathan Nieder wrote:\n\n> > With output caching gitweb can also support 'Range' requests, which\n> > means that it would support resumable download.  This would mean hat we\n> > would be able to resume downloading of snapshot (or in the future\n> > bundle)... if we cannot do this now.  This would require some more code\n> > to be added.\n> \n> Exciting stuff.\n> \n> Teaching gitweb to generate bundles sounds like a recipe for high server\n> loads, though.  I suspect manual (or by cronjob) generation would work\n> better, with a possible exception of very frequently cloned and\n> infrequently pushed-to repos like linus's linux-2.6.\n\nEven for Linus' linux repo, it is not a good idea to auto create bundle, \nexcept maybe once every major release which is every 3 months or so.  I \nreally don't think this is a good idea to put this in the realm of \ngitweb caching.\n\n\nNicolas\n"},{"id":"159209","messageId":"201101081215.42188.jnareb@gmail.com","threadId":"26236","inReplyTo":"alpine.LFD.2.00.1101072142550.22191@xanadu.home","subject":"Re: [RFD] My thoughts about implementing gitweb output caching","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-01-08T11:15:39Z","receivedAt":"2011-01-08T11:15:39Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Nicolas Pitre wrote:\n> On Fri, 7 Jan 2011, Jonathan Nieder wrote:\n> > Jakub Narebski wrote:\n\n> > > With output caching gitweb can also support 'Range' requests, which\n> > > means that it would support resumable download.  This would mean hat we\n> > > would be able to resume downloading of snapshot (or in the future\n> > > bundle)... if we cannot do this now.  This would require some more code\n> > > to be added.\n> > \n> > Exciting stuff.\n> > \n> > Teaching gitweb to generate bundles sounds like a recipe for high server\n> > loads, though.  I suspect manual (or by cronjob) generation would work\n> > better, with a possible exception of very frequently cloned and\n> > infrequently pushed-to repos like linus's linux-2.6.\n> \n> Even for Linus' linux repo, it is not a good idea to auto create bundle, \n> except maybe once every major release which is every 3 months or so.  I \n> really don't think this is a good idea to put this in the realm of \n> gitweb caching.\n\nYou are right, making gitweb aware of bundles (if they are put in some\nspecified place, e.g. .git/objects/pack or .git/objects/bundle) is I guess\na better idea than allowing for gitweb to generate bundles, perhaps\nonly if caching is enabled, and perhaps with very long expiration time.\n\nOf course the standard way should be also the standard place for git\nclients to find bundles in proposed extension to git-clone.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"159211","messageId":"201101081244.05287.jnareb@gmail.com","threadId":"26236","inReplyTo":"20110108002643.GD15495@burratino","subject":"Re: [RFD] My thoughts about implementing gitweb output caching","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-01-08T11:44:04Z","receivedAt":"2011-01-08T11:44:04Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sat, 8 Jan 2011, Jonathan Nieder wrote:\n> \n> [...] A few uninformed reactions.\n\nThanks for your comments.\n\n> Jakub Narebski wrote:\n> \n> > There was request to support installing gitweb modules in a separate\n> > directory, but that would require changes to \"gitweb: Prepare for\n> > splitting gitweb\" patch (but it is doable).  Is there wider interest\n> > in supporting such feature?\n> \n> If you are referring to my suggestion, I see no reason to wait on\n> that.  The lib/ dir can be made configurable later.\n\nYou are right.  Thanks for sanity check.  Though I'd prefer that we get\nit right from the first time.\n\nFor the record, the changes to support configurable gitweblibdir are:\n\ngitweb/Makefile:\n----------------\n\n   gitweblibdir_SQ = $(subst ','\\'',$(gitwebdir)/lib)#'\n\n to\n\n   gitweblibdir ?= $(gitwebdir)/lib\n   ...\n   gitweblibdir_SQ = $(subst ','\\'',$(gitweblibdir))#'\n\n\n   GITWEB_REPLACE = \\\n   ...\n   \t-e 's|++GITWEBLIBDIR++|$(gitweblibdir)|g' \\\n\n\ngitweb/gitweb.perl:\n-------------------\n\n   # __DIR__ is taken from Dir::Self __DIR__ fragment\n   sub __DIR__ () {\n   \tFile::Spec->rel2abs(join '', (File::Spec->splitpath(__FILE__))[0, 1]);\n   }\n   use lib __DIR__ . '/lib';\n\n to\n\n   use lib $ENV{'GITWEBLIBDIR'} || \"++GITWEBLIBDIR++\";\n\nor something like that\n\n\nt/gitweb-lib.sh:\n----------------\n\n   gitweb_run () {\n   ...\n   \tGITWEBLIBDIR=\"$GIT_BUILD_DIR/gitweb/lib\"\n   \texport GITWEBLIBDIR\n\nor the change to gitweb config in gitweb_init()\n\n> > Simplest solution is to use $cgi->self_url() (note that what J.H. v8\n> > uses, i.e.: \"$my_url?\". $ENV{'QUERY_STRING'}, where $my_url is just\n> > $cgi->url() is not enough - it doesn't take path_info into account).\n> >\n> > Alternate solution, which I used in my rewrite, is to come up with\n> > \"canonical\" URL, e.g. href(-replay => 1, -full => 1, -path_info => 0);\n> > with this solution using path_info vs query parameters or reordering\n> > query parameters still gives the same key.\n> \n> It is easy to miss dependencies on parts of the URL that are being\n> fuzzed out.  For example, the <base href...> tag is only inserted with\n> path_info.  Maybe it would be less risky to first use self_url(), then\n> canonicalize it in a separate patch?\n\nYou are right; it is only in the latest version of my rewrite that\nI remembered that <base href=...> must be added also when caching is\nenabled (c.f. using always \"text/html\" content type when caching is\nenabled).\n \n> > J.H. patches up and including v7, and my rewrite up and including v6,\n> > excluded error pages from caching.  I think that the original resoning\n> > behind choosing to do it this way was that A.), each of specific error\n> > pages is usually accessed only once, so caching them would only take up\n> > space bloating cache, but what is more important B.) that you can't\n> > cache errors from caching engine.\n> \n> Perhaps there is a user experience reason?  If I receive an error page\n> due to a problem with my repository, all else being equal, I would\n> prefer that the next time I reload it is fixed.  By comparison, having\n> to reload multiple times to forget an obsolete non-error response\n> would be less aggravating and perhaps expected.\n\nTrue, there are errors that are transient (e.g. load too high), there are\nerrors that need webmaster / admin intervention (e.g. no disk space, or\ncache permissions problem), and there are errors that are not fixable\n(e.g. page not found).\n \n> But the benefit from caching e.g. a response from a broken link would\n> outweigh that.\n\nThe problem is _only_ if there is progress info indicator for set.\n\nThe issue you mentioned might be solved if we are able to set cache\nexpiration time individually (perhaps with the hint from within gitweb),\ne.g. by using the fact that we cache HTTP response which can include\n'Expires:' or 'Cache-Control: max-age=' header... or by manipulating\nmtime.\n\n> > Second is if there is no stale data to serve (or data is too stale), but\n> > we have progress indicator.  In this case the foreground process is\n> > responsible for rendering progress indicator, and background process is\n> > responsible for generating data.  In this case foreground process waits\n> > for data to be generated (unless progress info subroutine exits), so\n> > strictly spaking we don't need to detach background process in this\n> > case.\n> \n> What happens when the client gets tired of waiting and closes the\n> connection?\n\nHmmm... I don't know what happens to worker in non-persistent (CGI),\nwrapped persistent (mod_perl via ModPerl::Registry, PSGI via gitweb.psgi)\nand persistent (FastCGI) environments.  Well, detaching also in the case\nof background generation because of progress_info wouldn't hurt, and\ncould help...\n\n\nThanks again for your comments.\n\nHoping to hear from J,H. soon...\n-- \nJakub Narebski\nPoland\n"}]}