Debian Bug report logs - #570447
x11-common: Optimize speed of Xsession.d scripts

Toggle useless messages

Message #5 received at submit@bugs.debian.org (full text, mbox, reply):

From: Martin Pitt <martin.pitt@ubuntu.com>

To: Debian BTS Submit <submit@bugs.debian.org>

Cc: Bryce Harrington <bryce@ubuntu.com>, Timo Aaltonen <tjaalton@ubuntu.com>

Subject: x11-common: Optimize speed of Xsession.d scripts

Date: Thu, 18 Feb 2010 23:23:21 +0100

[Message part 1 (text/plain, inline)]

Package: xorg
Version: 7.5+3
Severity: wishlist
Tags: patch
User: ubuntu-devel@lists.ubuntu.com
Usertags: origin-ubuntu lucid ubuntu-patch

Hello,

In our vendetta for faster boot I examined x11-common's Xsession.d
scripts and noticed that they call a lot of external programs (grep
several times, which) and run some code which is unlikely to be useful
on many machines (such as ~/.Xresources).

Attached patch streamlines the scripts to prefer shell builtins over
external programs. (Details see changelog).

Thanks for considering!

Martin
-- 
Martin Pitt                        | http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)

[xorg.optimize-xsession.d.debdiff (text/plain, attachment)]

[signature.asc (application/pgp-signature, inline)]

Message #10 received at 570447@bugs.debian.org (full text, mbox, reply):

From: Martin Pitt <martin.pitt@ubuntu.com>

To: 570447@bugs.debian.org

Cc: Bryce Harrington <bryce@ubuntu.com>, Timo Aaltonen <tjaalton@ubuntu.com>

Subject: Re: Bug#570447: Acknowledgement (x11-common: Optimize speed of Xsession.d scripts)

Date: Fri, 19 Feb 2010 06:45:58 +0100

[Message part 1 (text/plain, inline)]

Hello again,

argh, I just realized that the previous patch would catch commented
options as well, sorry for the blunder!

This updated patch defines a new function "has_options" which
encapsulates the option parsing, and thus makes the code easier to
read.

Thanks,

Martin
-- 
Martin Pitt                        | http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)

[xorg.optimize-xsession.d.debdiff (text/plain, attachment)]

[signature.asc (application/pgp-signature, inline)]

Message #17 received at 570447@bugs.debian.org (full text, mbox, reply):

From: Brice Goglin <Brice.Goglin@ens-lyon.org>

To: Martin Pitt <martin.pitt@ubuntu.com>, 570447@bugs.debian.org

Subject: Re: Bug#570447: Acknowledgement (x11-common: Optimize speed of Xsession.d scripts)

Date: Fri, 19 Feb 2010 12:37:17 +0100

Martin Pitt wrote:
> Hello again,
>
> argh, I just realized that the previous patch would catch commented
> options as well, sorry for the blunder!
>
> This updated patch defines a new function "has_options" which
> encapsulates the option parsing, and thus makes the code easier to
> read.
>
> Thanks,
>
> Martin
>   

How faster is the boot thanks to this patch ?

Brice

Message #22 received at 570447@bugs.debian.org (full text, mbox, reply):

From: Martin Pitt <mpitt@debian.org>

To: Brice Goglin <Brice.Goglin@ens-lyon.org>

Cc: 570447@bugs.debian.org

Subject: Re: Bug#570447: Acknowledgement (x11-common: Optimize speed of Xsession.d scripts)

Date: Fri, 19 Feb 2010 13:54:29 +0100

Brice Goglin [2010-02-19 12:37 +0100]:
> How faster is the boot thanks to this patch ?

It's nothing serious, of course, in the 0.1 s range. I was
just going through all Xsession.d scripts, and some like 90consolekit
were quite heavy. So I just cleaned up all of them while I was at it.

Now the entire Xsession.d collection just has one external program
call (the cat of the option file at the very beginning, thanks to dash
not knowing about the $(< file) syntax that bash supports).

Martin
-- 
Martin Pitt                        | http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)

