| 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.
durin42 created this revision. Herald added a subscriber: mercurial-devel. Herald added a reviewer: hg-reviewers. REVISION SUMMARY It seems like 2048 directories ought to be enough for any reasonable use of Mercurial? .. bc: Mercurial will now defend against OOMs by refusing to operate on paths with 2048 or more components. This means that _extremely_ deep path hierarchies will be rejected, but we anticipate nobody is using hierarchies this deep. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D7411 AFFECTED FILES mercurial/cext/dirs.c CHANGE DETAILS To: durin42, #hg-reviewers Cc: mercurial-devel