{"thread":{"id":"29710","subject":"[PATCHv3 0/3] gitweb: Faster project search","startedAt":"2012-02-23T15:54:45Z","lastAt":"2012-02-23T15:54:48Z","messageCount":4,"participants":["Jakub Narebski"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"185278","messageId":"1330012488-7970-1-git-send-email-jnareb@gmail.com","threadId":"29710","inReplyTo":null,"subject":"[PATCHv3 0/3] gitweb: Faster project search","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-23T15:54:45Z","receivedAt":"2012-02-23T15:54:45Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"[Cc-ing Junio because of his involvement in discussion]\n\nThese patches are separated from first part of previous version of\nthis series\n\n  \"[PATCHv2 0/8] gitweb: Faster and improved project search\"\n  http://thread.gmane.org/gmane.comp.version-control.git/190852\n\nIt is meant to replace 'jn/gitweb-search-optim' in pu\n\nThe intent of this series is to make project search faster by reducing\nnumber of git commands (and I/O).  This is done by first searching\nprojects, then filling all project info, rather than the reverse.\n\n\nJakub Narebski (3):\n  gitweb: Refactor checking if part of project info need filling\n  gitweb: Option for filling only specified info in\n    fill_project_list_info\n  gitweb: Faster project search\n\n gitweb/gitweb.perl |   66 +++++++++++++++++++++++++++++++++++++++++----------\n 1 files changed, 53 insertions(+), 13 deletions(-)\n\n-- \n1.7.9\n"},{"id":"185279","messageId":"1330012488-7970-2-git-send-email-jnareb@gmail.com","threadId":"29710","inReplyTo":"1330012488-7970-1-git-send-email-jnareb@gmail.com","subject":"[PATCHv3 1/3] gitweb: Refactor checking if part of project info need filling","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-23T15:54:46Z","receivedAt":"2012-02-23T15:54:46Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Extract the check if given keys (given parts) of project info needs to\nbe filled into project_info_needs_filling() subroutine.  It is for now\na thin wrapper around \"!exists $project_info->{$key}\".\n\nNote that !defined was replaced by more correct !exists.\n\nWhile at it uniquify treating of all project info, adding checks for\n'age' field before running git_get_last_activity(), and also checking\nfor all keys filled in code protected by conditional, and not only\none.\n\nThe code now looks like this\n\n  foreach my $project (@$project_list) {\n  \tif (given keys need to be filled) {\n  \t\tfill given keys\n  \t}\n  \t...\n  }\n\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\nThis patch appeared for first time in v2 of this series, and split off\nintroduction of project_info_needs_filling() function from partial\nfilling via @wanted_keys... which in this version of series is not\ndone in project_info_needs_filling() anyway.\n\nAlmost no changes from v2:\n* Remove duplication of empty line between project_info_needs_filling()\n  and fill_project_list_info()\n\n gitweb/gitweb.perl |   34 ++++++++++++++++++++++++++--------\n 1 files changed, 26 insertions(+), 8 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 057ba5b..cdd84c7 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -5195,6 +5195,20 @@ sub git_project_search_form {\n \tprint \"</div>\\n\";\n }\n \n+# entry for given @keys needs filling if at least one of keys in list\n+# is not present in %$project_info\n+sub project_info_needs_filling {\n+\tmy ($project_info, @keys) = @_;\n+\n+\t# return List::MoreUtils::any { !exists $project_info->{$_} } @keys;\n+\tforeach my $key (@keys) {\n+\t\tif (!exists $project_info->{$key}) {\n+\t\t\treturn 1;\n+\t\t}\n+\t}\n+\treturn;\n+}\n+\n # fills project list info (age, description, owner, category, forks)\n # for each project in the list, removing invalid projects from\n # returned list\n@@ -5206,24 +5220,28 @@ sub fill_project_list_info {\n \tmy $show_ctags = gitweb_check_feature('ctags');\n  PROJECT:\n \tforeach my $pr (@$projlist) {\n-\t\tmy (@activity) = git_get_last_activity($pr->{'path'});\n-\t\tunless (@activity) {\n-\t\t\tnext PROJECT;\n+\t\tif (project_info_needs_filling($pr, 'age', 'age_string')) {\n+\t\t\tmy (@activity) = git_get_last_activity($pr->{'path'});\n+\t\t\tunless (@activity) {\n+\t\t\t\tnext PROJECT;\n+\t\t\t}\n+\t\t\t($pr->{'age'}, $pr->{'age_string'}) = @activity;\n \t\t}\n-\t\t($pr->{'age'}, $pr->{'age_string'}) = @activity;\n-\t\tif (!defined $pr->{'descr'}) {\n+\t\tif (project_info_needs_filling($pr, 'descr', 'descr_long')) {\n \t\t\tmy $descr = git_get_project_description($pr->{'path'}) || \"\";\n \t\t\t$descr = to_utf8($descr);\n \t\t\t$pr->{'descr_long'} = $descr;\n \t\t\t$pr->{'descr'} = chop_str($descr, $projects_list_description_width, 5);\n \t\t}\n-\t\tif (!defined $pr->{'owner'}) {\n+\t\tif (project_info_needs_filling($pr, 'owner')) {\n \t\t\t$pr->{'owner'} = git_get_project_owner(\"$pr->{'path'}\") || \"\";\n \t\t}\n-\t\tif ($show_ctags) {\n+\t\tif ($show_ctags &&\n+\t\t    project_info_needs_filling($pr, 'ctags')) {\n \t\t\t$pr->{'ctags'} = git_get_project_ctags($pr->{'path'});\n \t\t}\n-\t\tif ($projects_list_group_categories && !defined $pr->{'category'}) {\n+\t\tif ($projects_list_group_categories &&\n+\t\t    project_info_needs_filling($pr, 'category')) {\n \t\t\tmy $cat = git_get_project_category($pr->{'path'}) ||\n \t\t\t                                   $project_list_default_category;\n \t\t\t$pr->{'category'} = to_utf8($cat);\n-- \n1.7.9\n"},{"id":"185280","messageId":"1330012488-7970-3-git-send-email-jnareb@gmail.com","threadId":"29710","inReplyTo":"1330012488-7970-1-git-send-email-jnareb@gmail.com","subject":"[PATCHv3 2/3] gitweb: Option for filling only specified info in fill_project_list_info","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-23T15:54:47Z","receivedAt":"2012-02-23T15:54:47Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Enhance fill_project_list_info() subroutine to accept optional\nparameters that specify which fields in project information needs to\nbe filled.  If none are specified then fill_project_list_info()\nbehaves as it used to, and ensure that all project info is filled.\n\nThis is in preparation of future lazy filling of project info in\nproject search and pagination of sorted list of projects.\n\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\nThis could have been squashed with the next commit, but this way it is\npure refactoring that shouldn't change gitweb behavior, and hopefully\nmakes this patch easier to review.\n\nChanges from v2:\n* Instead of overloading project_info_needs_filling() to do both\n  handling of which fields need to be filled, and which fields\n  we want to have filled, just make its caller filter its arguments.\n\n  This means that in this version of patch there are no changes to\n  project_info_needs_filling() subroutine.\n\n# Instead of handling both cases (the one with set of wanted keys,\n  and the one where we want to fill all remaining info) in a single\n  filtering subroutine, use two different anonymous subroutines\n  to filter or not filter keys.\n\n* Expanded comment about fill_project_list_info to include detailed\n  description of when it does remove projects from returned list (and\n  why it is), and examples for both types of usage of this subroutine.\n\n* Rewritten major part of commit message, which in v2 was not updated\n  to reflect new development, and referred to behavior in v1.\n\n[Should I include changes from v1 here, in the future?]\n\n\nOn Mon, 20 Feb 2012, Junio C Hamano wrote\nin http://thread.gmane.org/gmane.comp.version-control.git/190852/focus=191046\nJH>\nJH> I must say that the approach to put the set filtering logic inside\nJH> project_info_needs_filling function smells like a bad API design.\nJH>\nJH> In other words, wouldn't a callchain that uses a more natural set of API\nJH> functions be like this?\nJH> \nJH>       # the top-level caller that is interested only in these two fields\nJH>       fill_project_list_info($projlist, 'age', 'owner');\nJH> \nJH>       # in fill_project_list_info()\nJH>       my $projlist = shift;\nJH>       my %wanted = map { $_ => 1 } @_;\nJH> \nJH>       foreach my $pr (@$projlist) {\nJH>               if (project_info_needs_filling($pr,\nJH>                       filter_set(\\%wanted, 'age', 'age_string'))) {\n\nI think this might be an even better solution, instead of handling all\nkeys / selected keys in filter_set(), provide different $filter_set\nsubroutines:\n\n        foreach my $pr (@$projlist) {\n                  if (project_info_needs_filling($pr,\n                          $filter_set->('age', 'age_string'))) {\n\n\n gitweb/gitweb.perl |   33 +++++++++++++++++++++++++--------\n 1 files changed, 25 insertions(+), 8 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex cdd84c7..7fb7a55 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -5209,39 +5209,56 @@ sub project_info_needs_filling {\n \treturn;\n }\n \n-# fills project list info (age, description, owner, category, forks)\n+# fills project list info (age, description, owner, category, forks, etc.)\n # for each project in the list, removing invalid projects from\n-# returned list\n+# returned list, or fill only specified info.\n+#\n+# Invalid projects are removed from the returned list if and only if you\n+# ask 'age' or 'age_string' to be filled, because they are the only fields\n+# that run unconditionally git command that requires repository, and\n+# therefore do always check if project repository is invalid.\n+#\n+# USAGE:\n+# * fill_project_list_info(\\@project_list, 'descr_long', 'ctags')\n+#   ensures that 'descr_long' and 'ctags' fields are filled\n+# * @project_list = fill_project_list_info(\\@project_list)\n+#   ensures that all fields are filled (and invalid projects removed)\n+#\n # NOTE: modifies $projlist, but does not remove entries from it\n sub fill_project_list_info {\n-\tmy $projlist = shift;\n+\tmy ($projlist, @wanted_keys) = @_;\n \tmy @projects;\n+\tmy $filter_set = sub { return @_; };\n+\tif (@wanted_keys) {\n+\t\tmy %wanted_keys = map { $_ => 1 } @wanted_keys;\n+\t\t$filter_set = sub { return grep { $wanted_keys{$_} } @_; };\n+\t}\n \n \tmy $show_ctags = gitweb_check_feature('ctags');\n  PROJECT:\n \tforeach my $pr (@$projlist) {\n-\t\tif (project_info_needs_filling($pr, 'age', 'age_string')) {\n+\t\tif (project_info_needs_filling($pr, $filter_set->('age', 'age_string'))) {\n \t\t\tmy (@activity) = git_get_last_activity($pr->{'path'});\n \t\t\tunless (@activity) {\n \t\t\t\tnext PROJECT;\n \t\t\t}\n \t\t\t($pr->{'age'}, $pr->{'age_string'}) = @activity;\n \t\t}\n-\t\tif (project_info_needs_filling($pr, 'descr', 'descr_long')) {\n+\t\tif (project_info_needs_filling($pr, $filter_set->('descr', 'descr_long'))) {\n \t\t\tmy $descr = git_get_project_description($pr->{'path'}) || \"\";\n \t\t\t$descr = to_utf8($descr);\n \t\t\t$pr->{'descr_long'} = $descr;\n \t\t\t$pr->{'descr'} = chop_str($descr, $projects_list_description_width, 5);\n \t\t}\n-\t\tif (project_info_needs_filling($pr, 'owner')) {\n+\t\tif (project_info_needs_filling($pr, $filter_set->('owner'))) {\n \t\t\t$pr->{'owner'} = git_get_project_owner(\"$pr->{'path'}\") || \"\";\n \t\t}\n \t\tif ($show_ctags &&\n-\t\t    project_info_needs_filling($pr, 'ctags')) {\n+\t\t    project_info_needs_filling($pr, $filter_set->('ctags'))) {\n \t\t\t$pr->{'ctags'} = git_get_project_ctags($pr->{'path'});\n \t\t}\n \t\tif ($projects_list_group_categories &&\n-\t\t    project_info_needs_filling($pr, 'category')) {\n+\t\t    project_info_needs_filling($pr, $filter_set->('category'))) {\n \t\t\tmy $cat = git_get_project_category($pr->{'path'}) ||\n \t\t\t                                   $project_list_default_category;\n \t\t\t$pr->{'category'} = to_utf8($cat);\n-- \n1.7.9\n"},{"id":"185281","messageId":"1330012488-7970-4-git-send-email-jnareb@gmail.com","threadId":"29710","inReplyTo":"1330012488-7970-1-git-send-email-jnareb@gmail.com","subject":"[PATCHv3 3/3] gitweb: Faster project search","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-23T15:54:48Z","receivedAt":"2012-02-23T15:54:48Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Before searching by some field the information we search for must be\nfilled in, but we do not have to fill other fields that are not\ninvolved in the search.\n\nTo be able to request filling only specified fields,\nfill_project_list_info() was enhanced in previous commit to take\nadditional parameters which specify part of projects info to fill.\nThis way we can limit doing expensive calculations (like running\ngit-for-each-ref to get 'age' / \"Last changed\" info) to doing those\nonly for projects which we will show as search results.\n\nThis commit actually uses this interface, changing gitweb code from\nthe following behavior\n\n  fill all project info on all projects\n  search projects\n\nto behaving like this pseudocode\n\n  fill search fields on all projects\n  search projects\n  fill all project info on search results\n\n\nWith this commit the number of git commands used to generate search\nresults is 2*<matched projects> + 1, and depends on number of matched\nprojects rather than number of all projects (all repositories).\n\nNote: this is 'git for-each-ref' to find last activity, and 'git config'\nfor each project, and 'git --version' once.\n\n\nExample performance improvements, for search that selects 2\nrepositories out of 12 in total:\n\n* Before (warm cache):\n  \"This page took 0.867151 seconds  and 27 git commands to generate.\"\n\n* After (warm cache):\n  \"This page took 0.673643 seconds  and 5 git commands to generate.\"\n\nNow imagine that they are 5 repositories out of 5000, and cold or\ntrashed cache case.\n\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\nChanges from v2 (v2 is the same as v1):\n* Rewritten and extended commit message (though extending perhaps have\n  gone too far...).\n\n  Added paragraphs are \"This commit actually uses this interface...\"\n  and \"Example performance improvements...\"\n\n\nJunio C Hamano wrote in\nhttp://thread.gmane.org/gmane.comp.version-control.git/190852/focus=191053\n[...]\nJC> \"must be filled in.\" is correct, but that happens without the previous\nJC> patch.  The first sentence must also say:\nJC> \nJC>         In order to search by some field, the information we look for must\nJC>         be filled in, but we do not have to fill other fields that are not\nJC>         involved in the search.\nJC> \nJC> to justify the previous \"fill_project_list_info can be asked to return\nJC> without getting unused fields\" patch.  \n\nDone.  Thanks for a suggestion.\n\nJC> The rest of the log message then makes good sense.\n\nOoops...\n\n gitweb/gitweb.perl |    9 +++++++--\n 1 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 7fb7a55..4ceb1a6 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -2987,6 +2987,10 @@ sub search_projects_list {\n \treturn @$projlist\n \t\tunless ($tagfilter || $searchtext);\n \n+\t# searching projects require filling to be run before it;\n+\tfill_project_list_info($projlist,\n+\t                       $tagfilter  ? 'ctags' : (),\n+\t                       $searchtext ? ('path', 'descr') : ());\n \tmy @projects;\n  PROJECT:\n \tforeach my $pr (@$projlist) {\n@@ -5394,12 +5398,13 @@ sub git_project_list_body {\n \t# filtering out forks before filling info allows to do less work\n \t@projects = filter_forks_from_projects_list(\\@projects)\n \t\tif ($check_forks);\n-\t@projects = fill_project_list_info(\\@projects);\n-\t# searching projects require filling to be run before it\n+\t# search_projects_list pre-fills required info\n \t@projects = search_projects_list(\\@projects,\n \t                                 'searchtext' => $searchtext,\n \t                                 'tagfilter'  => $tagfilter)\n \t\tif ($tagfilter || $searchtext);\n+\t# fill the rest\n+\t@projects = fill_project_list_info(\\@projects);\n \n \t$order ||= $default_projects_order;\n \t$from = 0 unless defined $from;\n-- \n1.7.9\n"}]}