From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x429.google.com (mail-wr1-x429.google.com [IPv6:2a00:1450:4864:20::429]) by sourceware.org (Postfix) with ESMTPS id 8616A3858D28 for ; Thu, 16 Dec 2021 16:07:27 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 8616A3858D28 Received: by mail-wr1-x429.google.com with SMTP id e5so11657534wrc.5 for ; Thu, 16 Dec 2021 08:07:27 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=YfzkS2mMxD7i5CcKgfQJVkWMjvcL4jg89+iX+B8uF4I=; b=WtO/7Q/QYLgn7mnjPZpknhfrXV6AmAMdcoWRUtGUbphH9Lnq1PVMJVYYbT5Az9qKM0 CmGRCiLxnphaaIOifH7JIMu0/AdUNvXumdm3BESKRQ2/2OC23q7iSm8eO7RTcMArrGUD z6ZCFJZnt03xMhgajW1kjv3M2Kz5IliudCYPdwaCcB20340kBTesbBTXCZGK013OGc87 Iu519Qz5ZfJ8rnAAAUZcLZsFlILOa7QW2JqJomaAtZtUWQZlDbsqYxISAwk6hLdGq/5z fiKRe8lbikuatK3o3T6PV0jbZcJgQnd2xDZ5JwojKUUPdW/CMMtjQvgT81Oee+Wwzfg9 nnsQ== X-Gm-Message-State: AOAM53380LL5TBGtaB/72oZ9gscp4CHxcne1pgAgOc3IL+sHg5HbGJnp vdriv+XpkFtxV7nzV1AviglHld8oqabFUg== X-Google-Smtp-Source: ABdhPJyiZg9sd1sXI9DeE80ACtDbKIzs2kL8WkRVkJqjwr3N9ubzsNM1esF4PB8tcGlKey8yBkUvbw== X-Received: by 2002:a5d:58c5:: with SMTP id o5mr9208507wrf.15.1639670846187; Thu, 16 Dec 2021 08:07:26 -0800 (PST) Received: from google.com ([2a00:79e0:d:210:f9d6:dc00:bafe:ca6e]) by smtp.gmail.com with ESMTPSA id u13sm9218666wmq.14.2021.12.16.08.07.23 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 16 Dec 2021 08:07:24 -0800 (PST) Date: Thu, 16 Dec 2021 16:07:23 +0000 From: Matthias Maennich To: Dodji Seketeli Cc: libabigail@sourceware.org, gprocida@google.com, kernel-team@android.com Subject: Re: [PATCH 3/5] XML writer: track emitted types by bare pointer Message-ID: References: <20211203114622.2944173-1-maennich@google.com> <20211203114622.2944173-4-maennich@google.com> <87ilvwrawm.fsf@seketeli.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <87ilvwrawm.fsf@seketeli.org> X-Spam-Status: No, score=-22.3 required=5.0 tests=BAYES_00, DKIMWL_WL_MED, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, ENV_AND_HDR_SPF_MATCH, FSL_HELO_FAKE, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, USER_IN_DEF_DKIM_WL, USER_IN_DEF_SPF_WL autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: libabigail@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Mailing list of the Libabigail project List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 16 Dec 2021 16:07:29 -0000 Hi Dodji! Thanks for the review and thanks for your thoughts! On Fri, Dec 10, 2021 at 11:50:49AM +0100, Dodji Seketeli wrote: >Hello, > >Matthias Maennich a écrit: > >[...] > >> This is a performance and safety improvement made possible by the >> previous changes which ensure that the same pointers are inserted and >> looked up. >> >> This essentially removes the now unnecessary deep comparison. > >[...] > >> +++ b/src/abg-writer.cc >> @@ -123,14 +123,10 @@ typedef unordered_map> abigail::diff_utils::deep_ptr_eq_functor> type_ptr_map; >> >> // A convenience typedef for a set of type_base*. >> -typedef unordered_set> - abigail::diff_utils::deep_ptr_eq_functor> >> -type_ptr_set_type; >> +typedef std::unordered_set type_ptr_set_type; >> >> /// A convenience typedef for a set of function type*. >> -typedef unordered_set> - abigail::diff_utils::deep_ptr_eq_functor> >> -fn_type_ptr_set_type; >> +typedef std::unordered_set fn_type_ptr_set_type; > >The problem I see with doing this is that it's possible that two >declaration-only classes, that are equivalent but that have different >pointer values get into these sets. > >In that case, they would be considered different even though they are >not. If the XML writer considers two equivalent declaration-only types to be different, one question to ask is: what is the real difference, that is, how will this affect the outcome of abidiff? If the types never change (kind, name or declaration/definition status), nothing should ever be reported. If a type does change... there are two possibilities: either the types were really one type and now perhaps abidiff reports diffs for the same name in two different ways; or the types were really two different ones and abidiff has a simpler job. In my experience, abidiff doesn't always report declaration-only/defined transitions. It doesn't sound like there will be any really bad impact on diffs from having this kind of duplication. However, if someone can come up with a test case of the kind you mention, that would give some extra reassurance. > >So maybe it would be better have an equality operator that uses >is_non_canonicalized_type() to detect those rare cases and use >structural comparison in those cases? That might come at higher cost than it is beneficial. > >What do you think? For us specifically - building with clang and for our use cases - if we keep structural equality of any kind then we need a hash function to go along with this and, as we've sadly found out, this isn't working well at the moment. We are currently on a bit dated version of libabigail for our production use, but would like to close that gap again to come closer to master. The risk of infinite loops and the reality of 30x slowdowns for certain workloads mean we would need to apply these changes to remove structural equality testing from the XML writer and then maintain an Android version of libabigail as a more heavily-patched fork, to whatever extent is feasible. I would rather we find a good solution that works for all to get again close to upstream and not having to maintain such a fork. Yet, as an additional piece of assurance: the testing we have done does not only include kernels, but of course we heavily examined the libabigail test suite. Additionally, we maintain a large set of small test cases specifically created for ABI stability testing and to cover corner cases of all sorts. We are in the process of publishing those as well. So far, this has served as great input for this patch series as well. Does this make sense? What do you think? Cheers, Matthias > >-- > Dodji