Message #27 received at 570447-close@bugs.debian.org (full text, mbox, reply):

From: Brice Goglin <bgoglin@debian.org>

To: 570447-close@bugs.debian.org

Subject: Bug#570447: fixed in xorg 1:7.5+4

Date: Sun, 14 Mar 2010 10:32:58 +0000

Source: xorg
Source-Version: 1:7.5+4

We believe that the bug you reported is fixed in the latest version of
xorg, which is due to be installed in the Debian FTP archive:

libglu1-xorg-dev_7.5+4_all.deb
  to main/x/xorg/libglu1-xorg-dev_7.5+4_all.deb
libglu1-xorg_7.5+4_all.deb
  to main/x/xorg/libglu1-xorg_7.5+4_all.deb
x11-common_7.5+4_all.deb
  to main/x/xorg/x11-common_7.5+4_all.deb
xbase-clients_7.5+4_all.deb
  to main/x/xorg/xbase-clients_7.5+4_all.deb
xlibmesa-gl-dev_7.5+4_all.deb
  to main/x/xorg/xlibmesa-gl-dev_7.5+4_all.deb
xlibmesa-gl_7.5+4_all.deb
  to main/x/xorg/xlibmesa-gl_7.5+4_all.deb
xlibmesa-glu_7.5+4_all.deb
  to main/x/xorg/xlibmesa-glu_7.5+4_all.deb
xorg-dev_7.5+4_all.deb
  to main/x/xorg/xorg-dev_7.5+4_all.deb
xorg_7.5+4.dsc
  to main/x/xorg/xorg_7.5+4.dsc
xorg_7.5+4.tar.gz
  to main/x/xorg/xorg_7.5+4.tar.gz
xorg_7.5+4_i386.deb
  to main/x/xorg/xorg_7.5+4_i386.deb
xserver-xorg-input-all_7.5+4_i386.deb
  to main/x/xorg/xserver-xorg-input-all_7.5+4_i386.deb
xserver-xorg-video-all_7.5+4_i386.deb
  to main/x/xorg/xserver-xorg-video-all_7.5+4_i386.deb
xserver-xorg_7.5+4_i386.deb
  to main/x/xorg/xserver-xorg_7.5+4_i386.deb
xutils_7.5+4_all.deb
  to main/x/xorg/xutils_7.5+4_all.deb



A summary of the changes between this version and the previous one is
attached.

Thank you for reporting the bug, which will now be closed.  If you
have further comments please address them to 570447@bugs.debian.org,
and the maintainer will reopen the bug report if appropriate.

Debian distribution maintenance software
pp.
Brice Goglin <bgoglin@debian.org> (supplier of updated xorg package)

(This message was generated automatically at their request; if you
believe that there is a problem with it please contact the archive
administrators by mailing ftpmaster@debian.org)


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Format: 1.8
Date: Sun, 14 Mar 2010 11:15:07 +0100
Source: xorg
Binary: x11-common xserver-xorg xserver-xorg-video-all xserver-xorg-input-all xorg xorg-dev xlibmesa-gl xlibmesa-gl-dev xlibmesa-glu libglu1-xorg libglu1-xorg-dev xbase-clients xutils
Architecture: source all i386
Version: 1:7.5+4
Distribution: unstable
Urgency: low
Maintainer: Debian X Strike Force <debian-x@lists.debian.org>
Changed-By: Brice Goglin <bgoglin@debian.org>
Description: 
 libglu1-xorg - transitional package for Debian etch
 libglu1-xorg-dev - transitional package for Debian etch
 x11-common - X Window System (X.Org) infrastructure
 xbase-clients - miscellaneous X clients - metapackage
 xlibmesa-gl - transitional package for Debian etch
 xlibmesa-gl-dev - transitional package for Debian etch
 xlibmesa-glu - transitional package for Debian etch
 xorg       - X.Org X Window System
 xorg-dev   - the X.Org X Window System development libraries
 xserver-xorg - the X.Org X server
 xserver-xorg-input-all - the X.Org X server -- input driver metapackage
 xserver-xorg-video-all - the X.Org X server -- output driver metapackage
 xutils     - X Window System utility programs metapackage
