Re: [PATCH] compinit forks less

Charles Blake <[email protected]>
Newsgroups gmane.comp.shells.zsh.devel
Message-ID <CAKiz1a9_6YTDSXX+Pkfqyn+hkgLMB-M4U3i07HnYkDj_wo1gEQ@mail.gmail.com>
I like Dana's approach even better and am happy to adapt my patch
or let them.  It also a tad faster for me on an old Skylake CPU
(more like 30..40 not 230).  I suspect it varies from OS to OS and
CPU to CPU, but it also just looks cleaner.  It seems to basically
work, but I've only just tested it weakly for a couple minutes.

It bears mention that while my first here-string idea uses two temp
files, this new still uses one temp file, and a racy one at that (not
that I see any real attack by messing up bindkey introspection).

To see, you can just (on Linux)
    strace zsh -fc 'opt=( ${(s< >)${ bindkey \^i }} )'

This is because of how ${ } is implemented which surprised me when
I looked into it after Mikael's comment.  I think the relevant code
is the part of Src/subst.c near a comment beginning "Admittedly a
hack".  Essentially, ${ } re-writes to `>| tempFileByName { expr }`.
There is no O_EXCL flag or anything on the open since there is no
redirection syntax like `>` for that (at least that I'm aware of)
to lean on.

I imagine this must have been discussed long ago when ${ } got in,
but if we're not going to fix it before the release a single sentence
about using less-safe temp files in the feature list in NEWS might be
warranted or even conceivably the man page.  While compinit bindkey
probing is unlikely a problem, other ${ } uses may not be as lucky.
It also makes the comment in the source code in the definition of
gettempname no longer true - all callers do not use O_EXCL.

Anyway, FWIW, I also tried:
    local f=/dev/shm/z$RANDOM # yeah, yeah..just a demo
    bindkey '^i' > $f
    IFS=$' \t' read -A opt < $f
    zf_rm $f   # requires assuming user loads zsh/files
and that was the same time to within 15+-10 µs of Dana's.  So, I
suspect the perf savings is mostly just "one less pass".  The point
of mentioning this besides being a pre-${ } idea is then that is
likely also roughly the perf benefit of an array variant - although
just not worrying about parsing seems the bigger win there.
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.