From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 54643 invoked by alias); 22 Nov 2018 17:05:25 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 54623 invoked by uid 89); 22 Nov 2018 17:05:24 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy=H*F:D*ca, H*u:1.3.6, H*UA:1.3.6 X-HELO: smtp.polymtl.ca Received: from smtp.polymtl.ca (HELO smtp.polymtl.ca) (132.207.4.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 22 Nov 2018 17:05:23 +0000 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id wAMH5Gmx025835 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Thu, 22 Nov 2018 12:05:21 -0500 Received: by simark.ca (Postfix, from userid 112) id 743321E996; Thu, 22 Nov 2018 12:05:16 -0500 (EST) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id 0D44E1E477; Thu, 22 Nov 2018 12:05:13 -0500 (EST) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Thu, 22 Nov 2018 17:05:00 -0000 From: Simon Marchi To: Pedro Alves Cc: Simon Marchi , gdb-patches@sourceware.org, dblaikie@gmail.com Subject: Re: [PATCH] Use std::vector for displaced_step_inferior_states In-Reply-To: References: <20181122031229.15621-1-simon.marchi@ericsson.com> Message-ID: <68730078c8a3a37d65ad3046348ccbc6@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.3.6 X-IsSubscribed: yes X-SW-Source: 2018-11/txt/msg00374.txt.bz2 On 2018-11-22 10:32, Pedro Alves wrote: > On 11/22/2018 03:12 AM, Simon Marchi wrote: >> Commit >> >> 39a36629f68e ("Use std::forward_list for >> displaced_step_inferior_states") >> >> changed a hand-made linked list to use std::forward_list of pointers. >> As suggested by David Blaikie, we might as well use values instead of >> pointers. And instead of a list, we might as well use a vector. The >> size of this list will always be at most the number of inferiors, >> typically very small. And in any case the operation we do in the >> hottest path (doing a displaced step) is iterate, and iterating on a >> vector is always faster than a linked list. >> >> A consequence of using a vector is that objects can be moved, when the >> vector is resized. I don't think this is a problem, because we don't >> save the address of the objects. In displaced_step_prepare_throw, we >> save a pointer to the step_saved_copy field in a cleanup, but it is >> ran >> or discarded immediately after. > > Another alternative would be to put the displaced_step_inferior_state > object in struct inferior directly instead of keeping the objects > on the side. In practice, on x86 GNU/Linux at least, you end > up with an object per inferior anyway, assuming we actually > run the inferiors, which sounds like a good assumption. It didn't > use to be the case originally, since back then displaced stepping > was a new thing that wasn't on by default. Ok, I was wondering about that too. I assumed that it was simply to avoid stuffing too much random stuff in the inferior struct. I also thought about how other files use a registry for things like this. I did a quick test of having a pointer to displaced_step_inferior_state in the inferior structure (the implementation of displaced_step_inferior_state stays in infrun.c), it seems to work well. Would you prefer that? >> @@ -1484,36 +1484,40 @@ >> displaced_step_closure::~displaced_step_closure () = default; >> /* Per-inferior displaced stepping state. */ >> struct displaced_step_inferior_state >> { >> + displaced_step_inferior_state (inferior *inf) >> + : inf (inf) >> + {} > > explicit. > >> + >> + if (it != displaced_step_inferior_states.end ()) >> + displaced_step_inferior_states.erase (it); > > I think this could be unordered_remove. Thanks, I'll fix those two if we end up merging this patch. Simon