Closes: 230422 570447
Changes: 
 xorg (1:7.5+4) unstable; urgency=low
 .
   * Add Xreset and Xreset.d support, closes: #230422.
   * Improve startup speed of Xsession.d scripts by eliminating all unnecessary
     external program calls, thanks Martin Pitt, closes: #570447.
     - In 20x11-common_process-args, cat $OPTIONFILE once into a variable and
       use POSIX variable substitution in all scripts instead of calling grep
       for every single test.
     - Use shell built in "type" instead of external "which" to test for
       programs.
     - 30x11-common_xresources: Swap the order of tests to keep the most
       unlikely (like "~/.Xresources exists") outside, to avoid running the
       other tests (like "xrdb exists") on systems which don't use Xresources.
Checksums-Sha1: 
 38146e896159317decf964facaf7d2ba2393af93 1029 xorg_7.5+4.dsc
 df64f331b45aaee496c0b92368ff148e892df5c2 878374 xorg_7.5+4.tar.gz
 d5fdd15c80829633ec200a855f14de74f12421d1 279724 x11-common_7.5+4_all.deb
 fe839a8096c46bb903e97fe1aac4aa6729549205 31324 xorg-dev_7.5+4_all.deb
 b30e6920578db0ee68a5b86a3be2b6d1906fdd24 30956 xlibmesa-gl_7.5+4_all.deb
 35cc1464692405400fe9860081d19bb67257238c 30962 xlibmesa-gl-dev_7.5+4_all.deb
 d847b71f9b569b5963def9fc4782aabdef746da7 30958 xlibmesa-glu_7.5+4_all.deb
 2eecdd8ad768b235c30782b61485256d290bbe76 30950 libglu1-xorg_7.5+4_all.deb
 9e0cd30e7951e5c973d72bc6c618891b57f2b4f0 30958 libglu1-xorg-dev_7.5+4_all.deb
 9f5910ac2b48279280d31cfdc816e26bf312e037 37336 xbase-clients_7.5+4_all.deb
 001e3345e5ad7ed11e7fe5d2f5caeb10118d125a 37436 xutils_7.5+4_all.deb
 1903494b896e8c72d8ea1219cfb9788b5d384db2 52044 xserver-xorg_7.5+4_i386.deb
 683cb6eef55139c780ed238dc7d6c146288b877f 31162 xserver-xorg-video-all_7.5+4_i386.deb
 502118966b6db20911774ceba50316b07ced0acb 31020 xserver-xorg-input-all_7.5+4_i386.deb
 99700f21f34bed449db6af872549ecf50cdc4f23 31534 xorg_7.5+4_i386.deb
Checksums-Sha256: 
 bc36343f0977f467722794efaa4c7cbc1f315388e69f0ab571b2b1a0fcd7e72a 1029 xorg_7.5+4.dsc
 e4a9cab423aab5396147457653836940ca8752230a7b232b0d186b3dd39f9936 878374 xorg_7.5+4.tar.gz
 88e59e1c930d11e7ae8f362f8a9948135308d445cfadb5b90cf0c9b3dd249943 279724 x11-common_7.5+4_all.deb
 a8bfafa8c0e7bcfc4820c5786d759d495e600238026ae242a53e142d53d35282 31324 xorg-dev_7.5+4_all.deb
 7e015f9d521f304fb70196dc69c49f2074517b2eb6dd46396ed33b396869e830 30956 xlibmesa-gl_7.5+4_all.deb
 03d1c2d41c80039d7b5b977c8b3ea9a3961d601fcd461a6424ce1457190b8d24 30962 xlibmesa-gl-dev_7.5+4_all.deb
 d1b5f8c05efec90da50ecbca717bfc041eae681baf14343b344c16e7bdec605c 30958 xlibmesa-glu_7.5+4_all.deb
 234423170e84b18584f7f59d2be80f7a4ef0bbd6421097708fbd49d5a497924f 30950 libglu1-xorg_7.5+4_all.deb
 cfd35dea550c07cd16dd14920e81ed02c0efa060a279488eab021af5521eb34b 30958 libglu1-xorg-dev_7.5+4_all.deb
 8629f7aa2175210842d4d4f27d7a5b7aa0d0ae93ceb8f7b7e92a1c865c489423 37336 xbase-clients_7.5+4_all.deb
 cf04b957fcaa7c021d1f2508cc6ad37ff0987c7df9de6b0c2236667d5276c92a 37436 xutils_7.5+4_all.deb
 e835ad9b41604e5ed98b6826ba2b50a831d170de1184dcf75c3974ab1f7699df 52044 xserver-xorg_7.5+4_i386.deb
 b0d785a9c9f62bf8653712efc0a2a7b446668829a7513602a7696b95426be729 31162 xserver-xorg-video-all_7.5+4_i386.deb
 bdae7da9b948c1672ba9e44dc380accb8d0cd245d5c3bb9a977c79549d694870 31020 xserver-xorg-input-all_7.5+4_i386.deb
 b76e165ce3d9f1fc315722e616c44ce4f225ae81b9695dd100f1f57ed28e775b 31534 xorg_7.5+4_i386.deb
