From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 67588 invoked by alias); 29 Apr 2015 22:44:57 -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 67572 invoked by uid 89); 29 Apr 2015 22:44:57 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mail-ig0-f202.google.com Received: from mail-ig0-f202.google.com (HELO mail-ig0-f202.google.com) (209.85.213.202) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Wed, 29 Apr 2015 22:44:55 +0000 Received: by igdh15 with SMTP id h15so4536943igd.0 for ; Wed, 29 Apr 2015 15:44:53 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:mime-version:content-type :content-transfer-encoding:message-id:date:to:cc:subject:in-reply-to :references; bh=HrBW6QlbzQjEe/WYm91zs+xZ1+BFyIUGhWIlqMNzgWs=; b=VTUWO7vDdA31lBp8uBbtDeVrEnSC17wROrCTI7R1p4wLt6UKFaHcQjd9wsMqpq/keg Jf9gSQa0wgUq7MM1XuFi6StEwICGitXGFlZ14zWp4rlLJum/qw/Z6d4+jpWDYkAJ6pgV ejedtEYWi69mRSaZG4FFjCXNRKhf9lhXHyu7RpoBjjTSviYB022sXlwFjZbiAKpqJ/ex eO6H4e/y2dsFV6xZTIr4xR2EAq1RiFxM89rguvE5Q+LRA4OqSJeO+7XWznI4STjlemV+ obBo4xxzYIsTZhI5Z9AXYCa4uUjslkTj2s+4tMS/AEXOAI4fmQmQ/nwAwsgg1BIwpx6s 3Ygw== X-Gm-Message-State: ALoCoQnTvV5WMLwWVvCFG3mmE7m5rzNh3DMFz1yRxUaNk5NW7HkDzTCYbsG6X8Mnu5cd0Q/B+HFh2npiGzHw+4plHGQ8HADuHcx/RRzBaORUZPIN8Hk2prMXgzhaRL3C72XbQA6Aujmr10n3IpUVUH4uWDMGwQ5A0BrxmtTAwoRsGWX/32o+99A= X-Received: by 10.182.87.100 with SMTP id w4mr2143897obz.20.1430347493426; Wed, 29 Apr 2015 15:44:53 -0700 (PDT) Received: from corpmail-nozzle1-1.hot.corp.google.com ([100.108.1.104]) by gmr-mx.google.com with ESMTPS id f100si22146yhp.7.2015.04.29.15.44.52 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 29 Apr 2015 15:44:53 -0700 (PDT) Received: from ruffy2.mtv.corp.google.com ([172.17.128.107]) by corpmail-nozzle1-1.hot.corp.google.com with ESMTP id 4lPr3Lek.1; Wed, 29 Apr 2015 15:44:53 -0700 From: Doug Evans MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Message-ID: <21825.24292.47719.109523@ruffy2.mtv.corp.google.com> Date: Thu, 30 Apr 2015 00:26:00 -0000 To: Keith Seitz Cc: gdb-patches Subject: Re: [PATCH 00/18] Implement full completer limiting In-Reply-To: <55412566.2020403@redhat.com> References: <20150413192235.29172.13097.stgit@valrhona.uglyboxes.com> <55412566.2020403@redhat.com> X-IsSubscribed: yes X-SW-Source: 2015-04/txt/msg01090.txt.bz2 Keith Seitz writes: > On 04/19/2015 08:21 PM, Doug Evans wrote: > > I've gone over the entire patch set. A few things I like, but there's > > at least one thing I'm concerned about. Replicating the above switch > > in each completer: IWBN to avoid such duplication. > > We should still be able to remove the global state and fix 17960. > > The original patch series (necessarily IMO) exposed this > memory-management issue, and a global maintainer approved the change. > Surely the approving maintainer realized that this code would propagate > to other completer functions eventually, no? [I certainly hope the > maintainer did not accept that the complete_line hack would be long-lived!] Just to make sure we're on the same page, which complete_line hack? > I can certainly change the series to add a function which attempts to > hide this detail. For example: > > /* Returns whether the maximum number of completions has been > reached/exceeded. */ > > int > add_completion (struct completer_data *cdata, const char *match) > { > char *match = xstrdup (name); > enum maybe_add_completion_enum add_status; > > add_status = maybe_add_completion (cdata, match); > switch (add_status) > { > case MAYBE_ADD_COMPLETION_OK: > VEC_safe_push (char_ptr, cdata->results, match); > break; > case MAYBE_ADD_COMPLETION_OK_MAX_REACHED: > VEC_safe_push (char_ptr, cdata->results, match); > return 1; > case MAYBE_ADD_COMPLETION_MAX_REACHED: > xfree (match); > return 1; > case MAYBE_ADD_COMPLETION_DUPLICATE: > xfree (match); > break; > } > > return 0; > } > > Given that completions (like symbol reading/searching) are extremely > time-consuming, one of my design goals was to not introduce anything > that might slow down completion, including multiple allocations and copying. Is there any data that says malloc/copying plays a significant role in the time? At this point the main culprit is file i/o and(/or) debug info processing / symbol table processing. I can imagine a case where the number of completions is large enough that malloc/copying becomes an issue, but I think the number of completions would have to be pretty large, far higher than any reasonable value a user might set for max-completions. [for some definition of "reasonable"] So at this point I haven't been worrying about it. We're already doing a fair bit of malloc/copying in the code that's there today, e.g., xstrdup calls in complete_line, and every user I talk to is thrilled with the feature. [Maybe in a few years when they've forgotten it used to take minutes will they start to complain about the seconds. :-)] > Unfortunately with this goal in mind, the use of this proposed function > (alone) can only be pushed into four of the fourteen functions dealing > with completions. > > The ten remaining functions would require an additional malloc/copy or > yet another API function, e.g., > add_completion_1/add_completionA/add_completion_Ex [:-)] to deal with > this case. All this to simply forgo checking the return result of > maybe_add_completion. I don't see the benefit. > > Developers already have to deal with memory management in exception > handling, which is more complex than simply checking the return result > of a function -- especially in (what I would perceive as) cut-n-paste > code like this. Any time one duplicates code one needs to consider the increased maintenance burden. Plus less code is easier to read/digest. I'd like to avoid the duplication where I can. > Or perhaps you had another idea in mind? Some completers don't really need to track the count, (e.g., even if there were 10000 signals, I doubt it'd matter if signal_completer tracked them - interesting experiment though), and the final pass performs its own limiting, so one thing I have in mind is that individual tracking by every completer is unnecessary. Another thought I had was, for individual completers that do need to track the result, to what extent can they all use the same API provided by completion tracking (e.g., have an API call that contains the switch() and have the completers call it, like you've got above). That way the quantity of these switches doesn't proliferate. It may be that this API function can handle all completers, in which case great, signal_completer et.al. can use it too. I'm just not big on making it a requirement. [Though of course I would like the consistency. :-)] > In any case, let me know what you'd like me to do, and I'll get right at it. Does the above make sense?