zsh-workers
 help / color / mirror / code / Atom feed
* Documentation of the TIMEFMT variable
@ 2023-02-02 16:53 trillian
  2023-02-02 17:53 ` Daniel Shahaf
  2023-02-02 19:33 ` Bart Schaefer
  0 siblings, 2 replies; 8+ messages in thread
From: trillian @ 2023-02-02 16:53 UTC (permalink / raw)
  To: zsh-workers

The documentation of TIMEFMT says that %M means "The  maximum memory the 
process had in use at any time in kilobytes", but the actual value shown 
is in megabytes. (The kernel reports it in kilobytes and zsh divides it 
by 1024 again.)


While I'm at it, TMPSUFFIX (which is just a few lines below it) seems to 
have somewhat broken formatting, as half of its description is 
highlighted like it was a variable name (in the man page at least - this 
doesn't seem to show up in the HTML documentation).


~trillian



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Documentation of the TIMEFMT variable
  2023-02-02 16:53 Documentation of the TIMEFMT variable trillian
@ 2023-02-02 17:53 ` Daniel Shahaf
  2023-02-02 19:33 ` Bart Schaefer
  1 sibling, 0 replies; 8+ messages in thread
From: Daniel Shahaf @ 2023-02-02 17:53 UTC (permalink / raw)
  To: trillian; +Cc: zsh-workers

trillian wrote on Thu, Feb 02, 2023 at 18:53:15 +0200:
> While I'm at it, TMPSUFFIX (which is just a few lines below it) seems to
> have somewhat broken formatting, as half of its description is highlighted
> like it was a variable name (in the man page at least - this doesn't seem to
> show up in the HTML documentation).

Apparently caused by nesting var() within tt().

--- a/Doc/Zsh/params.yo
+++ b/Doc/Zsh/params.yo
@@ -1668,7 +1668,7 @@ well as any directory names.  The default is `tt(/tmp/zsh)'.
 vindex(TMPSUFFIX)
 item(tt(TMPSUFFIX))(
 A filename suffix which the shell will use for temporary files created
-by process substitutions (e.g., `tt(=LPAR()var(list)RPAR())').
+by process substitutions (e.g., `tt(=LPAR())var(list)tt(RPAR())').
 Note that the value should include a leading dot `tt(.)' if intended
 to be interpreted as a file extension.  The default is not to append
 any suffix, thus this parameter should be assigned only when needed


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Documentation of the TIMEFMT variable
  2023-02-02 16:53 Documentation of the TIMEFMT variable trillian
  2023-02-02 17:53 ` Daniel Shahaf
@ 2023-02-02 19:33 ` Bart Schaefer
  2023-02-03 20:52   ` Stephane Chazelas
  1 sibling, 1 reply; 8+ messages in thread
From: Bart Schaefer @ 2023-02-02 19:33 UTC (permalink / raw)
  To: zsh-workers

On Thu, Feb 2, 2023 at 8:54 AM trillian
<just.so.you.can.email.me@gmail.com> wrote:
>
> The documentation of TIMEFMT says that %M means "The  maximum memory the
> process had in use at any time in kilobytes", but the actual value shown
> is in megabytes. (The kernel reports it in kilobytes and zsh divides it
> by 1024 again.)

This has been reported twice before, although after looking at the
previous two reports I'm not sure if this is a documentation error or
a coding error or (OS-dependently) both.  Anyone?


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Documentation of the TIMEFMT variable
  2023-02-02 19:33 ` Bart Schaefer
@ 2023-02-03 20:52   ` Stephane Chazelas
  2023-02-05  8:36     ` Daniel Shahaf
  2023-03-24 10:25     ` Jun T
  0 siblings, 2 replies; 8+ messages in thread
From: Stephane Chazelas @ 2023-02-03 20:52 UTC (permalink / raw)
  To: Bart Schaefer; +Cc: zsh-workers

2023-02-02 11:33:56 -0800, Bart Schaefer:
> On Thu, Feb 2, 2023 at 8:54 AM trillian
> <just.so.you.can.email.me@gmail.com> wrote:
> >
> > The documentation of TIMEFMT says that %M means "The  maximum memory the
> > process had in use at any time in kilobytes", but the actual value shown
> > is in megabytes. (The kernel reports it in kilobytes and zsh divides it
> > by 1024 again.)
> 
> This has been reported twice before, although after looking at the
> previous two reports I'm not sure if this is a documentation error or
> a coding error or (OS-dependently) both.  Anyone?

A portability issue. Doc is correct, but code is only correct on
Darwin/macos. GNU time's autoconf has some info on the subject
IIRC.

It's been reported many times here. See workers/45489,
workers/42363, workers/30399, workers/49145.

macos/darwin is the odd one out. ru_maxrss was already in
kibibytes in the original getrusage() implementation in BSD4.2
in 1985.

-- 
Stephane


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Documentation of the TIMEFMT variable
  2023-02-03 20:52   ` Stephane Chazelas
@ 2023-02-05  8:36     ` Daniel Shahaf
  2023-03-24 10:25     ` Jun T
  1 sibling, 0 replies; 8+ messages in thread
From: Daniel Shahaf @ 2023-02-05  8:36 UTC (permalink / raw)
  To: zsh-workers

Stephane Chazelas wrote on Fri, Feb 03, 2023 at 20:52:44 +0000:
> 2023-02-02 11:33:56 -0800, Bart Schaefer:
> > On Thu, Feb 2, 2023 at 8:54 AM trillian
> > <just.so.you.can.email.me@gmail.com> wrote:
> > >
> > > The documentation of TIMEFMT says that %M means "The  maximum memory the
> > > process had in use at any time in kilobytes", but the actual value shown
> > > is in megabytes. (The kernel reports it in kilobytes and zsh divides it
> > > by 1024 again.)
> > 
> > This has been reported twice before, although after looking at the
> > previous two reports I'm not sure if this is a documentation error or
> > a coding error or (OS-dependently) both.  Anyone?
> 
> A portability issue. Doc is correct, but code is only correct on
> Darwin/macos. GNU time's autoconf has some info on the subject
> IIRC.
> 
> It's been reported many times here. See workers/45489,
> workers/42363, workers/30399, workers/49145.
> 
> macos/darwin is the odd one out. ru_maxrss was already in
> kibibytes in the original getrusage() implementation in BSD4.2
> in 1985.

Another consideration: the doc states that %X and %D are also given
in KB.  Assuming that for these two the implementation does in fact
match the docs, changing the implementation of %M to match the docs
would result in %M/%X/%D all expanding to values in KB, which seems
preferable to having some of them expand to values in KB and some to
values in MB.

Cheers,

Daniel


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Documentation of the TIMEFMT variable
  2023-02-03 20:52   ` Stephane Chazelas
  2023-02-05  8:36     ` Daniel Shahaf
@ 2023-03-24 10:25     ` Jun T
  2023-03-24 14:43       ` Oliver Kiddle
  1 sibling, 1 reply; 8+ messages in thread
From: Jun T @ 2023-03-24 10:25 UTC (permalink / raw)
  To: zsh-workers


> 2023/02/04 5:52, Stephane Chazelas <stephane@chazelas.org> wrote:
> 
> A portability issue. Doc is correct, but code is only correct on
> Darwin/macos.
(snip)
> macos/darwin is the odd one out. ru_maxrss was already in
> kibibytes in the original getrusage() implementation in BSD4.2
> in 1985.

If ru_maxrss is in KB in all OSes other than macOS, then we can just
check $host_os in configure and set a macro in config.h indicating
that ru_maxrss need be divided by 1024.

But in manpage for Solaris:
https://docs.oracle.com/cd/E86824_01/html/E54766/getrusage-3c.html

  ru_maxrss
  The maximum resident set size. Size is given in pages (the size of a page, in bytes,
  is given by the getpagesize(3C) function). See the NOTES section of this page.

  Notes
  The ru_maxrss, ru_ixrss, ru_idrss, and ru_isrss members of the rusage structure are
  set to 0 in this implementation.

Is there anyone who can confirm that ru_maxrss is set to zero?
If it is always zero, then we need not bother with page to KB conversion.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Documentation of the TIMEFMT variable
  2023-03-24 10:25     ` Jun T
@ 2023-03-24 14:43       ` Oliver Kiddle
  2023-03-27  0:55         ` Jun T
  0 siblings, 1 reply; 8+ messages in thread
From: Oliver Kiddle @ 2023-03-24 14:43 UTC (permalink / raw)
  To: zsh-workers

Jun T wrote:
> If ru_maxrss is in KB in all OSes other than macOS, then we can just
> check $host_os in configure and set a macro in config.h indicating
> that ru_maxrss need be divided by 1024.

If there's no way to detect the correct units directly then falling back
on an OS check would work.

Stephane did do some work on refactoring limits code that never got
fully finished and merged a couple of years ago.

> But in manpage for Solaris:

> Is there anyone who can confirm that ru_maxrss is set to zero?
> If it is always zero, then we need not bother with page to KB conversion.

It does appear to always be zero. The ksh93 ulimit that acts as /bin/sh
on Solaris 11 prints:
  max memory size (Kibytes)      (-m)  not supported

There's no -m option at all for the ulimit on Solaris 10.

Oliver


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Documentation of the TIMEFMT variable
  2023-03-24 14:43       ` Oliver Kiddle
@ 2023-03-27  0:55         ` Jun T
  0 siblings, 0 replies; 8+ messages in thread
From: Jun T @ 2023-03-27  0:55 UTC (permalink / raw)
  To: zsh-workers



> 2023/03/24 23:43、Oliver Kiddle <opk@zsh.org>のメール:

>> Is there anyone who can confirm that ru_maxrss is set to zero?
>> If it is always zero, then we need not bother with page to KB conversion.
> 
> It does appear to always be zero.

Thanks. I hope the following is enough:


diff --git a/Src/jobs.c b/Src/jobs.c
index 59ddd952e..4d7172550 100644
--- a/Src/jobs.c
+++ b/Src/jobs.c
@@ -888,8 +888,13 @@ printtime(struct timeval *real, child_times_t *ti, char *desc)
 		break;
 #endif
 #ifdef HAVE_STRUCT_RUSAGE_RU_MAXRSS
+#ifdef RU_MAXRSS_IS_IN_BYTES
+# define MAXRSS_IN_KB(x) ((x)/1024)
+#else
+# define MAXRSS_IN_KB(x) (x)
+#endif
 	    case 'M':
-		fprintf(stderr, "%ld", ti->ru_maxrss / 1024);
+		fprintf(stderr, "%ld", MAXRSS_IN_KB(ti->ru_maxrss));
 		break;
 #endif
 #ifdef HAVE_STRUCT_RUSAGE_RU_MAJFLT
@@ -1036,7 +1041,7 @@ should_report_time(Job j)
 
 #ifdef HAVE_GETRUSAGE
     if (reportmemory >= 0 &&
-	j->procs->ti.ru_maxrss / 1024 > reportmemory)
+	MAXRSS_IN_KB(j->procs->ti.ru_maxrss) > reportmemory)
 	return 1;
 #endif
 
diff --git a/configure.ac b/configure.ac
index f340d2993..e6ced85d9 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1965,6 +1965,15 @@ if test x$ac_cv_func_getrusage = xyes; then
 #endif
 #include <sys/resource.h>])
 fi
+dnl On some OSes (only macOS?) ru_maxrss is in bytes (not in kilobytes).
+dnl Solaris uses pages as the unit, but ru_maxrss is set to zero anyway.
+AH_TEMPLATE(RU_MAXRSS_IS_IN_BYTES,
+[Define to 1 if ru_maxrss in struct rusage is in bytes.])
+case "$host_os" in
+  darwin*)
+    AC_DEFINE(RU_MAXRSS_IS_IN_BYTES)
+  ;;
+esac
 
 
 dnl --------------------------------------------





^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2023-03-27  0:56 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-02 16:53 Documentation of the TIMEFMT variable trillian
2023-02-02 17:53 ` Daniel Shahaf
2023-02-02 19:33 ` Bart Schaefer
2023-02-03 20:52   ` Stephane Chazelas
2023-02-05  8:36     ` Daniel Shahaf
2023-03-24 10:25     ` Jun T
2023-03-24 14:43       ` Oliver Kiddle
2023-03-27  0:55         ` Jun T

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/zsh/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).