Files: 
 b0f5930348c7fd958a49d17cf5038ec7 1029 x11 optional xorg_7.5+4.dsc
 d82e1c36d5fa90489f0368f89e6ce247 878374 x11 optional xorg_7.5+4.tar.gz
 66db726f16b7d4470ed75b4aadac7255 279724 x11 optional x11-common_7.5+4_all.deb
 975e43d2651360127e52117fb732e904 31324 x11 optional xorg-dev_7.5+4_all.deb
 9012d377074589af2864979cf0741f1b 30956 libs optional xlibmesa-gl_7.5+4_all.deb
 706a71167d6e831a71dc05698e840cb2 30962 libdevel optional xlibmesa-gl-dev_7.5+4_all.deb
 9aa71a82029cff22c00caa8a0d778788 30958 libdevel optional xlibmesa-glu_7.5+4_all.deb
 5253281343a99c9a4f056007e3ef8489 30950 libs optional libglu1-xorg_7.5+4_all.deb
 ed44177e30433625696ecfb0ddb9a323 30958 libdevel optional libglu1-xorg-dev_7.5+4_all.deb
 81178946e9de4944965a49a4b2f1b690 37336 x11 optional xbase-clients_7.5+4_all.deb
 5e239fd18e8c5b33f52ae956d61bd27f 37436 x11 optional xutils_7.5+4_all.deb
 9d6e3d19510636935d37e24c99af47c9 52044 x11 optional xserver-xorg_7.5+4_i386.deb
 6ae843b2c433d0f45647f2b6febc98a2 31162 x11 optional xserver-xorg-video-all_7.5+4_i386.deb
 f64b1d59159e95f8629c1e3667068692 31020 x11 optional xserver-xorg-input-all_7.5+4_i386.deb
 7442e1cc6ad20229cd96d8b0419a3caf 31534 x11 optional xorg_7.5+4_i386.deb

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)

iEYEARECAAYFAkuct9QACgkQRh88F8PcWfqdSgCgsNpUTj/wMJ5wD+ReJ04cEh0t
p8kAn19OFvzO/eQ61RU/nxwCzaX7V4pU
=OXeM
-----END PGP SIGNATURE-----

Message #32 received at 570447@bugs.debian.org (full text, mbox, reply):

From: Julien Cristau <jcristau@debian.org>

To: Martin Pitt <martin.pitt@ubuntu.com>, 570447@bugs.debian.org

Cc: Bryce Harrington <bryce@ubuntu.com>, Timo Aaltonen <tjaalton@ubuntu.com>

Subject: Re: Bug#570447: Acknowledgement (x11-common: Optimize speed of Xsession.d scripts)

Date: Sun, 14 Mar 2010 12:04:23 +0100

[Message part 1 (text/plain, inline)]

Hi,

I'm probably looking at this a bit late, but...

