From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 80178 invoked by alias); 22 Nov 2018 17:17:19 -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 76711 invoked by uid 89); 22 Nov 2018 17:17:12 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 22 Nov 2018 17:17:08 +0000 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 28A834E93F; Thu, 22 Nov 2018 17:17:07 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 23B71604CD; Thu, 22 Nov 2018 17:17:05 +0000 (UTC) Subject: Re: [PATCH] Use std::vector for displaced_step_inferior_states To: Simon Marchi References: <20181122031229.15621-1-simon.marchi@ericsson.com> <68730078c8a3a37d65ad3046348ccbc6@polymtl.ca> Cc: Simon Marchi , gdb-patches@sourceware.org, dblaikie@gmail.com From: Pedro Alves Message-ID: <1f966d70-b48d-35a9-9841-8d580371c710@redhat.com> Date: Thu, 22 Nov 2018 17:17:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <68730078c8a3a37d65ad3046348ccbc6@polymtl.ca> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-SW-Source: 2018-11/txt/msg00375.txt.bz2 On 11/22/2018 05:05 PM, Simon Marchi wrote: > 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. Yeah, I think the original motivation for the registry is for when you want dynamic registration, say because the resource in question is managed by a source file that isn't always included in the build, like some foocpu-tdep.c file. For code that is always included in the build, I think that the registry obfuscates more than it helps. E.g., it makes debugging GDB harder. And it also doesn't have any benefit memory-wise. > > 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? I think that would be better, yeah. Either pointer or object (and moving the struct to some header), both are fine with me. >>> @@ -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. Thanks, Pedro Alves