This revision was automatically updated to reflect the committed changes.
Closed by commit rHG055fee3547df: merge: removed sorting in casefolding
detection, for a slight performance win (authored by alex_gaynor).
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST UPDATE
https://phab.mercurial-scm.o
alex_gaynor added a comment.
If you want error message stability I'm pretty sure I can get that by
changing the error message formatting to be `tuple(sorted([f, foldmap[fold]))`.
Let me know if you'd like me to add that.
REPOSITORY
rHG Mercurial
REVISION DETAIL
https://phab.mercurial-sc
quark accepted this revision.
quark added a comment.
Looks good to me. I'd prefer perf to error message stability. If the latter
is a concern, we can re-run the loop if there is at least one case-folding
collision detected.
REPOSITORY
rHG Mercurial
REVISION DETAIL
https://phab.mercurial
alex_gaynor updated this revision to Diff 737.
alex_gaynor added a comment.
Herald added a reviewer: hg-reviewers.
Undo a change which was incorrect
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST UPDATE
https://phab.mercurial-scm.org/D30?vs=46&id=737
BRANCH
performance-changes (bookmark)
alex_gaynor added inline comments.
INLINE COMMENTS
> quark wrote in merge.py:765
> I think this would be incorrect, since `foldprefix` is mutated and used in
> the loop.
I think you're right, removing the first `sorted` should be safe though.
REPOSITORY
rHG Mercurial
REVISION DETAIL
https
quark added a comment.
@durin42 The case-folding collision might be a small feature that Rust is
meaningful.
INLINE COMMENTS
> merge.py:765
> foldprefix = unfoldprefix = lastfull = ''
> -for fold, f in sorted(foldmap.items()):
> +for fold, f in foldmap.items():
> if fol
alex_gaynor added a comment.
Going to note that I'm less confident in the correctness of the second change
than I am the first.
REPOSITORY
rHG Mercurial
REVISION DETAIL
https://phab.mercurial-scm.org/D30
EMAIL PREFERENCES
https://phab.mercurial-scm.org/settings/panel/emailpreferences/
alex_gaynor added a comment.
Not quite sure this is science, but
First set of numbers is `tip`, second is with my patch:
~/p/mozilla-central ❯❯❯ time ~/projects/hg/hg up
consolidate-sandbox-disable; time ~/projects/hg/hg up central
4324 files updated, 0 files merged, 491 f
durin42 added a comment.
Ah, I see. Alex, do you have any idea what the performance numbers on this
are like, anecdotally on your mozilla repo?
REPOSITORY
rHG Mercurial
REVISION DETAIL
https://phab.mercurial-scm.org/D30
EMAIL PREFERENCES
https://phab.mercurial-scm.org/settings/panel/e
alex_gaynor added a comment.
Whether or not an error occurs is deterministic, the exact message is the
only non-deterministic bit (assuming there's no bugs, heh).
REPOSITORY
rHG Mercurial
REVISION DETAIL
https://phab.mercurial-scm.org/D30
EMAIL PREFERENCES
https://phab.mercurial-scm.o
durin42 added a comment.
In https://phab.mercurial-scm.org/D30#1098, @quark wrote:
> > Is it important that the error be deterministic?
>
> Good question. Deterministic error is just to make tests stable AFAIK. If
the perf win is noticable, I personally prefer perf win. We can use `(g
quark added a comment.
> Is it important that the error be deterministic?
Good question. Deterministic error is just to make tests stable AFAIK. If the
perf win is noticable, I personally prefer perf win. We can use `(glob)` and
`(?)` in tests to make it support non-deterministic outputs
alex_gaynor added a comment.
Is it important that the error be deterministic?
I think we can improve the situation if you have exactly two collisions, by
sorting `(f, foldmap[fold])` and `(lastfull, f)`, but I don't think it can
easily be preserved for multiple collisions.
REPOSITORY
13 matches
Mail list logo
