On 8/4/2020 1:31 PM, Derrick Stolee wrote: > On 8/4/2020 1:00 PM, SZEDER Gábor wrote: >> Having said that, the best (i.e faster and accurate) solution to this >> issue is probably: >> >> - Update the callchain between diff_tree_oid() and the diff callback >> functions to allow the callbacks to break diffing with a non-zero >> error code. > > It looks like this part would not be too difficult. Oh, my hubris! I gave this a shot for some time this morning. This will definitely take some work to do right. Just changing the callbacks to return 'int' is a wide-sweeping change, but the place where they are called already has an 'int' return that means something different. I'm not saying this is impossible. It just takes more attention and care than I can currently devote, given my other works in progress right now. >> - Fill Bloom filters using the approach presented in: >> >> https://public-inbox.org/git/20200529085038.26008-21-szeder.dev@xxxxxxxxx/ >> >> but modify the callbacks to return non-zero when too many paths >> have been processed. > > Thanks for the pointer to that specific patch. You do a good job of > describing your thought process, including why you used the callback > approach instead of the diff queue approach. The main reason seemed to > be memory overhead from populating the entire diff queue before > checking the limit. > > However, if we are using the diff queue as the short-circuit, then > perhaps that memory overhead isn't as much of a problem? > > You admit yourself, that > > This patch implements a more efficient, but more complex, approach: > > The logic around matching prefixes definitely seems complex and > hard to test, especially around the file/directory changes with the > sort order problems that have plagued similar prefix checks recently. > I'm not doubting your implementation, just saying that the complexity > is worth considering before jumping to that solution too quickly. > > To sum up, I intend to start with a fix that uses the diff queue > count as a limit, then try the callback approach to see if there are > measurable improvements in performance. That fix is now available [1]. [1] https://lore.kernel.org/git/d1c4bbcaa9627068d5d9fbd0e4a2e8c8834a4bd3.1596646576.git.me@xxxxxxxxxxxx/ Again, the callback approach seems promising. The complexity is stopping me from trying to apply it on top of the current implementation, while I should be focusing on other things. I completely believe that that approach is faster and more memory-efficient. I would love to test and review a patch that takes that approach here. Thanks, -Stolee
