From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail8.parnet.fi (mail8.parnet.fi [77.234.108.134]) by sourceware.org (Postfix) with ESMTPS id 091C238582AD for ; Tue, 6 Sep 2022 09:39:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 091C238582AD Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=martin.st Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=martin.st Received: from mail9.parnet.fi (mail9.parnet.fi [77.234.108.21]) by mail8.parnet.fi with ESMTP id 2869dq8e019085-2869dq8f019085; Tue, 6 Sep 2022 12:39:52 +0300 Received: from foo.martin.st (host-97-187.parnet.fi [77.234.97.187]) by mail9.parnet.fi (Postfix) with ESMTPS id C6909A1467; Tue, 6 Sep 2022 12:39:51 +0300 (EEST) Date: Tue, 6 Sep 2022 12:39:50 +0300 (EEST) From: =?ISO-8859-15?Q?Martin_Storsj=F6?= To: Nick Clifton cc: binutils@sourceware.org Subject: Re: [PATCH] ld: pe: Improve performance of object file exclude symbol directives In-Reply-To: <174963e9-fa71-b70e-7659-0f534db073f8@redhat.com> Message-ID: <10615ade-446f-a711-ee6c-789a9d95ee57@martin.st> References: <20220902105903.2249507-1-martin@martin.st> <174963e9-fa71-b70e-7659-0f534db073f8@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed X-FE-Policy-ID: 3:14:2:SYSTEM X-Spam-Status: No, score=-3.5 required=5.0 tests=BAYES_00,JMQ_SPF_NEUTRAL,KAM_DMARC_STATUS,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Mon, 5 Sep 2022, Nick Clifton wrote: > Hi Martin, > >> Store the list of excluded symbols in a sorted list, speeding up >> checking for duplicates when inserting new entries. > > An excellent idea. FWIW, regarding most of this review here: This file already has got two copies of the exact same routines for inserting structs into a sorted list of structs (find_export_in_list, def_file_add_export, find_import_in_list, def_file_add_import), so for this patch I just made a third copy of the same routines, with the exact same behaviours, but adapted for the exclude_symbol structs. I'd be happy to adjust things based on your review, but then I'll probably follow up with a separate patch to adapt the other existing implementations based on this feedback. > >> /* From EXCLUDE_SYMBOLS or embedded directives. */ >> + int num_exclude_symbols; > > Presumably there can never be a negative number of excluded symbols, > so using "unsigned int" here might be more appropriate. Sure, I can change that. > > >> + if (fdef->exclude_symbols) > > Given that the for() loop to follow checks fdef->num_exclude_symbols, > this if() statement is redundant. Sure, I can remove it. (The existing exports/imports lists of structs have the exact same structure.) >> + for (i = 0; i < fdef->num_exclude_symbols; i++) >> + free (fdef->exclude_symbols[i].symbol_name); >> + free (fdef->exclude_symbols); > > Calling free(NULL) is allowed, so the if() statement is still not needed. Sure. (Also preexisting from exports/imports.) >> +/* Search the position of the identical element, or returns the position >> + of the next higher element. If last valid element is smaller, then MAX >> + is returned. */ > > The comment should explain the use of the IS_IDENT parameter. > > It should probably also note that MAX is expected to be the same as the > number > of entries in the B array. Ok, I can expand the comment to explain these. >> +static int >> +find_exclude_in_list (def_file_exclude_symbol *b, int max, >> + const char *name, int *is_ident) > > Since we are dealing with array indicies here, I would suggest that the > function > should return an unsigned int. Likewise the MAX parameter should be > unsigned. > Plus is_ident should be a boolean pointer, not an integer pointer. Ok, I can change that. (Ditto for the preexisting functions.) > >> +static def_file_exclude_symbol * >> +def_file_add_exclude_symbol (def_file *fdef, const char *name, int >> *is_dup) > > I think that IS_DUP should be a "boolean *" not a "int *". Ok > >> + int max_exclude_symbols = ROUND_UP(fdef->num_exclude_symbols, 32); > > Magic numbers are bad. Please replace 32 with a defined constant. > >> + if (fdef->num_exclude_symbols >= max_exclude_symbols) > > This is sneaky and not very intuitive. I would prefer it if you had > two fields in the fdef structure, one for the number of allocated > slots in the exclude_symbols array and one for the number of used slots. > > Then testing for when the array needs to be extended would be a case > of comparing these two fields. Using two fields might take up more > space in the structure, but it sure makes a lot more sense to me. This was also designed according to the existing code - but yes, I agree that using a separate variable keeping track of the allocation is much safer and clearer. (It took a while to think through to see how this works for the initial allocation and all that.) >> + max_exclude_symbols = ROUND_UP(fdef->num_exclude_symbols + 1, 32); > > Given that the point of this patch is to improve performance when there > are a large number of excluded symbols, incrementing the array by 32 slots > at a time seems counter intuitive. I would suggest a bigger number, eg 1024 > or 10240. Sure, or doubling as Jan suggested. >> + e = fdef->exclude_symbols + pos; >> + if (pos != fdef->num_exclude_symbols) >> + memmove (&e[1], e, (sizeof (def_file_exclude_symbol) * >> (fdef->num_exclude_symbols - pos))); > > What the ? OK, so what is going on here ? At the very least I think that > you are going to need to add a comment to this piece of code. Sure. (This is also preexisting code for the other lists.) As this inserts into a sorted list of structs, it moves the later structs forward when inserting in the middle of the list. // Martin