| Submitter | Boris Feld |
|---|---|
| Date | Dec. 2, 2018, 4:17 p.m. |
| Message ID | <9708243274585f9544c7.1543767463@pc62.home> |
| Download | mbox | patch |
| Permalink | /patch/36906/ |
| State | Accepted |
| Headers | show |
Comments
On Sun, Dec 2, 2018 at 7:23 PM Boris Feld <boris.feld@octobus.net> wrote: > # HG changeset patch > # User Boris Feld <boris.feld@octobus.net> > # Date 1542949784 -3600 > # Fri Nov 23 06:09:44 2018 +0100 > # Node ID 9708243274585f9544c70925eb0b0fa0ec7aba4f > # Parent 0fff68dfbe48d87dce8b8736c0347ed5aa79030e > # EXP-Topic mmap > # Available At https://bitbucket.org/octobus/mercurial-devel/ > # hg pull https://bitbucket.org/octobus/mercurial-devel/ -r > 970824327458 > mmapindex: set default to 1MB > > mmapping index is more efficient if we only need a small part of it. > > The 1MB value has been picked arbitrarily, a lower value might be better. > On a large repository with a 60MB index, we see the following performance > gain: > > hg perfindex > before: ! wall 0.032023 comb 0.040000 user 0.000000 sys 0.040000 (best of > 100) > after: ! wall 0.000196 comb 0.000000 user 0.000000 sys 0.000000 (best of > 1060) > > The speed boost benefit all cases, including the one where the full index > needs to be parsed. > > hg perfindex --rev 0 > before: ! wall 0.040673 comb 0.030000 user 0.000000 sys 0.030000 (best of > 100) > after ! wall 0.010713 comb 0.020000 user 0.010000 sys 0.010000 (best of > 212) > > This gain reflect in higher level operation: > > hg perfbookmarks --clear-revlogs > before: ! wall 0.161339 comb 0.160000 user 0.130000 sys 0.030000 (best of > 56) > after: ! wall 0.123228 comb 0.120000 user 0.120000 sys 0.000000 (best of > 68) > I can confirm that it is a required config option on large repos and result in similar speedups. Looks good to me, queued the series. Many thanks!
On Sun, 02 Dec 2018 17:17:43 +0100, Boris Feld wrote: > # HG changeset patch > # User Boris Feld <boris.feld@octobus.net> > # Date 1542949784 -3600 > # Fri Nov 23 06:09:44 2018 +0100 > # Node ID 9708243274585f9544c70925eb0b0fa0ec7aba4f > # Parent 0fff68dfbe48d87dce8b8736c0347ed5aa79030e > # EXP-Topic mmap > # Available At https://bitbucket.org/octobus/mercurial-devel/ > # hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 970824327458 > mmapindex: set default to 1MB Can you check if strip/rollback properly copy the revlog before truncating it? If a mmapped revlog is truncated by another process, the mapped memory could be invalid. The worst case, the process would be killed by SIGBUS.
On 04/12/2018 12:09, Yuya Nishihara wrote: > On Sun, 02 Dec 2018 17:17:43 +0100, Boris Feld wrote: >> # HG changeset patch >> # User Boris Feld <boris.feld@octobus.net> >> # Date 1542949784 -3600 >> # Fri Nov 23 06:09:44 2018 +0100 >> # Node ID 9708243274585f9544c70925eb0b0fa0ec7aba4f >> # Parent 0fff68dfbe48d87dce8b8736c0347ed5aa79030e >> # EXP-Topic mmap >> # Available At https://bitbucket.org/octobus/mercurial-devel/ >> # hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 970824327458 >> mmapindex: set default to 1MB > Can you check if strip/rollback properly copy the revlog before truncating it? > > If a mmapped revlog is truncated by another process, the mapped memory could be > invalid. The worst case, the process would be killed by SIGBUS. Hum good catch, process reading a repository being stripped have always been up for troubles. However, mmap makes it worse by raising a signal instead of just having wonky seek. It also introduces new cases where this can happen. What shall we do here, I guess our best bet is to intercept these SIGBUS when reading revlog index? > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@mercurial-scm.org > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
On Wed, 2 Jan 2019 23:40:11 +0100, Boris FELD wrote: > On 04/12/2018 12:09, Yuya Nishihara wrote: > > On Sun, 02 Dec 2018 17:17:43 +0100, Boris Feld wrote: > >> # HG changeset patch > >> # User Boris Feld <boris.feld@octobus.net> > >> # Date 1542949784 -3600 > >> # Fri Nov 23 06:09:44 2018 +0100 > >> # Node ID 9708243274585f9544c70925eb0b0fa0ec7aba4f > >> # Parent 0fff68dfbe48d87dce8b8736c0347ed5aa79030e > >> # EXP-Topic mmap > >> # Available At https://bitbucket.org/octobus/mercurial-devel/ > >> # hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 970824327458 > >> mmapindex: set default to 1MB > > Can you check if strip/rollback properly copy the revlog before truncating it? > > > > If a mmapped revlog is truncated by another process, the mapped memory could be > > invalid. The worst case, the process would be killed by SIGBUS. > > Hum good catch, process reading a repository being stripped have always > been up for troubles. However, mmap makes it worse by raising a signal > instead of just having wonky seek. It also introduces new cases where > this can happen. mmap isn't worse because of SIGBUS, but because the index data can be updated after the index length is determined. Before, a single in-memory revlog index was at least consistent. > What shall we do here, I guess our best bet is to intercept these SIGBUS > when reading revlog index? I don't think it'll be easy to handle SIGBUS properly. And SIGBUS won't always be raised. Perhaps, the easiest solution is to copy the revlog index before strip/rollback. IIRC, the mmap implementation was highly experimental. I don't know if it's widely tested other than in FB where strip is never used.
On 03/01/2019 09:58, Yuya Nishihara wrote: > On Wed, 2 Jan 2019 23:40:11 +0100, Boris FELD wrote: >> On 04/12/2018 12:09, Yuya Nishihara wrote: >>> On Sun, 02 Dec 2018 17:17:43 +0100, Boris Feld wrote: >>>> # HG changeset patch >>>> # User Boris Feld <boris.feld@octobus.net> >>>> # Date 1542949784 -3600 >>>> # Fri Nov 23 06:09:44 2018 +0100 >>>> # Node ID 9708243274585f9544c70925eb0b0fa0ec7aba4f >>>> # Parent 0fff68dfbe48d87dce8b8736c0347ed5aa79030e >>>> # EXP-Topic mmap >>>> # Available At https://bitbucket.org/octobus/mercurial-devel/ >>>> # hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 970824327458 >>>> mmapindex: set default to 1MB >>> Can you check if strip/rollback properly copy the revlog before truncating it? >>> >>> If a mmapped revlog is truncated by another process, the mapped memory could be >>> invalid. The worst case, the process would be killed by SIGBUS. >> Hum good catch, process reading a repository being stripped have always >> been up for troubles. However, mmap makes it worse by raising a signal >> instead of just having wonky seek. It also introduces new cases where >> this can happen. > mmap isn't worse because of SIGBUS, but because the index data can be updated > after the index length is determined. Before, a single in-memory revlog index > was at least consistent. > >> What shall we do here, I guess our best bet is to intercept these SIGBUS >> when reading revlog index? Yes, but it would be inconsistent with the data it was pointing to. Access to this data would result in error too. The new thing is that we can get SIGBUS while accessing the index data themselves, as you are pointing out. > I don't think it'll be easy to handle SIGBUS properly. And SIGBUS won't always > be raised. Perhaps, the easiest solution is to copy the revlog index before > strip/rollback. I'm afraid at the performance impact, we are talking of potentially hundreds of MB of index data to be rolled back. Maybe we can keep the current truncation in normal transaction rollback and use the slower copies for the hg strip command itself (and rewrite)? However, I'm afraid we need to come up with a solution for mmap as it would be useful to use it more and more. Maybe we can come up with something catching the SIGBUS? Or maybe we need to never truncate files and keep an alternative way to track the maximum offset? Any other ideas? > > IIRC, the mmap implementation was highly experimental. I don't know if it's > widely tested other than in FB where strip is never used. We have been using it internally, and one of our clients deployed it too. It results in significant speed and memory improvement. > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@mercurial-scm.org > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
On Thu, Jan 3, 2019 at 2:37 PM Yuya Nishihara <yuya@tcha.org> wrote: > On Wed, 2 Jan 2019 23:40:11 +0100, Boris FELD wrote: > > On 04/12/2018 12:09, Yuya Nishihara wrote: > > > On Sun, 02 Dec 2018 17:17:43 +0100, Boris Feld wrote: > > >> # HG changeset patch > > >> # User Boris Feld <boris.feld@octobus.net> > > >> # Date 1542949784 -3600 > > >> # Fri Nov 23 06:09:44 2018 +0100 > > >> # Node ID 9708243274585f9544c70925eb0b0fa0ec7aba4f > > >> # Parent 0fff68dfbe48d87dce8b8736c0347ed5aa79030e > > >> # EXP-Topic mmap > > >> # Available At https://bitbucket.org/octobus/mercurial-devel/ > > >> # hg pull https://bitbucket.org/octobus/mercurial-devel/ > -r 970824327458 > > >> mmapindex: set default to 1MB > > > Can you check if strip/rollback properly copy the revlog before > truncating it? > > > > > > If a mmapped revlog is truncated by another process, the mapped memory > could be > > > invalid. The worst case, the process would be killed by SIGBUS. > > > > Hum good catch, process reading a repository being stripped have always > > been up for troubles. However, mmap makes it worse by raising a signal > > instead of just having wonky seek. It also introduces new cases where > > this can happen. > > mmap isn't worse because of SIGBUS, but because the index data can be > updated > after the index length is determined. Before, a single in-memory revlog > index > was at least consistent. > > > What shall we do here, I guess our best bet is to intercept these SIGBUS > > when reading revlog index? > > I don't think it'll be easy to handle SIGBUS properly. And SIGBUS won't > always > be raised. Perhaps, the easiest solution is to copy the revlog index before > strip/rollback. > > IIRC, the mmap implementation was highly experimental. I don't know if it's > widely tested other than in FB where strip is never used. > We did use mmap implementation and strip both for some time and didn't faced any issues except strip itself being an issue for slowdowns. However our use was limited.
On Mon, 7 Jan 2019 09:45:32 +0100, Boris FELD wrote: > On 03/01/2019 09:58, Yuya Nishihara wrote: > > On Wed, 2 Jan 2019 23:40:11 +0100, Boris FELD wrote: > >> On 04/12/2018 12:09, Yuya Nishihara wrote: > >>> On Sun, 02 Dec 2018 17:17:43 +0100, Boris Feld wrote: > >>>> # HG changeset patch > >>>> # User Boris Feld <boris.feld@octobus.net> > >>>> # Date 1542949784 -3600 > >>>> # Fri Nov 23 06:09:44 2018 +0100 > >>>> # Node ID 9708243274585f9544c70925eb0b0fa0ec7aba4f > >>>> # Parent 0fff68dfbe48d87dce8b8736c0347ed5aa79030e > >>>> # EXP-Topic mmap > >>>> # Available At https://bitbucket.org/octobus/mercurial-devel/ > >>>> # hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 970824327458 > >>>> mmapindex: set default to 1MB > >>> Can you check if strip/rollback properly copy the revlog before truncating it? > >>> > >>> If a mmapped revlog is truncated by another process, the mapped memory could be > >>> invalid. The worst case, the process would be killed by SIGBUS. > >> Hum good catch, process reading a repository being stripped have always > >> been up for troubles. However, mmap makes it worse by raising a signal > >> instead of just having wonky seek. It also introduces new cases where > >> this can happen. > > mmap isn't worse because of SIGBUS, but because the index data can be updated > > after the index length is determined. Before, a single in-memory revlog index > > was at least consistent. > > > >> What shall we do here, I guess our best bet is to intercept these SIGBUS > >> when reading revlog index? > Yes, but it would be inconsistent with the data it was pointing to. > Access to this data would result in error too. Correct. > The new thing is that we > can get SIGBUS while accessing the index data themselves, as you are > pointing out. Another new thing is that truncated revisions can be seen as valid changesets of '000...' hash with 0-length text. If the whole (or maybe dozens of) revisions were truncated, SIGBUS would be raised. In other cases, the truncated part would probably be read as zeros. > > I don't think it'll be easy to handle SIGBUS properly. And SIGBUS won't always > > be raised. Perhaps, the easiest solution is to copy the revlog index before > > strip/rollback. > > I'm afraid at the performance impact, we are talking of potentially > hundreds of MB of index data to be rolled back. > > Maybe we can keep the current truncation in normal transaction rollback > and use the slower copies for the hg strip command itself (and rewrite)? > > However, I'm afraid we need to come up with a solution for mmap as it > would be useful to use it more and more. > > Maybe we can come up with something catching the SIGBUS? Or maybe we > need to never truncate files and keep an alternative way to track the > maximum offset? Any other ideas? I've no idea. My point is that catching SIGBUS wouldn't save us from many possible failures. > > IIRC, the mmap implementation was highly experimental. I don't know if it's > > widely tested other than in FB where strip is never used. > We have been using it internally, and one of our clients deployed it > too. It results in significant speed and memory improvement. Yup. I'm just afraid of enabling it by default.
On 08/01/2019 15:41, Yuya Nishihara wrote: > On Mon, 7 Jan 2019 09:45:32 +0100, Boris FELD wrote: >> On 03/01/2019 09:58, Yuya Nishihara wrote: >>> On Wed, 2 Jan 2019 23:40:11 +0100, Boris FELD wrote: >>>> On 04/12/2018 12:09, Yuya Nishihara wrote: >>>>> On Sun, 02 Dec 2018 17:17:43 +0100, Boris Feld wrote: >>>>>> # HG changeset patch >>>>>> # User Boris Feld <boris.feld@octobus.net> >>>>>> # Date 1542949784 -3600 >>>>>> # Fri Nov 23 06:09:44 2018 +0100 >>>>>> # Node ID 9708243274585f9544c70925eb0b0fa0ec7aba4f >>>>>> # Parent 0fff68dfbe48d87dce8b8736c0347ed5aa79030e >>>>>> # EXP-Topic mmap >>>>>> # Available At https://bitbucket.org/octobus/mercurial-devel/ >>>>>> # hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 970824327458 >>>>>> mmapindex: set default to 1MB >>>>> Can you check if strip/rollback properly copy the revlog before truncating it? >>>>> >>>>> If a mmapped revlog is truncated by another process, the mapped memory could be >>>>> invalid. The worst case, the process would be killed by SIGBUS. >>>> Hum good catch, process reading a repository being stripped have always >>>> been up for troubles. However, mmap makes it worse by raising a signal >>>> instead of just having wonky seek. It also introduces new cases where >>>> this can happen. >>> mmap isn't worse because of SIGBUS, but because the index data can be updated >>> after the index length is determined. Before, a single in-memory revlog index >>> was at least consistent. >>> >>>> What shall we do here, I guess our best bet is to intercept these SIGBUS >>>> when reading revlog index? >> Yes, but it would be inconsistent with the data it was pointing to. >> Access to this data would result in error too. > Correct. > >> The new thing is that we >> can get SIGBUS while accessing the index data themselves, as you are >> pointing out. > Another new thing is that truncated revisions can be seen as valid changesets > of '000...' hash with 0-length text. If the whole (or maybe dozens of) > revisions were truncated, SIGBUS would be raised. In other cases, the truncated > part would probably be read as zeros. > >>> I don't think it'll be easy to handle SIGBUS properly. And SIGBUS won't always >>> be raised. Perhaps, the easiest solution is to copy the revlog index before >>> strip/rollback. >> I'm afraid at the performance impact, we are talking of potentially >> hundreds of MB of index data to be rolled back. >> >> Maybe we can keep the current truncation in normal transaction rollback >> and use the slower copies for the hg strip command itself (and rewrite)? >> >> However, I'm afraid we need to come up with a solution for mmap as it >> would be useful to use it more and more. >> >> Maybe we can come up with something catching the SIGBUS? Or maybe we >> need to never truncate files and keep an alternative way to track the >> maximum offset? Any other ideas? > I've no idea. My point is that catching SIGBUS wouldn't save us from many > possible failures. > >>> IIRC, the mmap implementation was highly experimental. I don't know if it's >>> widely tested other than in FB where strip is never used. >> We have been using it internally, and one of our clients deployed it >> too. It results in significant speed and memory improvement. > Yup. I'm just afraid of enabling it by default. Ok, what do you think we should do for 4.9? > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@mercurial-scm.org > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
On Thu, Jan 10, 2019 at 9:03 AM Boris FELD <boris.feld@octobus.net> wrote: > On 08/01/2019 15:41, Yuya Nishihara wrote: > > On Mon, 7 Jan 2019 09:45:32 +0100, Boris FELD wrote: > >> On 03/01/2019 09:58, Yuya Nishihara wrote: > >>> On Wed, 2 Jan 2019 23:40:11 +0100, Boris FELD wrote: > >>>> On 04/12/2018 12:09, Yuya Nishihara wrote: > >>>>> On Sun, 02 Dec 2018 17:17:43 +0100, Boris Feld wrote: > >>>>>> # HG changeset patch > >>>>>> # User Boris Feld <boris.feld@octobus.net> > >>>>>> # Date 1542949784 -3600 > >>>>>> # Fri Nov 23 06:09:44 2018 +0100 > >>>>>> # Node ID 9708243274585f9544c70925eb0b0fa0ec7aba4f > >>>>>> # Parent 0fff68dfbe48d87dce8b8736c0347ed5aa79030e > >>>>>> # EXP-Topic mmap > >>>>>> # Available At https://bitbucket.org/octobus/mercurial-devel/ > >>>>>> # hg pull > https://bitbucket.org/octobus/mercurial-devel/ -r 970824327458 > >>>>>> mmapindex: set default to 1MB > >>>>> Can you check if strip/rollback properly copy the revlog before > truncating it? > >>>>> > >>>>> If a mmapped revlog is truncated by another process, the mapped > memory could be > >>>>> invalid. The worst case, the process would be killed by SIGBUS. > >>>> Hum good catch, process reading a repository being stripped have > always > >>>> been up for troubles. However, mmap makes it worse by raising a signal > >>>> instead of just having wonky seek. It also introduces new cases where > >>>> this can happen. > >>> mmap isn't worse because of SIGBUS, but because the index data can be > updated > >>> after the index length is determined. Before, a single in-memory > revlog index > >>> was at least consistent. > >>> > >>>> What shall we do here, I guess our best bet is to intercept these > SIGBUS > >>>> when reading revlog index? > >> Yes, but it would be inconsistent with the data it was pointing to. > >> Access to this data would result in error too. > > Correct. > > > >> The new thing is that we > >> can get SIGBUS while accessing the index data themselves, as you are > >> pointing out. > > Another new thing is that truncated revisions can be seen as valid > changesets > > of '000...' hash with 0-length text. If the whole (or maybe dozens of) > > revisions were truncated, SIGBUS would be raised. In other cases, the > truncated > > part would probably be read as zeros. > > > >>> I don't think it'll be easy to handle SIGBUS properly. And SIGBUS > won't always > >>> be raised. Perhaps, the easiest solution is to copy the revlog index > before > >>> strip/rollback. > >> I'm afraid at the performance impact, we are talking of potentially > >> hundreds of MB of index data to be rolled back. > >> > >> Maybe we can keep the current truncation in normal transaction rollback > >> and use the slower copies for the hg strip command itself (and rewrite)? > >> > >> However, I'm afraid we need to come up with a solution for mmap as it > >> would be useful to use it more and more. > >> > >> Maybe we can come up with something catching the SIGBUS? Or maybe we > >> need to never truncate files and keep an alternative way to track the > >> maximum offset? Any other ideas? > > I've no idea. My point is that catching SIGBUS wouldn't save us from many > > possible failures. > > > >>> IIRC, the mmap implementation was highly experimental. I don't know if > it's > >>> widely tested other than in FB where strip is never used. > >> We have been using it internally, and one of our clients deployed it > >> too. It results in significant speed and memory improvement. > > Yup. I'm just afraid of enabling it by default. > > Ok, what do you think we should do for 4.9? > We're introducing a new repo requirement in 4.9 for sparse revlogs. I believe this strip problem (along with a ton of other problems) goes away if we have reader locks on repos. Since we are already introducing a new repo requirement for new repos, we can introduce yet more repo requirements without any additional significant user inconvenience. I know it is a stretch since we only have a few days left before the 4.9 RC, but is anybody will to implement reader locks (or some other repo mutual exclusion mechanism that could mitigate the strip problem)? Of course, reader locks break the ability to open a repository on a read-only filesystem. So, we shouldn't use them lightly! But we could do something like write a repo/transaction "generation number" into .hg/store and read it to detect repo mutations before certain operations. We could potentially also have "mmap mode" require writing a file, socket file, etc into .hg/store and have any writer refuse to strip if there is an active reader doing mmap. That's a lot of random ideas, I know. But I think that in order to enable mmap by default, we need to lock out mutations on mmap'd file segments in order to prevent crashes. This seemingly requires a reader lock or transaction changes of some form.
On 11/01/2019 01:05, Gregory Szorc wrote: > On Thu, Jan 10, 2019 at 9:03 AM Boris FELD <boris.feld@octobus.net > <mailto:boris.feld@octobus.net>> wrote: > > On 08/01/2019 15:41, Yuya Nishihara wrote: > > On Mon, 7 Jan 2019 09:45:32 +0100, Boris FELD wrote: > >> On 03/01/2019 09:58, Yuya Nishihara wrote: > >>> On Wed, 2 Jan 2019 23:40:11 +0100, Boris FELD wrote: > >>>> On 04/12/2018 12:09, Yuya Nishihara wrote: > >>>>> On Sun, 02 Dec 2018 17:17:43 +0100, Boris Feld wrote: > >>>>>> # HG changeset patch > >>>>>> # User Boris Feld <boris.feld@octobus.net > <mailto:boris.feld@octobus.net>> > >>>>>> # Date 1542949784 -3600 > >>>>>> # Fri Nov 23 06:09:44 2018 +0100 > >>>>>> # Node ID 9708243274585f9544c70925eb0b0fa0ec7aba4f > >>>>>> # Parent 0fff68dfbe48d87dce8b8736c0347ed5aa79030e > >>>>>> # EXP-Topic mmap > >>>>>> # Available At https://bitbucket.org/octobus/mercurial-devel/ > >>>>>> # hg pull > https://bitbucket.org/octobus/mercurial-devel/ -r 970824327458 > >>>>>> mmapindex: set default to 1MB > >>>>> Can you check if strip/rollback properly copy the revlog > before truncating it? > >>>>> > >>>>> If a mmapped revlog is truncated by another process, the > mapped memory could be > >>>>> invalid. The worst case, the process would be killed by SIGBUS. > >>>> Hum good catch, process reading a repository being stripped > have always > >>>> been up for troubles. However, mmap makes it worse by raising > a signal > >>>> instead of just having wonky seek. It also introduces new > cases where > >>>> this can happen. > >>> mmap isn't worse because of SIGBUS, but because the index data > can be updated > >>> after the index length is determined. Before, a single > in-memory revlog index > >>> was at least consistent. > >>> > >>>> What shall we do here, I guess our best bet is to intercept > these SIGBUS > >>>> when reading revlog index? > >> Yes, but it would be inconsistent with the data it was pointing to. > >> Access to this data would result in error too. > > Correct. > > > >> The new thing is that we > >> can get SIGBUS while accessing the index data themselves, as > you are > >> pointing out. > > Another new thing is that truncated revisions can be seen as > valid changesets > > of '000...' hash with 0-length text. If the whole (or maybe > dozens of) > > revisions were truncated, SIGBUS would be raised. In other > cases, the truncated > > part would probably be read as zeros. > > > >>> I don't think it'll be easy to handle SIGBUS properly. And > SIGBUS won't always > >>> be raised. Perhaps, the easiest solution is to copy the revlog > index before > >>> strip/rollback. > >> I'm afraid at the performance impact, we are talking of potentially > >> hundreds of MB of index data to be rolled back. > >> > >> Maybe we can keep the current truncation in normal transaction > rollback > >> and use the slower copies for the hg strip command itself (and > rewrite)? > >> > >> However, I'm afraid we need to come up with a solution for mmap > as it > >> would be useful to use it more and more. > >> > >> Maybe we can come up with something catching the SIGBUS? Or > maybe we > >> need to never truncate files and keep an alternative way to > track the > >> maximum offset? Any other ideas? > > I've no idea. My point is that catching SIGBUS wouldn't save us > from many > > possible failures. > > > >>> IIRC, the mmap implementation was highly experimental. I don't > know if it's > >>> widely tested other than in FB where strip is never used. > >> We have been using it internally, and one of our clients > deployed it > >> too. It results in significant speed and memory improvement. > > Yup. I'm just afraid of enabling it by default. > > Ok, what do you think we should do for 4.9? > > > We're introducing a new repo requirement in 4.9 for sparse revlogs. We are not introducing a new repo requirement in 4.9 for sparse-revlogs. The repo-requirements and its guarantees have been introduced in 4.7. > I believe this strip problem (along with a ton of other problems) goes > away if we have reader locks on repos. > > Since we are already introducing a new repo requirement for new repos, > we can introduce yet more repo requirements without any additional > significant user inconvenience. (except we are not introducing a new requirement) > > I know it is a stretch since we only have a few days left before the > 4.9 RC, but is anybody will to implement reader locks (or some other > repo mutual exclusion mechanism that could mitigate the strip > problem)? Of course, reader locks break the ability to open a > repository on a read-only filesystem. So, we shouldn't use them > lightly! But we could do something like write a repo/transaction > "generation number" into .hg/store and read it to detect repo > mutations before certain operations. We could potentially also have > "mmap mode" require writing a file, socket file, etc into .hg/store > and have any writer refuse to strip if there is an active reader doing > mmap. > > That's a lot of random ideas, I know. But I think that in order to > enable mmap by default, we need to lock out mutations on mmap'd file > segments in order to prevent crashes. This seemingly requires a reader > lock or transaction changes of some form. We are a week away from the freeze and reader locks are neither written nor tested. In addition, reader locks solve the strip issue but come with their own set of troubles, the first one being that they require write access. I don't think it is realistic to use reader lock to solve the mmap question right now. Having a new repository format to handle better handle transaction is something we will need, but also a significant amount of work and testing. I would rather focus on having a "graceful" way to catch SIGBUS (and potential other errors that Yuya pointed out). The mmap function has been tested in multiple places without troubles, and it raises large benefit. Just consolidating the potential crashing corner case seems good enough to get it shipped to all. We can spend more time later to narrow these corner case if they prove troublesome. In all case, we have the easy fallback of disabling it again in case of need, it does not affect the on-disk format. Yuya, what do you think? > > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@mercurial-scm.org > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
On Fri, 11 Jan 2019 09:59:49 +0100, Boris FELD wrote: > > On 11/01/2019 01:05, Gregory Szorc wrote: > > On Thu, Jan 10, 2019 at 9:03 AM Boris FELD <boris.feld@octobus.net > > <mailto:boris.feld@octobus.net>> wrote: > > > > On 08/01/2019 15:41, Yuya Nishihara wrote: > > > On Mon, 7 Jan 2019 09:45:32 +0100, Boris FELD wrote: > > >> On 03/01/2019 09:58, Yuya Nishihara wrote: > > >>> On Wed, 2 Jan 2019 23:40:11 +0100, Boris FELD wrote: > > >>>> On 04/12/2018 12:09, Yuya Nishihara wrote: > > >>>>> On Sun, 02 Dec 2018 17:17:43 +0100, Boris Feld wrote: > > >>>>>> # HG changeset patch > > >>>>>> # User Boris Feld <boris.feld@octobus.net > > <mailto:boris.feld@octobus.net>> > > >>>>>> # Date 1542949784 -3600 > > >>>>>> # Fri Nov 23 06:09:44 2018 +0100 > > >>>>>> # Node ID 9708243274585f9544c70925eb0b0fa0ec7aba4f > > >>>>>> # Parent 0fff68dfbe48d87dce8b8736c0347ed5aa79030e > > >>>>>> # EXP-Topic mmap > > >>>>>> # Available At https://bitbucket.org/octobus/mercurial-devel/ > > >>>>>> # hg pull > > https://bitbucket.org/octobus/mercurial-devel/ -r 970824327458 > > >>>>>> mmapindex: set default to 1MB > > >>>>> Can you check if strip/rollback properly copy the revlog > > before truncating it? > > >>>>> > > >>>>> If a mmapped revlog is truncated by another process, the > > mapped memory could be > > >>>>> invalid. The worst case, the process would be killed by SIGBUS. > > >>>> Hum good catch, process reading a repository being stripped > > have always > > >>>> been up for troubles. However, mmap makes it worse by raising > > a signal > > >>>> instead of just having wonky seek. It also introduces new > > cases where > > >>>> this can happen. > > >>> mmap isn't worse because of SIGBUS, but because the index data > > can be updated > > >>> after the index length is determined. Before, a single > > in-memory revlog index > > >>> was at least consistent. > > >>> > > >>>> What shall we do here, I guess our best bet is to intercept > > these SIGBUS > > >>>> when reading revlog index? > > >> Yes, but it would be inconsistent with the data it was pointing to. > > >> Access to this data would result in error too. > > > Correct. > > > > > >> The new thing is that we > > >> can get SIGBUS while accessing the index data themselves, as > > you are > > >> pointing out. > > > Another new thing is that truncated revisions can be seen as > > valid changesets > > > of '000...' hash with 0-length text. If the whole (or maybe > > dozens of) > > > revisions were truncated, SIGBUS would be raised. In other > > cases, the truncated > > > part would probably be read as zeros. > > > > > >>> I don't think it'll be easy to handle SIGBUS properly. And > > SIGBUS won't always > > >>> be raised. Perhaps, the easiest solution is to copy the revlog > > index before > > >>> strip/rollback. > > >> I'm afraid at the performance impact, we are talking of potentially > > >> hundreds of MB of index data to be rolled back. > > >> > > >> Maybe we can keep the current truncation in normal transaction > > rollback > > >> and use the slower copies for the hg strip command itself (and > > rewrite)? > > >> > > >> However, I'm afraid we need to come up with a solution for mmap > > as it > > >> would be useful to use it more and more. > > >> > > >> Maybe we can come up with something catching the SIGBUS? Or > > maybe we > > >> need to never truncate files and keep an alternative way to > > track the > > >> maximum offset? Any other ideas? > > > I've no idea. My point is that catching SIGBUS wouldn't save us > > from many > > > possible failures. > > > > > >>> IIRC, the mmap implementation was highly experimental. I don't > > know if it's > > >>> widely tested other than in FB where strip is never used. > > >> We have been using it internally, and one of our clients > > deployed it > > >> too. It results in significant speed and memory improvement. > > > Yup. I'm just afraid of enabling it by default. > > > > Ok, what do you think we should do for 4.9? > > > > > > We're introducing a new repo requirement in 4.9 for sparse revlogs. > We are not introducing a new repo requirement in 4.9 for sparse-revlogs. > The repo-requirements and its guarantees have been introduced in 4.7. > > I believe this strip problem (along with a ton of other problems) goes > > away if we have reader locks on repos. > > > > Since we are already introducing a new repo requirement for new repos, > > we can introduce yet more repo requirements without any additional > > significant user inconvenience. > (except we are not introducing a new requirement) > > > > I know it is a stretch since we only have a few days left before the > > 4.9 RC, but is anybody will to implement reader locks (or some other > > repo mutual exclusion mechanism that could mitigate the strip > > problem)? Of course, reader locks break the ability to open a > > repository on a read-only filesystem. So, we shouldn't use them > > lightly! But we could do something like write a repo/transaction > > "generation number" into .hg/store and read it to detect repo > > mutations before certain operations. We could potentially also have > > "mmap mode" require writing a file, socket file, etc into .hg/store > > and have any writer refuse to strip if there is an active reader doing > > mmap. > > > > That's a lot of random ideas, I know. But I think that in order to > > enable mmap by default, we need to lock out mutations on mmap'd file > > segments in order to prevent crashes. This seemingly requires a reader > > lock or transaction changes of some form. > We are a week away from the freeze and reader locks are neither written > nor tested. In addition, reader locks solve the strip issue but come > with their own set of troubles, the first one being that they require > write access. > > I don't think it is realistic to use reader lock to solve the mmap > question right now. Having a new repository format to handle better > handle transaction is something we will need, but also a significant > amount of work and testing. Agreed. I don't think reader locks would be implemented correctly in 5 days. It'll require more careful inspection given our storage codes splattered around various modules. I also think mmap is in the same boat. It isn't a drop-in replacement for read()/write(). IMHO, we should carefully design the data structure and the operations so that mmap-ed data are reliably processed. > I would rather focus on having a "graceful" way to catch SIGBUS (and > potential other errors that Yuya pointed out). The mmap function has > been tested in multiple places without troubles, and it raises large > benefit. Just consolidating the potential crashing corner case seems > good enough to get it shipped to all. We can spend more time later to > narrow these corner case if they prove troublesome. > > In all case, we have the easy fallback of disabling it again in case of > need, it does not affect the on-disk format. > > Yuya, what do you think? My vote is to not enable the mmap by default. I'm okay to move the option out of the experimental so it gets more reachable by experienced users. FWIW, it appears not documented.
On 11/01/2019 15:46, Yuya Nishihara wrote: > On Fri, 11 Jan 2019 09:59:49 +0100, Boris FELD wrote: >> On 11/01/2019 01:05, Gregory Szorc wrote: >>> On Thu, Jan 10, 2019 at 9:03 AM Boris FELD <boris.feld@octobus.net >>> <mailto:boris.feld@octobus.net>> wrote: >>> >>> On 08/01/2019 15:41, Yuya Nishihara wrote: >>> > On Mon, 7 Jan 2019 09:45:32 +0100, Boris FELD wrote: >>> >> On 03/01/2019 09:58, Yuya Nishihara wrote: >>> >>> On Wed, 2 Jan 2019 23:40:11 +0100, Boris FELD wrote: >>> >>>> On 04/12/2018 12:09, Yuya Nishihara wrote: >>> >>>>> On Sun, 02 Dec 2018 17:17:43 +0100, Boris Feld wrote: >>> >>>>>> # HG changeset patch >>> >>>>>> # User Boris Feld <boris.feld@octobus.net >>> <mailto:boris.feld@octobus.net>> >>> >>>>>> # Date 1542949784 -3600 >>> >>>>>> # Fri Nov 23 06:09:44 2018 +0100 >>> >>>>>> # Node ID 9708243274585f9544c70925eb0b0fa0ec7aba4f >>> >>>>>> # Parent 0fff68dfbe48d87dce8b8736c0347ed5aa79030e >>> >>>>>> # EXP-Topic mmap >>> >>>>>> # Available At https://bitbucket.org/octobus/mercurial-devel/ >>> >>>>>> # hg pull >>> https://bitbucket.org/octobus/mercurial-devel/ -r 970824327458 >>> >>>>>> mmapindex: set default to 1MB >>> >>>>> Can you check if strip/rollback properly copy the revlog >>> before truncating it? >>> >>>>> >>> >>>>> If a mmapped revlog is truncated by another process, the >>> mapped memory could be >>> >>>>> invalid. The worst case, the process would be killed by SIGBUS. >>> >>>> Hum good catch, process reading a repository being stripped >>> have always >>> >>>> been up for troubles. However, mmap makes it worse by raising >>> a signal >>> >>>> instead of just having wonky seek. It also introduces new >>> cases where >>> >>>> this can happen. >>> >>> mmap isn't worse because of SIGBUS, but because the index data >>> can be updated >>> >>> after the index length is determined. Before, a single >>> in-memory revlog index >>> >>> was at least consistent. >>> >>> >>> >>>> What shall we do here, I guess our best bet is to intercept >>> these SIGBUS >>> >>>> when reading revlog index? >>> >> Yes, but it would be inconsistent with the data it was pointing to. >>> >> Access to this data would result in error too. >>> > Correct. >>> > >>> >> The new thing is that we >>> >> can get SIGBUS while accessing the index data themselves, as >>> you are >>> >> pointing out. >>> > Another new thing is that truncated revisions can be seen as >>> valid changesets >>> > of '000...' hash with 0-length text. If the whole (or maybe >>> dozens of) >>> > revisions were truncated, SIGBUS would be raised. In other >>> cases, the truncated >>> > part would probably be read as zeros. >>> > >>> >>> I don't think it'll be easy to handle SIGBUS properly. And >>> SIGBUS won't always >>> >>> be raised. Perhaps, the easiest solution is to copy the revlog >>> index before >>> >>> strip/rollback. >>> >> I'm afraid at the performance impact, we are talking of potentially >>> >> hundreds of MB of index data to be rolled back. >>> >> >>> >> Maybe we can keep the current truncation in normal transaction >>> rollback >>> >> and use the slower copies for the hg strip command itself (and >>> rewrite)? >>> >> >>> >> However, I'm afraid we need to come up with a solution for mmap >>> as it >>> >> would be useful to use it more and more. >>> >> >>> >> Maybe we can come up with something catching the SIGBUS? Or >>> maybe we >>> >> need to never truncate files and keep an alternative way to >>> track the >>> >> maximum offset? Any other ideas? >>> > I've no idea. My point is that catching SIGBUS wouldn't save us >>> from many >>> > possible failures. >>> > >>> >>> IIRC, the mmap implementation was highly experimental. I don't >>> know if it's >>> >>> widely tested other than in FB where strip is never used. >>> >> We have been using it internally, and one of our clients >>> deployed it >>> >> too. It results in significant speed and memory improvement. >>> > Yup. I'm just afraid of enabling it by default. >>> >>> Ok, what do you think we should do for 4.9? >>> >>> >>> We're introducing a new repo requirement in 4.9 for sparse revlogs. >> We are not introducing a new repo requirement in 4.9 for sparse-revlogs. >> The repo-requirements and its guarantees have been introduced in 4.7. >>> I believe this strip problem (along with a ton of other problems) goes >>> away if we have reader locks on repos. >>> >>> Since we are already introducing a new repo requirement for new repos, >>> we can introduce yet more repo requirements without any additional >>> significant user inconvenience. >> (except we are not introducing a new requirement) >>> I know it is a stretch since we only have a few days left before the >>> 4.9 RC, but is anybody will to implement reader locks (or some other >>> repo mutual exclusion mechanism that could mitigate the strip >>> problem)? Of course, reader locks break the ability to open a >>> repository on a read-only filesystem. So, we shouldn't use them >>> lightly! But we could do something like write a repo/transaction >>> "generation number" into .hg/store and read it to detect repo >>> mutations before certain operations. We could potentially also have >>> "mmap mode" require writing a file, socket file, etc into .hg/store >>> and have any writer refuse to strip if there is an active reader doing >>> mmap. >>> >>> That's a lot of random ideas, I know. But I think that in order to >>> enable mmap by default, we need to lock out mutations on mmap'd file >>> segments in order to prevent crashes. This seemingly requires a reader >>> lock or transaction changes of some form. >> We are a week away from the freeze and reader locks are neither written >> nor tested. In addition, reader locks solve the strip issue but come >> with their own set of troubles, the first one being that they require >> write access. >> >> I don't think it is realistic to use reader lock to solve the mmap >> question right now. Having a new repository format to handle better >> handle transaction is something we will need, but also a significant >> amount of work and testing. > Agreed. I don't think reader locks would be implemented correctly in 5 days. > It'll require more careful inspection given our storage codes splattered > around various modules. I also think mmap is in the same boat. It isn't > a drop-in replacement for read()/write(). IMHO, we should carefully design > the data structure and the operations so that mmap-ed data are reliably > processed. > >> I would rather focus on having a "graceful" way to catch SIGBUS (and >> potential other errors that Yuya pointed out). The mmap function has >> been tested in multiple places without troubles, and it raises large >> benefit. Just consolidating the potential crashing corner case seems >> good enough to get it shipped to all. We can spend more time later to >> narrow these corner case if they prove troublesome. >> >> In all case, we have the easy fallback of disabling it again in case of >> need, it does not affect the on-disk format. >> >> Yuya, what do you think? > My vote is to not enable the mmap by default. I'm okay to move the option > out of the experimental so it gets more reachable by experienced users. > FWIW, it appears not documented. Okay, so let's put mmaped indexes back to disabled by default for this release. The potential SIGBUS/zero issues pointed by Yuya seems serious enough. I'm tempted to move the config back to experimental too until we decide what final form the feature will take. Given the issue we need to solve we will probably have to change some of the read/write/truncates patterns and possibly the on-disk format. This will need a new requirement. The simplest option is probably to have strip/rollback copy the file over, but this will require careful performance testing. Moving mmap back to experimental means backing out 74a9f428227e and 875d2af8cb4e. > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@mercurial-scm.org > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
On Wed, 16 Jan 2019 15:23:57 +0100, Boris FELD wrote: > On 11/01/2019 15:46, Yuya Nishihara wrote: > > On Fri, 11 Jan 2019 09:59:49 +0100, Boris FELD wrote: > >> On 11/01/2019 01:05, Gregory Szorc wrote: > >>> On Thu, Jan 10, 2019 at 9:03 AM Boris FELD <boris.feld@octobus.net > >>> <mailto:boris.feld@octobus.net>> wrote: > >>> > >>> On 08/01/2019 15:41, Yuya Nishihara wrote: > >>> > On Mon, 7 Jan 2019 09:45:32 +0100, Boris FELD wrote: > >>> >> On 03/01/2019 09:58, Yuya Nishihara wrote: > >>> >>> On Wed, 2 Jan 2019 23:40:11 +0100, Boris FELD wrote: > >>> >>>> On 04/12/2018 12:09, Yuya Nishihara wrote: > >>> >>>>> On Sun, 02 Dec 2018 17:17:43 +0100, Boris Feld wrote: > >>> >>>>>> # HG changeset patch > >>> >>>>>> # User Boris Feld <boris.feld@octobus.net > >>> <mailto:boris.feld@octobus.net>> > >>> >>>>>> # Date 1542949784 -3600 > >>> >>>>>> # Fri Nov 23 06:09:44 2018 +0100 > >>> >>>>>> # Node ID 9708243274585f9544c70925eb0b0fa0ec7aba4f > >>> >>>>>> # Parent 0fff68dfbe48d87dce8b8736c0347ed5aa79030e > >>> >>>>>> # EXP-Topic mmap > >>> >>>>>> # Available At https://bitbucket.org/octobus/mercurial-devel/ > >>> >>>>>> # hg pull > >>> https://bitbucket.org/octobus/mercurial-devel/ -r 970824327458 > >>> >>>>>> mmapindex: set default to 1MB > >>> >>>>> Can you check if strip/rollback properly copy the revlog > >>> before truncating it? > >>> >>>>> > >>> >>>>> If a mmapped revlog is truncated by another process, the > >>> mapped memory could be > >>> >>>>> invalid. The worst case, the process would be killed by SIGBUS. > >>> >>>> Hum good catch, process reading a repository being stripped > >>> have always > >>> >>>> been up for troubles. However, mmap makes it worse by raising > >>> a signal > >>> >>>> instead of just having wonky seek. It also introduces new > >>> cases where > >>> >>>> this can happen. > >>> >>> mmap isn't worse because of SIGBUS, but because the index data > >>> can be updated > >>> >>> after the index length is determined. Before, a single > >>> in-memory revlog index > >>> >>> was at least consistent. > >>> >>> > >>> >>>> What shall we do here, I guess our best bet is to intercept > >>> these SIGBUS > >>> >>>> when reading revlog index? > >>> >> Yes, but it would be inconsistent with the data it was pointing to. > >>> >> Access to this data would result in error too. > >>> > Correct. > >>> > > >>> >> The new thing is that we > >>> >> can get SIGBUS while accessing the index data themselves, as > >>> you are > >>> >> pointing out. > >>> > Another new thing is that truncated revisions can be seen as > >>> valid changesets > >>> > of '000...' hash with 0-length text. If the whole (or maybe > >>> dozens of) > >>> > revisions were truncated, SIGBUS would be raised. In other > >>> cases, the truncated > >>> > part would probably be read as zeros. > >>> > > >>> >>> I don't think it'll be easy to handle SIGBUS properly. And > >>> SIGBUS won't always > >>> >>> be raised. Perhaps, the easiest solution is to copy the revlog > >>> index before > >>> >>> strip/rollback. > >>> >> I'm afraid at the performance impact, we are talking of potentially > >>> >> hundreds of MB of index data to be rolled back. > >>> >> > >>> >> Maybe we can keep the current truncation in normal transaction > >>> rollback > >>> >> and use the slower copies for the hg strip command itself (and > >>> rewrite)? > >>> >> > >>> >> However, I'm afraid we need to come up with a solution for mmap > >>> as it > >>> >> would be useful to use it more and more. > >>> >> > >>> >> Maybe we can come up with something catching the SIGBUS? Or > >>> maybe we > >>> >> need to never truncate files and keep an alternative way to > >>> track the > >>> >> maximum offset? Any other ideas? > >>> > I've no idea. My point is that catching SIGBUS wouldn't save us > >>> from many > >>> > possible failures. > >>> > > >>> >>> IIRC, the mmap implementation was highly experimental. I don't > >>> know if it's > >>> >>> widely tested other than in FB where strip is never used. > >>> >> We have been using it internally, and one of our clients > >>> deployed it > >>> >> too. It results in significant speed and memory improvement. > >>> > Yup. I'm just afraid of enabling it by default. > >>> > >>> Ok, what do you think we should do for 4.9? > >>> > >>> > >>> We're introducing a new repo requirement in 4.9 for sparse revlogs. > >> We are not introducing a new repo requirement in 4.9 for sparse-revlogs. > >> The repo-requirements and its guarantees have been introduced in 4.7. > >>> I believe this strip problem (along with a ton of other problems) goes > >>> away if we have reader locks on repos. > >>> > >>> Since we are already introducing a new repo requirement for new repos, > >>> we can introduce yet more repo requirements without any additional > >>> significant user inconvenience. > >> (except we are not introducing a new requirement) > >>> I know it is a stretch since we only have a few days left before the > >>> 4.9 RC, but is anybody will to implement reader locks (or some other > >>> repo mutual exclusion mechanism that could mitigate the strip > >>> problem)? Of course, reader locks break the ability to open a > >>> repository on a read-only filesystem. So, we shouldn't use them > >>> lightly! But we could do something like write a repo/transaction > >>> "generation number" into .hg/store and read it to detect repo > >>> mutations before certain operations. We could potentially also have > >>> "mmap mode" require writing a file, socket file, etc into .hg/store > >>> and have any writer refuse to strip if there is an active reader doing > >>> mmap. > >>> > >>> That's a lot of random ideas, I know. But I think that in order to > >>> enable mmap by default, we need to lock out mutations on mmap'd file > >>> segments in order to prevent crashes. This seemingly requires a reader > >>> lock or transaction changes of some form. > >> We are a week away from the freeze and reader locks are neither written > >> nor tested. In addition, reader locks solve the strip issue but come > >> with their own set of troubles, the first one being that they require > >> write access. > >> > >> I don't think it is realistic to use reader lock to solve the mmap > >> question right now. Having a new repository format to handle better > >> handle transaction is something we will need, but also a significant > >> amount of work and testing. > > Agreed. I don't think reader locks would be implemented correctly in 5 days. > > It'll require more careful inspection given our storage codes splattered > > around various modules. I also think mmap is in the same boat. It isn't > > a drop-in replacement for read()/write(). IMHO, we should carefully design > > the data structure and the operations so that mmap-ed data are reliably > > processed. > > > >> I would rather focus on having a "graceful" way to catch SIGBUS (and > >> potential other errors that Yuya pointed out). The mmap function has > >> been tested in multiple places without troubles, and it raises large > >> benefit. Just consolidating the potential crashing corner case seems > >> good enough to get it shipped to all. We can spend more time later to > >> narrow these corner case if they prove troublesome. > >> > >> In all case, we have the easy fallback of disabling it again in case of > >> need, it does not affect the on-disk format. > >> > >> Yuya, what do you think? > > My vote is to not enable the mmap by default. I'm okay to move the option > > out of the experimental so it gets more reachable by experienced users. > > FWIW, it appears not documented. > > Okay, so let's put mmaped indexes back to disabled by default for this > release. The potential SIGBUS/zero issues pointed by Yuya seems serious > enough. > > I'm tempted to move the config back to experimental too until we decide > what final form the feature will take. Given the issue we need to solve > we will probably have to change some of the read/write/truncates > patterns and possibly the on-disk format. This will need a new > requirement. The simplest option is probably to have strip/rollback copy > the file over, but this will require careful performance testing. > > Moving mmap back to experimental means backing out 74a9f428227e and > 875d2af8cb4e. Can you send backout patches? Matt Harbison found Windows issue. Perhaps, an mmapped revlog file would be locked while it's open, and couldn't be updated by another process. https://groups.google.com/d/msg/thg-dev/X7RLiEzSPm0/qiTMjDRNEAAJ
On 18/01/2019 13:32, Yuya Nishihara wrote: > On Wed, 16 Jan 2019 15:23:57 +0100, Boris FELD wrote: >> On 11/01/2019 15:46, Yuya Nishihara wrote: >>> On Fri, 11 Jan 2019 09:59:49 +0100, Boris FELD wrote: >>>> On 11/01/2019 01:05, Gregory Szorc wrote: >>>>> On Thu, Jan 10, 2019 at 9:03 AM Boris FELD <boris.feld@octobus.net >>>>> <mailto:boris.feld@octobus.net>> wrote: >>>>> >>>>> On 08/01/2019 15:41, Yuya Nishihara wrote: >>>>> > On Mon, 7 Jan 2019 09:45:32 +0100, Boris FELD wrote: >>>>> >> On 03/01/2019 09:58, Yuya Nishihara wrote: >>>>> >>> On Wed, 2 Jan 2019 23:40:11 +0100, Boris FELD wrote: >>>>> >>>> On 04/12/2018 12:09, Yuya Nishihara wrote: >>>>> >>>>> On Sun, 02 Dec 2018 17:17:43 +0100, Boris Feld wrote: >>>>> >>>>>> # HG changeset patch >>>>> >>>>>> # User Boris Feld <boris.feld@octobus.net >>>>> <mailto:boris.feld@octobus.net>> >>>>> >>>>>> # Date 1542949784 -3600 >>>>> >>>>>> # Fri Nov 23 06:09:44 2018 +0100 >>>>> >>>>>> # Node ID 9708243274585f9544c70925eb0b0fa0ec7aba4f >>>>> >>>>>> # Parent 0fff68dfbe48d87dce8b8736c0347ed5aa79030e >>>>> >>>>>> # EXP-Topic mmap >>>>> >>>>>> # Available At https://bitbucket.org/octobus/mercurial-devel/ >>>>> >>>>>> # hg pull >>>>> https://bitbucket.org/octobus/mercurial-devel/ -r 970824327458 >>>>> >>>>>> mmapindex: set default to 1MB >>>>> >>>>> Can you check if strip/rollback properly copy the revlog >>>>> before truncating it? >>>>> >>>>> >>>>> >>>>> If a mmapped revlog is truncated by another process, the >>>>> mapped memory could be >>>>> >>>>> invalid. The worst case, the process would be killed by SIGBUS. >>>>> >>>> Hum good catch, process reading a repository being stripped >>>>> have always >>>>> >>>> been up for troubles. However, mmap makes it worse by raising >>>>> a signal >>>>> >>>> instead of just having wonky seek. It also introduces new >>>>> cases where >>>>> >>>> this can happen. >>>>> >>> mmap isn't worse because of SIGBUS, but because the index data >>>>> can be updated >>>>> >>> after the index length is determined. Before, a single >>>>> in-memory revlog index >>>>> >>> was at least consistent. >>>>> >>> >>>>> >>>> What shall we do here, I guess our best bet is to intercept >>>>> these SIGBUS >>>>> >>>> when reading revlog index? >>>>> >> Yes, but it would be inconsistent with the data it was pointing to. >>>>> >> Access to this data would result in error too. >>>>> > Correct. >>>>> > >>>>> >> The new thing is that we >>>>> >> can get SIGBUS while accessing the index data themselves, as >>>>> you are >>>>> >> pointing out. >>>>> > Another new thing is that truncated revisions can be seen as >>>>> valid changesets >>>>> > of '000...' hash with 0-length text. If the whole (or maybe >>>>> dozens of) >>>>> > revisions were truncated, SIGBUS would be raised. In other >>>>> cases, the truncated >>>>> > part would probably be read as zeros. >>>>> > >>>>> >>> I don't think it'll be easy to handle SIGBUS properly. And >>>>> SIGBUS won't always >>>>> >>> be raised. Perhaps, the easiest solution is to copy the revlog >>>>> index before >>>>> >>> strip/rollback. >>>>> >> I'm afraid at the performance impact, we are talking of potentially >>>>> >> hundreds of MB of index data to be rolled back. >>>>> >> >>>>> >> Maybe we can keep the current truncation in normal transaction >>>>> rollback >>>>> >> and use the slower copies for the hg strip command itself (and >>>>> rewrite)? >>>>> >> >>>>> >> However, I'm afraid we need to come up with a solution for mmap >>>>> as it >>>>> >> would be useful to use it more and more. >>>>> >> >>>>> >> Maybe we can come up with something catching the SIGBUS? Or >>>>> maybe we >>>>> >> need to never truncate files and keep an alternative way to >>>>> track the >>>>> >> maximum offset? Any other ideas? >>>>> > I've no idea. My point is that catching SIGBUS wouldn't save us >>>>> from many >>>>> > possible failures. >>>>> > >>>>> >>> IIRC, the mmap implementation was highly experimental. I don't >>>>> know if it's >>>>> >>> widely tested other than in FB where strip is never used. >>>>> >> We have been using it internally, and one of our clients >>>>> deployed it >>>>> >> too. It results in significant speed and memory improvement. >>>>> > Yup. I'm just afraid of enabling it by default. >>>>> >>>>> Ok, what do you think we should do for 4.9? >>>>> >>>>> >>>>> We're introducing a new repo requirement in 4.9 for sparse revlogs. >>>> We are not introducing a new repo requirement in 4.9 for sparse-revlogs. >>>> The repo-requirements and its guarantees have been introduced in 4.7. >>>>> I believe this strip problem (along with a ton of other problems) goes >>>>> away if we have reader locks on repos. >>>>> >>>>> Since we are already introducing a new repo requirement for new repos, >>>>> we can introduce yet more repo requirements without any additional >>>>> significant user inconvenience. >>>> (except we are not introducing a new requirement) >>>>> I know it is a stretch since we only have a few days left before the >>>>> 4.9 RC, but is anybody will to implement reader locks (or some other >>>>> repo mutual exclusion mechanism that could mitigate the strip >>>>> problem)? Of course, reader locks break the ability to open a >>>>> repository on a read-only filesystem. So, we shouldn't use them >>>>> lightly! But we could do something like write a repo/transaction >>>>> "generation number" into .hg/store and read it to detect repo >>>>> mutations before certain operations. We could potentially also have >>>>> "mmap mode" require writing a file, socket file, etc into .hg/store >>>>> and have any writer refuse to strip if there is an active reader doing >>>>> mmap. >>>>> >>>>> That's a lot of random ideas, I know. But I think that in order to >>>>> enable mmap by default, we need to lock out mutations on mmap'd file >>>>> segments in order to prevent crashes. This seemingly requires a reader >>>>> lock or transaction changes of some form. >>>> We are a week away from the freeze and reader locks are neither written >>>> nor tested. In addition, reader locks solve the strip issue but come >>>> with their own set of troubles, the first one being that they require >>>> write access. >>>> >>>> I don't think it is realistic to use reader lock to solve the mmap >>>> question right now. Having a new repository format to handle better >>>> handle transaction is something we will need, but also a significant >>>> amount of work and testing. >>> Agreed. I don't think reader locks would be implemented correctly in 5 days. >>> It'll require more careful inspection given our storage codes splattered >>> around various modules. I also think mmap is in the same boat. It isn't >>> a drop-in replacement for read()/write(). IMHO, we should carefully design >>> the data structure and the operations so that mmap-ed data are reliably >>> processed. >>> >>>> I would rather focus on having a "graceful" way to catch SIGBUS (and >>>> potential other errors that Yuya pointed out). The mmap function has >>>> been tested in multiple places without troubles, and it raises large >>>> benefit. Just consolidating the potential crashing corner case seems >>>> good enough to get it shipped to all. We can spend more time later to >>>> narrow these corner case if they prove troublesome. >>>> >>>> In all case, we have the easy fallback of disabling it again in case of >>>> need, it does not affect the on-disk format. >>>> >>>> Yuya, what do you think? >>> My vote is to not enable the mmap by default. I'm okay to move the option >>> out of the experimental so it gets more reachable by experienced users. >>> FWIW, it appears not documented. >> Okay, so let's put mmaped indexes back to disabled by default for this >> release. The potential SIGBUS/zero issues pointed by Yuya seems serious >> enough. >> >> I'm tempted to move the config back to experimental too until we decide >> what final form the feature will take. Given the issue we need to solve >> we will probably have to change some of the read/write/truncates >> patterns and possibly the on-disk format. This will need a new >> requirement. The simplest option is probably to have strip/rollback copy >> the file over, but this will require careful performance testing. >> >> Moving mmap back to experimental means backing out 74a9f428227e and >> 875d2af8cb4e. > Can you send backout patches? > > Matt Harbison found Windows issue. Perhaps, an mmapped revlog file would > be locked while it's open, and couldn't be updated by another process. > > https://groups.google.com/d/msg/thg-dev/X7RLiEzSPm0/qiTMjDRNEAAJ Okay, series coming. > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@mercurial-scm.org > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Patch
diff --git a/mercurial/configitems.py b/mercurial/configitems.py --- a/mercurial/configitems.py +++ b/mercurial/configitems.py @@ -976,7 +976,7 @@ coreconfigitem('push', 'pushvars.server' default=False, ) coreconfigitem('storage', 'mmap-threshold', - default=None, + default='1MB', alias=[('experimental', 'mmapindexthreshold')], ) coreconfigitem('storage', 'new-repo-backend',