use more cython features for item.pyx

[RonnyPfannschmidt]

this is a simple transform to c level classes, which removes the need for slots, and uses c level properties as well

i wonder how to benchmark the impact

[ThomasWaldmann]

Thanks. Did you see some tests are failing?

What's the supposed performance (speed, memory usage) impact of this?

[RonnyPfannschmidt]

i wonder how to measure, and im currently fighting the memory views for the chunk compare

[ThomasWaldmann]

Speed: There are some tests using pytest-benchmark, there is borg bench(mark?) command or one could just run borg against a specific dataset on a idle machine, backing up into a local repo.

Memory usage: not sure we have something for that yet.

[RonnyPfannschmidt]

i finished a initial iteraation, im going to work out in more detail some properties

i want method tables to be gone i want the encoded /optional properties to use c level encode/decode

[RonnyPfannschmidt]

i havent figured why src/borg/testsuite/archiver.py::DiffArchiverTestCase::test_basic_functionality fails yet

[codecov-commenter]

Codecov Report

Merging #5763 (aa844d2) into master (12d27d7) will decrease coverage by 0.09%. The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5763      +/-   ##
==========================================
- Coverage   82.94%   82.84%   -0.10%     
==========================================
  Files          39       39              
  Lines       10669    10669              
  Branches     2094     2094              
==========================================
- Hits         8849     8839      -10     
- Misses       1307     1315       +8     
- Partials      513      515       +2     

Impacted Files	Coverage Δ
src/borg/archiver.py	78.60% <0.00%> (-0.32%)	:arrow_down:
src/borg/archive.py	81.40% <0.00%> (-0.25%)	:arrow_down:
src/borg/helpers/parseformat.py	90.04% <0.00%> (-0.17%)	:arrow_down:
src/borg/repository.py	82.62% <0.00%> (+0.17%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 65c7829...aa844d2. Read the comment docs.

[RonnyPfannschmidt]

ok, unless i missed something thats done

now whats missing in top is a script to measure perf changes, i didnt find the time to look into that

[ThomasWaldmann]

@RonnyPfannschmidt can i fix some cosmetic issues?

[RonnyPfannschmidt]

@ThomasWaldmann feel free to, may i funnel up the rename of the chunk content compare function to a global to master, it makes building the benchmarks against master vs this easier, and i believe for more practical speed-up i might need to add some extras for the encoding down the line (aka turning the python lookups into a c vtable lookup

for a few personal/professional reasons i may not be able to get back to this until after the python language summit

[ThomasWaldmann]

ok, pep8 issues fixed, so the linter does not block the real tests any more.

[ThomasWaldmann]

I'ld like to make a release soon. Would be nice to have this merged, just make sure it is no regression with resource consumption.

[RonnyPfannschmidt]

@ThomasWaldmann i cant yet quantify if this add sensible/measurable perf gains, but im happy to rebase later today to make it mergable and recent

[RonnyPfannschmidt]

@ThomasWaldmann i finally got around rebasing this, sorry for the delay, i think one of the next steps is me taking a good look at profiles

[ThomasWaldmann]

Any progress here? As this is a low-level change, I'ld rather like to have this merged before releasing a 1.2 release-candidate.

[ThomasWaldmann]

Also, tests are failing in a strange way - maybe rebase on current master and force-push to trigger a new test run?

[RonnyPfannschmidt]

thanks for the ping, will rebase this evening

[ThomasWaldmann]

BTW, I have only 1 ticket (#5679) left for next milestone (which is 1.2.0rc1).

After rc1, I would not like to add any low-level changes, so this PR would have to wait until 1.2.1(beta/rc) at least.

The minimum required for merge is working tests and some reasoning why this is at least as good as what we have from the performance / resource usage perspective (== no regression).

I'ld like to have it in as the source code is prettier.

[RonnyPfannschmidt]

For personal reasons I won't be able to touch this with the next 4 weeks at least 🙁

[ThomasWaldmann]

Well, my timing also came out differently than expected. But the current idea is to get 1.2 out at twos-day (tuesday 2022-02-22).

So if this can be updated / fixed, we could try to get it into final beta or rc1.

[ThomasWaldmann]

I'ld try a rebase on current master now (if you don't mind) to see if the test failures still happen after that.

Currently, one can't even see why they were failing:

Error: We are currently unable to download the log. Please try again later.
run pytest via tox

[ThomasWaldmann]

Rebased without any conflicts, so let's see what CI says...

[ThomasWaldmann]

CI looking better now.

[RonnyPfannschmidt]

thanks for the rebase/fix, im still far from taking a look myself, i hope to make the escaping helpers cimport-able sooner or later to sidestep python there as well but those bits can be skipped for getting the normal enhancement in first

[ThomasWaldmann]

Please do not add more features here.

Blocking here is the evaluation of performance / resource usage of the existing proposed changes (== assuring there is no regression).

[ThomasWaldmann]

superseded by #7063, closing this.