From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS53758 23.128.96.0/24 X-Spam-Status: No, score=-3.9 required=3.0 tests=AWL,BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI, SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by dcvr.yhbt.net (Postfix) with ESMTP id 1FBE41F8C6 for ; Thu, 2 Sep 2021 23:04:42 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1347962AbhIBXFh (ORCPT ); Thu, 2 Sep 2021 19:05:37 -0400 Received: from cloud.peff.net ([104.130.231.41]:38406 "EHLO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S244682AbhIBXFg (ORCPT ); Thu, 2 Sep 2021 19:05:36 -0400 Received: (qmail 25277 invoked by uid 109); 2 Sep 2021 23:04:37 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Thu, 02 Sep 2021 23:04:37 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 1863 invoked by uid 111); 2 Sep 2021 23:04:38 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Thu, 02 Sep 2021 19:04:38 -0400 Authentication-Results: peff.net; auth=none Date: Thu, 2 Sep 2021 19:04:36 -0400 From: Jeff King To: Taylor Blau Cc: git@vger.kernel.org Subject: Re: [PATCH 2/3] builtin/pack-objects.c: simplify add_objects_in_unpacked_packs() Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org On Sun, Aug 29, 2021 at 10:48:54PM -0400, Taylor Blau wrote: > +static int add_object_in_unpacked_pack(const struct object_id *oid, > + struct packed_git *pack, > + uint32_t pos, > + void *_data) > { > [...] > + struct object *obj = lookup_unknown_object(the_repository, oid); > + if (obj->flags & OBJECT_ADDED) > + return 0; > + add_object_entry(oid, obj->type, "", 0); > + obj->flags |= OBJECT_ADDED; > + return 0; > } This is not new in your patch series, but while merging this with another topic I had, I noticed another opportunity for optimization here. We already know the pack/pos in which we found the object. But when we call add_object_entry(), it will do another lookup, via want_object_in_pack(). We could shortcut that by providing it with the extra information, the way add_object_entry_from_bitmap() does. The original code before your series could have done the same optimization, but it became much more obvious after your series, since -Wunused-parameters notices that we do not look at the "pack" or "pos" parameters at all. :) It may not be that exciting an optimization, though. Pack lookups aren't _that_ expensive, and the pack-mru code would mean we always find it in the first pack (since by definition we're iterating through the objects in whole packs, our locality is perfect). It would also probably involve some slight refactoring of add_object_entry() to avoid duplication (though possibly the result could reduce similar duplication with the bitmap variant). Hmm, actually looking further, we already have add_object_entry_from_pack() for --stdin-packs. So I offer it mainly as an observation, in case somebody wants to look into it further (both for the optimization and the possibility of simplifying the code). -Peff