#294 confirm_path is very inefficient

Opened by rohan, on July 26, 2018
rohan commented on July 26, 2018

I've got a test repo partly converted from git which has 12 directories containing 806 files and 25Mb of code. Recording while profiling with callgrind shows that the nested self.iter_nodes loops eat up much of the time. For example, recording a 10 line change across 3 files takes 63 seconds. Commenting out the two calls to confirm_path reduces that to 21 seconds.

From the profile it seems that confirm_path ends up calling the node-iterator's next method about 750k times per file.

FlorentBecker commented on August 28, 2018

This should be fixed in master, but I'm not good at reading the output of callgrind. Would you be able to check?

rohan commented on August 29, 2018

Thanks. This may take a while to test as the newer version of pijul is giving a NoDB error for my test repo and the main git-pijul doesn't build against the latest libpijul

yory commented on September 7, 2018

you can fix the nodb error by running "pijul record", even if there's nothing to record, it will unlock the issue

rohan commented on September 9, 2018

Hi Yory

Unfortunately that just gives this error and doesn't fix the NoDB:

thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', libcore/option.rs:345:21
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
   1: std::sys_common::backtrace::print
   2: std::panicking::default_hook::{{closure}}
   3: std::panicking::default_hook
   4: std::panicking::rust_panic_with_hook
   5: std::panicking::continue_panic_fmt
   6: rust_begin_unwind
   7: core::panicking::panic_fmt
   8: core::panicking::panic
   9: libpijul::record::<impl libpijul::backend::GenericTxn<sanakirja::transaction::MutTxn<'env, ()>, R>>::record_inode
             at libcore/macros.rs:20
             at /home/rohan/src/pijul/libpijul/src/patch/mod.rs:775
             at /home/rohan/src/pijul/libpijul/src/record.rs:667
  10: libpijul::record::<impl libpijul::backend::GenericTxn<sanakirja::transaction::MutTxn<'env, ()>, R>>::record_children
             at /home/rohan/src/pijul/libpijul/src/record.rs:563
  11: libpijul::record::<impl libpijul::backend::GenericTxn<sanakirja::transaction::MutTxn<'env, ()>, T>>::record
             at /home/rohan/src/pijul/libpijul/src/record.rs:846
  12: pijul::commands::record::changes_from_prefixes
             at pijul/src/commands/record.rs:379
  13: pijul::commands::record::run
             at pijul/src/commands/record.rs:145
             at pijul/src/commands/record.rs:212
  14: pijul::main
             at pijul/src/main.rs:76
rohan commented on September 9, 2018

Well, I've patched git-pijul for this version of libpijul but can't seem to get valgrind to read the debug info.

The good news is that it's much faster, around 10 times based on wall clock and perf shows that graph::retrieve and record::diff_with_binary pretty equally share the remaining time.

RohanHart commented on September 11, 2018

If anyone's interested here's the perf graph

pmeunier commented on September 27, 2018

Has this been fixed?