On Fri, Feb 19, 2010 at 06:45:58 +0100, Martin Pitt wrote:

> diff -Nru xorg-7.5+1ubuntu2/debian/changelog xorg-7.5+1ubuntu3/debian/changelog
> --- xorg-7.5+1ubuntu2/debian/changelog	2010-01-26 00:38:13.000000000 +0100
> +++ xorg-7.5+1ubuntu3/debian/changelog	2010-02-18 23:14:11.000000000 +0100
> @@ -1,3 +1,18 @@
> +xorg (1:7.5+1ubuntu3) lucid; urgency=low
> +
> +  * Improve startup speed of Xsession.d scripts by eliminating all unnecessary
> +    external program calls:
> +    - In 20x11-common_process-args, cat $OPTIONFILE once into a variable and
> +      use POSIX variable substitution in all scripts instead of calling grep
> +      for every single test.

This looks like a good idea.

> +    - Use shell built in "type" instead of external "which" to test for
> +      programs.

There's no guarantee that /bin/sh has a 'type' built-in, as far as I can
tell from SUSv3 and policy).  I'd suggest command -v, but apparently
posh doesn't like that either, so I don't know.

> +    - 30x11-common_xresources: Swap the order of tests to keep the most
> +      unlikely (like "~/.Xresources exists") outside, to avoid running the
> +      other tests (like "xrdb exists") on systems which don't use Xresources.
> +

I'm not sure about this one.  x11-common installs a file in
/etc/X11/Xresources, so you'll end up looking for xrdb on pretty much
every system regardless.  And then the reordering means you're looking
for it twice (granted, with 'type' the shell can cache the result of the
first lookup, but still).

Cheers,
Julien

[signature.asc (application/pgp-signature, inline)]

Message #37 received at 570447@bugs.debian.org (full text, mbox, reply):

From: Martin Pitt <mpitt@debian.org>

To: Julien Cristau <jcristau@debian.org>

Cc: 570447@bugs.debian.org, Bryce Harrington <bryce@ubuntu.com>, Timo Aaltonen <tjaalton@ubuntu.com>

Subject: Re: Bug#570447: Acknowledgement (x11-common: Optimize speed of Xsession.d scripts)

Date: Thu, 1 Apr 2010 10:27:55 +0200

[Message part 1 (text/plain, inline)]

Hello Julien,

Julien Cristau [2010-03-14 12:04 +0100]:
> > +    - Use shell built in "type" instead of external "which" to test for
> > +      programs.
> 
> There's no guarantee that /bin/sh has a 'type' built-in, as far as I can
> tell from SUSv3 and policy).  I'd suggest command -v, but apparently
> posh doesn't like that either, so I don't know.

Hm, it's present in dash and bash, anyway. But if you like command -V
better, I'm fine with that.

> > +    - 30x11-common_xresources: Swap the order of tests to keep the most
> > +      unlikely (like "~/.Xresources exists") outside, to avoid running the
> > +      other tests (like "xrdb exists") on systems which don't use Xresources.
> > +
> 
> I'm not sure about this one.  x11-common installs a file in
> /etc/X11/Xresources, so you'll end up looking for xrdb on pretty much
> every system regardless.

Right, it's a minor case, if someone decides to remove
/etc/X11/Xresources/x11-common. But I still think it's a tad less
likely to be true than the existence of xrdb.

> And then the reordering means you're looking for it twice (granted,
> with 'type' the shell can cache the result of the first lookup, but
> still).

Hm, I don't understand? The second type is only done if  
[ -f "$USRRESOURCES" ], and ~/.Xresources should exist pretty seldomly
these days?

Thanks,

Martin
-- 
Martin Pitt                        | http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)

[signature.asc (application/pgp-signature, inline)]

Message #42 received at 570447@bugs.debian.org (full text, mbox, reply):

From: Julien Cristau <jcristau@debian.org>

To: Martin Pitt <mpitt@debian.org>

Cc: 570447@bugs.debian.org, Bryce Harrington <bryce@ubuntu.com>, Timo Aaltonen <tjaalton@ubuntu.com>

