From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Dmitry Gutov Newsgroups: gmane.emacs.bugs Subject: bug#66020: (bug#64735 spin-off): regarding the default for read-process-output-max Date: Sun, 24 Sep 2023 00:51:28 +0300 Message-ID: References: <83il8lxjcu.fsf@gnu.org> <2e21ec81-8e4f-4c02-ea15-43bd6da3daa7@gutov.dev> <8334zmtwwi.fsf@gnu.org> <83tts0rkh5.fsf@gnu.org> <831qf3pd1y.fsf@gnu.org> <28a7916e-92d5-77ab-a61e-f85b59ac76b1@gutov.dev> <83sf7jnq0m.fsf@gnu.org> <5c493f86-0af5-256f-41a7-7d886ab4c5e4@gutov.dev> <83ledanvzw.fsf@gnu.org> <83r0n2m7qz.fsf@gnu.org> <26afa109-9ba3-78a3-0e68-7585ae8e3a19@gutov.dev> <83il8dna30.fsf@gnu.org> <83bke5mhvs.fsf@gnu.org> <83a5tmk79p.fsf@gnu.org> <937d9927-506f-aa36-94e9-3cceb8f629dd@gutov.dev> <83zg1hay6q.fsf@gnu.org> <451d6012-e5ab-df6c-50e3-dac20b91781c@gutov.dev> <83led09dlk.fsf@gnu.org> <1d43341a-2e7c-9f1d-68e9-9bac6f4b5fba@gutov.dev> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="33593"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 Cc: stefankangas@gmail.com, 66020@debbugs.gnu.org, monnier@iro.umontreal.ca To: Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Sat Sep 23 23:52:08 2023 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1qkAXn-0008UP-65 for geb-bug-gnu-emacs@m.gmane-mx.org; Sat, 23 Sep 2023 23:52:08 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1qkAXY-0003ZG-FC; Sat, 23 Sep 2023 17:51:52 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qkAXW-0003Z3-Qi for bug-gnu-emacs@gnu.org; Sat, 23 Sep 2023 17:51:50 -0400 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1qkAXW-0008Do-I1 for bug-gnu-emacs@gnu.org; Sat, 23 Sep 2023 17:51:50 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1qkAXh-0006jd-Ia for bug-gnu-emacs@gnu.org; Sat, 23 Sep 2023 17:52:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Dmitry Gutov Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sat, 23 Sep 2023 21:52:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 66020 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 66020-submit@debbugs.gnu.org id=B66020.169550591425868 (code B ref 66020); Sat, 23 Sep 2023 21:52:01 +0000 Original-Received: (at 66020) by debbugs.gnu.org; 23 Sep 2023 21:51:54 +0000 Original-Received: from localhost ([127.0.0.1]:40708 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qkAXZ-0006jA-SI for submit@debbugs.gnu.org; Sat, 23 Sep 2023 17:51:54 -0400 Original-Received: from wout2-smtp.messagingengine.com ([64.147.123.25]:60373) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qkAXW-0006iv-KN for 66020@debbugs.gnu.org; Sat, 23 Sep 2023 17:51:52 -0400 Original-Received: from compute4.internal (compute4.nyi.internal [10.202.2.44]) by mailout.west.internal (Postfix) with ESMTP id 1BBBF3200708; Sat, 23 Sep 2023 17:51:33 -0400 (EDT) Original-Received: from mailfrontend2 ([10.202.2.163]) by compute4.internal (MEProxy); Sat, 23 Sep 2023 17:51:33 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gutov.dev; h=cc :cc:content-transfer-encoding:content-type:content-type:date :date:from:from:in-reply-to:in-reply-to:message-id:mime-version :references:reply-to:sender:subject:subject:to:to; s=fm1; t= 1695505892; x=1695592292; bh=lU2PwP/nCbNrGF9LADRreHQ6651kwnKlFlG HmvnQAKU=; b=F0MM96mM5+hglEDZml84m5bK7mbtivfmA5Cv8RWFMp8pvj4qlUL IK1KVbVU3I3G0bqKcL9W4jG/sH3av+4/3prVLu0ceRJTl/BZVHVeX2jPlCg9ROVB EWuKAz8MGfak+AIhtmF9htMSdqZnu+Mdxpf2YAhUajUeHP00/z4/Sb/rAJ5Lrsa7 xrzoF40fTurqAWR/L8tJ4kbo1D0ip0uO0+wU7xw0CFmvexLbbqg9FlE35MmBRxL6 zeIhLaozN0Ygtd8Z3rkJ4WFscQos0tf6DTJNPyUbz524RZOHIkAfhCDxhESoZ2YW UDMMyAxmyZ+uxM/pH8eGYUJfU/X5XUTPCvQ== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding :content-type:content-type:date:date:feedback-id:feedback-id :from:from:in-reply-to:in-reply-to:message-id:mime-version :references:reply-to:sender:subject:subject:to:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm2; t= 1695505892; x=1695592292; bh=lU2PwP/nCbNrGF9LADRreHQ6651kwnKlFlG HmvnQAKU=; b=IP/vPYrNJr777BjwMguIsdSy8cK84ZpbtGWH7YEntN6+q6LAoPA i/bHpzSLmF2ri6IIXDHX1CXIxLbKavkFjrsFyNQX4q6cdI4hWpx+v5af+T4GNmuB 9M47WTWShsdnrf4IgfIJWopvvdMWC/7bBOOg///fF7Tul7DdU2qNvyYq3A8qG3xu 1l3cCV0pqcVQHruo/BzVaqFDzqsw//LDXt1gZvQTGb6+QcwjVhvqkt6Z0523l27K pE4jwWMWnH608B3/KpfkQmaP9lSuYSXSqwRgJtRiTsu12OGmBzt4aYI/VGsCAgMM Gm9S+mrex7XuV6l2B03jmvk4EnVgJS8Takw== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedviedrudeluddgtdegucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhepkfffgggfuffhvfevfhgjtgfgsehtkeertddtfeejnecuhfhrohhmpeffmhhi thhrhicuifhuthhovhcuoegumhhithhrhiesghhuthhovhdruggvvheqnecuggftrfgrth htvghrnhepteehheekteegfeeufffgtdetvdduledvjeejffeikeevleevffffiefgueej keelnecuffhomhgrihhnpehsthgrtghkvgigtghhrghnghgvrdgtohhmnecuvehluhhsth gvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepughmihhtrhihsehguhht ohhvrdguvghv X-ME-Proxy: Feedback-ID: i0e71465a:Fastmail Original-Received: by mail.messagingengine.com (Postfix) with ESMTPA; Sat, 23 Sep 2023 17:51:31 -0400 (EDT) Content-Language: en-US In-Reply-To: <1d43341a-2e7c-9f1d-68e9-9bac6f4b5fba@gutov.dev> X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:271194 Archived-At: On 21/09/2023 20:33, Dmitry Gutov wrote: > On 21/09/2023 17:37, Dmitry Gutov wrote: >> We could look into improving that part specifically: for example, >> reading from the process multiple times into 'chars' right away while >> there is still pending output present (either looping inside >> read_process_output, or calling it in a loop in >> wait_reading_process_output, at least until the process' buffered >> output is exhausted). That could reduce reactivity, however (can we >> find out how much is already buffered in advance, and only loop until >> we exhaust that length?) > > Hmm, the naive patch below offers some improvement for the value 4096, > but still not comparable to raising the buffer size: 0.76 -> 0.72. > > diff --git a/src/process.c b/src/process.c > index 2376d0f288d..a550e223f78 100644 > --- a/src/process.c > +++ b/src/process.c > @@ -5893,7 +5893,7 @@ wait_reading_process_output (intmax_t time_limit, > int nsecs, int read_kbd, >            && ((fd_callback_info[channel].flags & (KEYBOARD_FD | > PROCESS_FD)) >            == PROCESS_FD)) >          { > -          int nread; > +          int nread = 0, nnread; > >            /* If waiting for this channel, arrange to return as >           soon as no more input to be processed.  No more > @@ -5912,7 +5912,13 @@ wait_reading_process_output (intmax_t time_limit, > int nsecs, int read_kbd, >            /* Read data from the process, starting with our >           buffered-ahead character if we have one.  */ > > -          nread = read_process_output (proc, channel); > +          do > +        { > +          nnread = read_process_output (proc, channel); > +          nread += nnread; > +        } > +          while (nnread >= 4096); > + >            if ((!wait_proc || wait_proc == XPROCESS (proc)) >            && got_some_output < nread) >          got_some_output = nread; > > > And "unlocking" the pipe size on the external process takes the > performance further up a notch (by default it's much larger): 0.72 -> 0.65. > > diff --git a/src/process.c b/src/process.c > index 2376d0f288d..85fc1b4d0c8 100644 > --- a/src/process.c > +++ b/src/process.c > @@ -2206,10 +2206,10 @@ create_process (Lisp_Object process, char > **new_argv, Lisp_Object current_dir) >        inchannel = p->open_fd[READ_FROM_SUBPROCESS]; >        forkout = p->open_fd[SUBPROCESS_STDOUT]; > > -#if (defined (GNU_LINUX) || defined __ANDROID__)    \ > -  && defined (F_SETPIPE_SZ) > -      fcntl (inchannel, F_SETPIPE_SZ, read_process_output_max); > -#endif /* (GNU_LINUX || __ANDROID__) && F_SETPIPE_SZ */ > +/* #if (defined (GNU_LINUX) || defined __ANDROID__)    \ */ > +/*   && defined (F_SETPIPE_SZ) */ > +/*       fcntl (inchannel, F_SETPIPE_SZ, read_process_output_max); */ > +/* #endif /\* (GNU_LINUX || __ANDROID__) && F_SETPIPE_SZ *\/ */ >      } > >    if (!NILP (p->stderrproc)) > > Apparently the patch from bug#55737 also made things a little worse by > default, by limiting concurrency (the external process has to wait while > the pipe is blocked, and by default Linux's pipe is larger). Just > commenting it out makes performance a little better as well, though not > as much as the two patches together. > > Note that both changes above are just PoC (e.g. the hardcoded 4096, and > probably other details like carryover). > > I've tried to make a more nuanced loop inside read_process_output > instead (as replacement for the first patch above), and so far it > performs worse that the baseline. If anyone can see when I'm doing wrong > (see attachment), comments are very welcome. This seems to have been a dead end: while looping does indeed make things faster, it doesn't really fit the approach of the 'adaptive_read_buffering' part that's implemented in read_process_output. And if the external process is crazy fast (while we, e.g. when using a Lisp filter, are not so fast), the result could be much reduced interactivity, with this one process keeping us stuck in the loop. But it seems I've found an answer to one previous question: "can we find out how much is already buffered in advance?" The patch below asks that from the OS (how portable is this? not sure) and allocates a larger buffer when more output has been buffered. If we keep OS's default value of pipe buffer size (64K on Linux and 16K-ish on macOS, according to https://unix.stackexchange.com/questions/11946/how-big-is-the-pipe-buffer), that means auto-scaling the buffer on Emacs's side depending on how much the process outputs. The effect on performance is similar to the previous (looping) patch (0.70 -> 0.65), and is comparable to bumping read-process-output-max to 65536. So if we do decide to bump the default, I suppose the below should not be necessary. And I don't know whether we should be concerned about fragmentation: this way buffers do get allocates in different sizes (almost always multiples of 4096, but with rare exceptions among larger values). diff --git a/src/process.c b/src/process.c index 2376d0f288d..13cf6d6c50d 100644 --- a/src/process.c +++ b/src/process.c @@ -6137,7 +6145,18 @@ specpdl_ref count = SPECPDL_INDEX (); Lisp_Object odeactivate; char *chars; +#ifdef USABLE_FIONREAD +#ifdef DATAGRAM_SOCKETS + if (!DATAGRAM_CHAN_P (channel)) +#endif + { + int available_read; + ioctl (p->infd, FIONREAD, &available_read); + readmax = MAX (readmax, available_read); + } +#endif + USE_SAFE_ALLOCA; chars = SAFE_ALLOCA (sizeof coding->carryover + readmax); What do people think?