Fix quadratic performance and memory issues in duplicate-code checker

[Pierre-Sassoulas]

Type of Changes

	Type
✓	🐛 Bug fix
✓	🔨 Refactoring

Description

Foor optimizations to symilar:

• Rolling hash: compute the window hash incrementally (subtract the leaving line hash, add the entering one) instead of re-summing all k line hashes for every position.

• Cache hash_lineset results per lineset in _iter_sims: each file was being hashed once per pair (N-1 times) instead of once total.

• Remove the LinesChunk wrapper class and use plain int dict keys, so frozenset intersection and dict lookups use C-level hash/eq.

• Above a certain threshold of similar line the algorithm is different (this one is due to psf/black/profiling/ directory and its pathological content, good profiling tool black's contributors, congrat).

One optimization to the duplicate-code checker:

• Reusing pylint's parsed ast in the check instead of recalculating as it was only useful if symilar was launched independently. (This one also reduce the memory used, I went from being oom killed on cpython to having a result in 90s)

Benchmark: duplicate-code checker performance

Measured with:

"""Benchmark the duplicate-code checker on a given path.

Usage: python bench_symilar.py <path>
"""

import subprocess
import sys


def main() -> None:
  if len(sys.argv) < 2:
      print(f"Usage: {sys.argv[0]} <path>")
      sys.exit(1)

  path = sys.argv[1]
  nfiles = int(
      subprocess.check_output(
          f"find {path} -name '*.py' | wc -l", shell=True
      ).strip()
  )
  nlines = int(
      subprocess.check_output(
          f"find {path} -name '*.py' -exec cat {{}} + | wc -l", shell=True
      ).strip()
  )

  result = subprocess.run(
      [
          "/usr/bin/time", "-v",
          sys.executable, "-m", "pylint",
          "--disable=all", "--enable=duplicate-code", path,
      ],
      capture_output=True,
      text=True,
  )

  time_s = mem_mb = "?"
  for line in result.stderr.splitlines():
      if "wall clock" in line:
          time_s = line.split()[-1]
      if "Maximum resident" in line:
          mem_mb = int(line.split()[-1]) // 1024

  print(f"files={nfiles}  lines={nlines}  time={time_s}  mem={mem_mb}MB")


if __name__ == "__main__":
  main()

Project	Files	Lines	before	after	Speedup	Mem before	Mem after	Mem reduction
flask	83	18,399	4.72s	2.87s	1.6x	123 MB	108 MB	-12%
astroid	96	30,352	6.55s	2.54s	2.6x	128 MB	107 MB	-16%
psycopg	248	67,478	26.50s	8.12s	3.3x	270 MB	208 MB	-23%
black	316	129,230	stuck	6.69s	inf	?	294 MB	?
django	902	162,352	3m45s	14.52s	15.5x	393 MB	328 MB	-17%
pandas	1,414	635,395	19m58s	54.52s	22.0x	1,205 MB	877 MB	-27%
sentry	7,609	1,443,533	stuck	2m51s	inf	?	1,854 MB	?

Small projects are dominated by startup/parsing overhead, but the performance increase shows for bigger one. Maybe we could even add duplicate-code back to the primer now.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.24%. Comparing base (b080a21) to head (1e003ed).

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10881      +/-   ##
==========================================
+ Coverage   96.19%   96.24%   +0.05%     
==========================================
  Files         178      178              
  Lines       19683    19679       -4     
==========================================
+ Hits        18934    18940       +6     
+ Misses        749      739      -10     

Files with missing lines	Coverage Δ
pylint/checkers/symilar.py	99.13% <100.00%> (+2.83%)	⬆️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[DanielNoord]

Do we have an expert on this in our pool of maintainers? If the tests pass this LGTM as I really have no experience with this part of the codebase or these kind of computations... 😅

But I'm not sure we have anybody who does have that experience.

[Pierre-Sassoulas]

The expert is hippo91, I'm going to ping him when it's ready (there's still a coverage issue, and I didn't run the primer locally yet because it takes a ton of time). Although I'm not sure there's going to be a lot of duplication in the primer, maintainers tend to remove it. Also we're going to have primer for all the duplicate-code related fixes next. (in particular the search for duplicate code in the same file is pretty bad in pylint itself).

[jacobtylerwalls]

I looked, and the changes smell right and are clearly described.

I don't know anything about "still lets remove_successive coalesce consecutive matches.", so I would like just a little test coverage in this area. Can we have a test that mocks _HASH_BUCKET_PRODUCT_LIMIT to something very low, and then runs a functional test over some code that triggers the other form of the algorithm?

[jacobtylerwalls]

I guess we can trial this for now and revert it if it's too slow.

[Pierre-Sassoulas]

Hey @hippo91 as the expert in optimizing duplicate-code would you mind reviewing this ? (Work better commit by commit)

[github-actions]

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit 1e003ed