Subject: Re: Bug#570447: Acknowledgement (x11-common: Optimize speed of Xsession.d scripts)

Date: Sat, 3 Apr 2010 19:16:52 +0200

[Message part 1 (text/plain, inline)]

On Thu, Apr  1, 2010 at 10:27:55 +0200, Martin Pitt wrote:

> Hello Julien,
> 
> Julien Cristau [2010-03-14 12:04 +0100]:
> > > +    - Use shell built in "type" instead of external "which" to test for
> > > +      programs.
> > 
> > There's no guarantee that /bin/sh has a 'type' built-in, as far as I can
> > tell from SUSv3 and policy).  I'd suggest command -v, but apparently
> > posh doesn't like that either, so I don't know.
> 
> Hm, it's present in dash and bash, anyway. But if you like command -V
> better, I'm fine with that.
> 
I'd like something which is guaranteed to work.  "which" was, "type"
isn't...

Another way would be to run xrdb -help >/dev/null 2>&1 and test the
result of that.

> > > +    - 30x11-common_xresources: Swap the order of tests to keep the most
> > > +      unlikely (like "~/.Xresources exists") outside, to avoid running the
> > > +      other tests (like "xrdb exists") on systems which don't use Xresources.
> > > +
> > 
> > I'm not sure about this one.  x11-common installs a file in
> > /etc/X11/Xresources, so you'll end up looking for xrdb on pretty much
> > every system regardless.
> 
> Right, it's a minor case, if someone decides to remove
> /etc/X11/Xresources/x11-common. But I still think it's a tad less
> likely to be true than the existence of xrdb.
> 
> > And then the reordering means you're looking for it twice (granted,
> > with 'type' the shell can cache the result of the first lookup, but
> > still).
> 
> Hm, I don't understand? The second type is only done if  
> [ -f "$USRRESOURCES" ], and ~/.Xresources should exist pretty seldomly
> these days?
> 
we went from:

if has xrdb; then
  if has sysresources; then
    xrdb -merge sysresources
  fi
  if want userresources and has userresources; then
    xrdb -merge userresources
  fi
else
  warn
fi

to

if has sysresources and has xrdb; then
  xrdb -merge sysresources
fi
if want userresources and has userresources; then
  if has xrdb; then
    xrdb -merge userresources
  else
    warn
  fi
fi

The changelog says this was to avoid testing for xrdb if ~/.Xresources
doesn't exist.  But we'll keep testing for xrdb anyway, since we'll
pretty much always have /etc/X11/Xresources/.  So you'll still run the
exact same tests.

Plus there's a behaviour change in the new version which only warns when
~/.Xresources exists and is enabled.

So I'm still not convinced this reordering provides any gain.  Not a big
deal though.

Cheers,
Julien

[signature.asc (application/pgp-signature, inline)]

Message #47 received at 570447@bugs.debian.org (full text, mbox, reply):

From: Martin Pitt <mpitt@debian.org>

To: Julien Cristau <jcristau@debian.org>

Cc: 570447@bugs.debian.org, Bryce Harrington <bryce@ubuntu.com>, Timo Aaltonen <tjaalton@ubuntu.com>

Subject: Re: Bug#570447: Acknowledgement (x11-common: Optimize speed of Xsession.d scripts)

Date: Sun, 4 Apr 2010 20:01:26 +0200

[Message part 1 (text/plain, inline)]

Hello Julien,

Julien Cristau [2010-04-03 19:16 +0200]:
> we went from [...] to [...]
>
> The changelog says this was to avoid testing for xrdb if ~/.Xresources
> doesn't exist.  But we'll keep testing for xrdb anyway, since we'll
> pretty much always have /etc/X11/Xresources/.  So you'll still run the
> exact same tests.

Ah, of course. I'm not sure now, but I guess back then I simply
overlooked the system Xresource file then.

So please feel free to revert that.

Thanks,

Martin

-- 
Martin Pitt                        | http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)

[signature.asc (application/pgp-signature, inline)]

Send a report that this bug log contains spam.