[PATCH v2] osutil: add darwin-only version of os.listdir using cffi
Augie Fackler
raf at durin42.com
Fri Jul 15 13:05:15 EDT 2016
On Fri, Jul 15, 2016 at 09:58:59AM +0200, Maciej Fijalkowski wrote:
> On Thu, Jul 14, 2016 at 4:22 PM, Jun Wu <quark at fb.com> wrote:
> > I did a quick look and the direction looks good to me. I think this needs
> > some eyes from people more familiar with OSX.
> >
> > Excerpts from Maciej Fijalkowski's message of 2016-07-12 19:30:07 +0200:
> >> # HG changeset patch
> >> # User Maciej Fijalkowski <fijall at gmail.com>
> >> # Date 1468227908 -7200
> >> # Mon Jul 11 11:05:08 2016 +0200
> >> # Node ID e932d21fdf2c8efe34807f3aa5081e98b71b523d
> >> # Parent 065fb34034a3849f07ca836ce2f4eb17c7f543db
> >> osutil: add darwin-only version of os.listdir using cffi
> >>
> >> diff -r 065fb34034a3 -r e932d21fdf2c mercurial/build_osutil_cffi.py
> >
> > This seems to be the first file under "mercurial" related to build. Thus
> > looks a bit strange. I think the existing pattern is to put these just in
> > setup.py.
>
> I would prefer for setup.py not to grow indefinitely (it's already
> huge) which directory you want it?
Maybe call it setup_osutil_cffi.py and put it in the repo root. It
definitely doesn't belong in mercurial/ as a namespace.
>
> >
> >> diff -r 065fb34034a3 -r e932d21fdf2c mercurial/pure/osutil.py
> >> --- a/mercurial/pure/osutil.py Mon Jul 11 10:44:18 2016 +0200
> >> +++ b/mercurial/pure/osutil.py Mon Jul 11 11:05:08 2016 +0200
> >> @@ -14,6 +14,10 @@
> >> import stat as statmod
> >> import sys
> >>
> >> +from . import policy
> >> +modulepolicy = policy.policy
> >> +policynocffi = policy.policynocffi
> >> +
> >> def _mode_to_kind(mode):
> >> if statmod.S_ISREG(mode):
> >> return statmod.S_IFREG
> >> @@ -31,7 +35,7 @@
> >> return statmod.S_IFSOCK
> >> return mode
> >>
> >> -def listdir(path, stat=False, skip=None):
> >> +def listdirpure(path, stat=False, skip=None):
> >> '''listdir(path, stat=False) -> list_of_tuples
> >>
> >> Return a sorted list containing information about the entries
> >> @@ -61,6 +65,97 @@
> >> result.append((fn, _mode_to_kind(st.st_mode)))
> >> return result
> >>
> >> +ffi = None
> >> +if modulepolicy not in policynocffi and sys.platform == 'darwin':
> >> + try:
> >> + from _osutil_cffi import ffi, lib
> >
> > Is _osutil_cffi in future patches?
>
> It's the effect of running build_osutil_cffi
>
> >
> >> + except ImportError:
> >> + if modulepolicy == 'cffi': # strict cffi import
> >> + raise
> >
> >> + listdir = listdirpure
> >
> > This line is unnecessary.
>
> indeed
>
> >
> >> +if sys.platform == 'darwin' and ffi is not None:
> >> + listdir_batch_size = 4096
> >> +
> >> + attrkinds = [None] * 20
> >
> > Better to add a comment on how the number 20 is chosen.
>
> noted
>
> >
> >> +
> >> + attrkinds[lib.VREG] = statmod.S_IFREG
> >> + attrkinds[lib.VDIR] = statmod.S_IFDIR
> >> + attrkinds[lib.VLNK] = statmod.S_IFLNK
> >> + attrkinds[lib.VBLK] = statmod.S_IFBLK
> >> + attrkinds[lib.VCHR] = statmod.S_IFCHR
> >> + attrkinds[lib.VFIFO] = statmod.S_IFIFO
> >> + attrkinds[lib.VSOCK] = statmod.S_IFSOCK
> >> +
> >> + class stat_res(object):
> >> + def __init__(self, st_mode, st_mtime, st_size):
> >> + self.st_mode = st_mode
> >> + self.st_mtime = st_mtime
> >> + self.st_size = st_size
> >> +
> >> + tv_sec_ofs = ffi.offsetof("struct timespec", "tv_sec")
> >> + buf = ffi.new("char[]", listdir_batch_size)
> >> +
> >> + def listdirinternal(dfd, req, stat, skip):
> >> + ret = []
> >> + while True:
> >> + r = lib.getattrlistbulk(dfd, req, buf, listdir_batch_size, 0)
> >> + if r == 0:
> >> + break
> >> + if r == -1:
> >> + raise OSError(ffi.errno, os.strerror(ffi.errno))
> >> + cur = int(ffi.cast("intptr_t", buf))
> >> + for i in range(r):
> >> + lgt = ffi.cast("uint32_t*", cur)[0]
> >> + c = cur + 4 + ffi.sizeof("attribute_set_t")
> >
> > Could we define the "val_attrs_t" struct in the ffi module to avoid manual
> > pointer operations here? Will it hurt perf?
> >
> > typedef struct val_attrs {
> > uint32_t length;
> > attribute_set_t returned;
> > uint32_t error;
> > attrreference_t name_info;
> > char *name;
> > fsobj_type_t obj_type;
> > } val_attrs_t;
>
> cffi does not support __attribute__(packed), so we can't define it
> like that (maybe there is a better way)
>
> >
> >> + ofs = ffi.cast("uint32_t*", c)[0]
> >> + str_lgt = ffi.cast("uint32_t*", c)[1]
> >> + name = str(ffi.buffer(ffi.cast("char*", c + ofs), str_lgt - 1))
> >> + c += ffi.sizeof("attrreference_t")
> >> + tp = attrkinds[ffi.cast("uint8_t*", c)[0]]
> >> + c += ffi.sizeof("uint32_t")
> >> + if name == "." or name == "..":
> >> + continue
> >> + if skip == name and tp == statmod.S_ISDIR:
> >> + return []
> >> + if stat:
> >> + c1 = c + tv_sec_ofs
> >> + mtime = ffi.cast("time_t*", c1)[0]
> >> + c += ffi.sizeof("struct timespec")
> >> + mode = (ffi.cast("uint32_t*", c)[0] & ~lib.S_IFMT)| tp
> >> + c += ffi.sizeof("uint32_t")
> >> + size = ffi.cast("off_t*", c)[0]
> >> + ret.append((name, tp, stat_res(st_mode=mode, st_mtime=mtime,
> >> + st_size=size)))
> >> + else:
> >> + ret.append((name, tp))
> >> + cur += lgt
> >> + return ret
> >> +
> >> + def listdir(path, stat=False, skip=None):
> >> + req = ffi.new("struct attrlist*")
> >> + req.bitmapcount = lib.ATTR_BIT_MAP_COUNT
> >> + req.commonattr = (lib.ATTR_CMN_NAME | lib.ATTR_CMN_OBJTYPE |
> >> + lib.ATTR_CMN_RETURNED_ATTRS |
> >> + lib.ATTR_CMN_ACCESSMASK |
> >> + lib.ATTR_CMN_MODTIME)
> >> + req.fileattr = lib.ATTR_FILE_DATALENGTH
> >> + dfd = lib.open(path, lib.O_RDONLY, 0)
> >> + if dfd == -1:
> >> + raise OSError(ffi.errno, os.strerror(ffi.errno))
> >> +
> >> + try:
> >> + ret = listdirinternal(dfd, req, stat, skip)
> >> + finally:
> >> + try:
> >> + lib.close(dfd)
> >> + except BaseException:
> >> + pass # we ignore all the errors from closing, not
> >> + # much we can do about that
> >> + return ret
> >> +else:
> >> + listdir = listdirpure
> >> +
> >> if os.name != 'nt':
> >> posixfile = open
> >>
> >> diff -r 065fb34034a3 -r e932d21fdf2c setup.py
> >> --- a/setup.py Mon Jul 11 10:44:18 2016 +0200
> >> +++ b/setup.py Mon Jul 11 11:05:08 2016 +0200
> >> @@ -320,6 +320,9 @@
> >> elif self.distribution.cffi:
> >> exts = []
> >> # cffi modules go here
> >> + if sys.platform == 'darwin':
> >> + from mercurial import build_osutil_cffi
> >> + exts.append(build_osutil_cffi.ffi.distutils_extension())
> >> self.distribution.ext_modules = exts
> >> else:
> >> h = os.path.join(get_python_inc(), 'Python.h')
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
More information about the Mercurial-devel
mailing list