Submitter | phabricator |
---|---|
Date | Nov. 14, 2019, 9:22 p.m. |
Message ID | <differential-rev-PHID-DREV-t7ikglxfjme6pz6pzltg-req@mercurial-scm.org> |
Download | mbox | patch |
Permalink | /patch/43220/ |
State | Superseded |
Headers | show |
Comments
indygreg added a comment. I support this approach. But I'd feel better if we captured the performance implications. Especially since there is a comment just below talking about how important the loop is for performance. Perhaps we should add this check to the loop below by checking how m INLINE COMMENTS > dirs.c:66 > + for (i = 0; i < pos; ++i) { > + if (cpath[i] == '/') { > + ++num_slashes; What code calls this function? Do we have any good perf numbers for introducing this loop? I ask because the diffing code is surprisingly impacted by the the "find newlines" stage. Using an implementation that the compiler can expand to SSE/AVX instructions is substantially faster. FWIW glibc and other C implementations have assembly versions of `strchr()` and `memchr()`, which could be substantially faster if the compiler isn't smart enough to detect the "count occurrences of chars" pattern. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7411/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7411 To: durin42, #hg-reviewers Cc: indygreg, mercurial-devel
durin42 added inline comments. durin42 marked an inline comment as done. INLINE COMMENTS > indygreg wrote in dirs.c:66 > What code calls this function? Do we have any good perf numbers for introducing this loop? > > I ask because the diffing code is surprisingly impacted by the the "find newlines" stage. Using an implementation that the compiler can expand to SSE/AVX instructions is substantially faster. FWIW glibc and other C implementations have assembly versions of `strchr()` and `memchr()`, which could be substantially faster if the compiler isn't smart enough to detect the "count occurrences of chars" pattern. I'd be happy to use memchr() but it occurred to me as I was failing to use memchr() effectively that we already find slashes in a loop here, and there's no risk of on-disk corruption so we can just count slashes as we populate the dict and stop. It's not nearly as fast in the fuzzer, but it does pass for the specific input that we're stuck on. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7411/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7411 To: durin42, #hg-reviewers Cc: indygreg, mercurial-devel
This revision is now accepted and ready to land. indygreg added a comment. indygreg accepted this revision. This is much better. Thanks. REPOSITORY rHG Mercurial BRANCH default CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7411/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7411 To: durin42, #hg-reviewers, indygreg Cc: indygreg, mercurial-devel
Patch
diff --git a/mercurial/cext/dirs.c b/mercurial/cext/dirs.c --- a/mercurial/cext/dirs.c +++ b/mercurial/cext/dirs.c @@ -48,12 +48,30 @@ return pos; } +/* Mercurial will fail to run on directory hierarchies deeper than + * this constant, so we should try and keep this constant as big as + * possible. + */ +#define MAX_DIRS_DEPTH 2048 + static int _addpath(PyObject *dirs, PyObject *path) { const char *cpath = PyBytes_AS_STRING(path); Py_ssize_t pos = PyBytes_GET_SIZE(path); PyObject *key = NULL; int ret = -1; + size_t num_slashes = 0, i; + + for (i = 0; i < pos; ++i) { + if (cpath[i] == '/') { + ++num_slashes; + } + } + if (num_slashes > MAX_DIRS_DEPTH) { + PyErr_SetString(PyExc_ValueError, + "Directory hierarchy too deep."); + goto bail; + } /* This loop is super critical for performance. That's why we inline * access to Python structs instead of going through a supported API.