Skip to main content
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [jgit-dev] faster ref-updates where the update-type is known to be non-fast-forward
On 26 August 2013 16:33, Shawn Pearce <spearce@xxxxxxxxxxx> wrote:
On Sun, Aug 25, 2013 at 5:00 AM, Roberto Tyley <roberto.tyley@xxxxxxxxx> wrote:
> Use case: the BFG has rewritten the whole of a big repository's history, and
> needs to update the branch refs. Calling isMergedInto() can take the best
> part of a minute, and in fact it can crash the JVM with a stackoverflow
> (reported by Elliot Glaysher when using the BFG on Chromium's source).

A stack overflow during a revwalk is bad. We shouldn't do this. *sigh*

Although I was able to reproduce the stack overflow on Chromium's source (~200,000 commits - http://git.chromium.org/chromium/src.git), I must confess I didn't dig deep enough to establish exactly why it was happening. I did try running against a test repo of 300,000 empty consecutive commits, and while isMergedInto() was slow, it didn't crash. The stack overflow looked like this:

Exception in thread "main" java.lang.StackOverflowError
    at org.eclipse.jgit.revwalk.MergeBaseGenerator.carryOntoOne(MergeBaseGenerator.java:205)
    at org.eclipse.jgit.revwalk.MergeBaseGenerator.carryOntoHistory(MergeBaseGenerator.java:194)
    at org.eclipse.jgit.revwalk.MergeBaseGenerator.carryOntoHistory(MergeBaseGenerator.java:195)
Possibly Chromium's repo has a pathological number of merges or something- I'm afraid I concentrated on trying to avoid calling isMergedInto() at all.



> Although this isn't current behaviour, perhaps setting the ReceiveCommand
> update-type to UPDATE_NONFASTFORWARD before calling BatchRefUpdate.execute()
> should be enough to tell JGit that I know the update type, and don't want
> JGit to calculate it?

There isn't a clean way to do this sort of update, but there should
be. It is completely reasonable for a calling application to say it
has already vetted the update and just wants it to proceed, provided
the reference still exactly matches the expected old SHA-1 (if
supplied).

I thought it bypasses if the update-type is already set to
UPDATE_NONFASTFORWARD as you suggest, as we only run the check when
the type is the default of UPDATE.

I did try this, while one call to isMergedInto() is avoided, the other one still occurs in RefUpdate.updateImpl(), where there is no guard:


(it's also currently a bit fiddly as an external user to set the ReceiveCommand up in the correct state to do this, as ReceiveCommand.setType() is not public and the 4-arg constructor does not set status to Result.NOT_ATTEMPTED...  so you must call setResult(NOT_ATTEMPTED) manually before executing the command- obviously, trivial to fix).

I'd be happy to contribute a change to make the behaviour of RefUpdate match what we've described above.

Back to the top