Skip to content

fix(deep_crawling): BFS O(n²) parent lookup and BestFirst duplicate enqueue - #2243

Open
chelsealong wants to merge 1 commit into
unclecode:mainfrom
chelsealong:fix/deep-crawl-bfs-and-bestfirst-o-n-squared
Open

fix(deep_crawling): BFS O(n²) parent lookup and BestFirst duplicate enqueue#2243
chelsealong wants to merge 1 commit into
unclecode:mainfrom
chelsealong:fix/deep-crawl-bfs-and-bestfirst-o-n-squared

Conversation

@chelsealong

Copy link
Copy Markdown

Summary

Fixes #2242, two related performance bugs in deep_crawling:

  1. BFSDeepCrawlStrategy — O(n²) parent lookup. _arun_batch and
    _arun_stream looked up each result's parent URL with
    next((parent for (u, parent) in current_level if u == url), None),
    a linear scan of the whole current level, executed once per result in
    that level. For a level of N URLs this is O(N²) pure-Python work. Fixed
    by building a url -> parent dict once per level (dict(current_level))
    and doing an O(1) .get(url) per result.

  2. BestFirstCrawlingStrategy — duplicate enqueue. link_discovery
    only read visited (if base_url in visited: continue) but never
    updated it, so when two sibling pages processed in the same batch both
    link to the same third URL, that URL was scored and pushed onto the
    priority queue twice before either copy was ever dequeued (the dupe was
    later silently dropped at dequeue time via the same visited check, so
    final output was correct — the waste was in scoring/enqueueing).

    The obvious fix — marking visited as soon as a URL is discovered,
    inside link_discovery — turned out to be wrong: visited is also
    read by the dequeue loop in _arun_best_first to mean "already
    processed/dequeued", and marking a URL visited at discovery time
    made the dequeue loop skip it before it was ever crawled. Instead this
    adds a separate self._enqueued set, tracked distinctly from
    visited and checked/updated only in link_discovery, mirroring the
    pattern DFSDeepCrawlStrategy already uses for its own _dfs_seen set
    for exactly this reason (see the docstring on DFSDeepCrawlStrategy).

Test plan

Added tests/deep_crawling/test_deep_crawl_efficiency.py with two
regression tests, both built on the mocked-crawler pattern already used in
tests/deep_crawling/test_deep_crawl_cancellation.py:

  • test_parent_lookup_scales_linearly_with_level_size — runs
    BFSDeepCrawlStrategy._arun_batch against a mocked crawler with a level
    of 2,500 URLs and then one of 20,000 URLs, and asserts the wall-clock
    growth ratio stays under 12x for an 8x increase in level size (linear
    bookkeeping keeps this near 8x; O(n²) bookkeeping pushes it well past
    it). This is a self-relative check (small vs. large run of the same
    code) so it isn't tied to absolute machine speed.
  • test_shared_link_is_scored_and_enqueued_once — start URL links to
    pages A and B, both of which link to a shared third URL. Asserts the
    shared URL's scorer is called exactly once and it appears exactly once
    in the yielded results.

Both tests fail on the pre-fix code and pass after the fix:

$ git stash push -- crawl4ai/deep_crawling/bfs_strategy.py crawl4ai/deep_crawling/bff_strategy.py
$ python -m pytest tests/deep_crawling/test_deep_crawl_efficiency.py -v
...
FAILED ...test_parent_lookup_scales_linearly_with_level_size - AssertionError: per-level
  bookkeeping does not scale linearly: 2500 URLs took 0.443s, 20000 URLs took 7.632s
  (17.2x for an 8x increase in level size)
FAILED ...test_shared_link_is_scored_and_enqueued_once - AssertionError: assert 2 == 1
2 failed in 8.95s

$ git stash pop
$ python -m pytest tests/deep_crawling/test_deep_crawl_efficiency.py -v
...
2 passed in 4.28s

Full existing deep-crawling unit suite still green (excluding two
pre-existing failures caused by Playwright's browser binary not being
installed in this sandbox — confirmed identical on unmodified main):

$ python -m pytest tests/deep_crawling/ \
    --deselect tests/deep_crawling/test_deep_crawl_resume_integration.py::TestBFSResumeIntegration::test_real_crawl_state_capture_and_resume \
    --deselect tests/deep_crawling/test_deep_crawl_resume_integration.py::TestBFSResumeIntegration::test_state_export_method
69 passed, 2 deselected in 4.48s

Notes

  • Per CONTRIBUTING.md, PRs should target the develop branch rather than
    main — please retarget if this was opened against main.
  • This change and its tests were written with AI assistance (Claude Code),
    reviewed and verified locally before submission.

🤖 Generated with Claude Code

… BestFirst

BFSDeepCrawlStrategy re-scanned the entire current_level list once per
fetched result to find that result's parent URL, making per-level
bookkeeping O(n^2) instead of O(n). Build a url->parent dict once per
level instead.

BestFirstCrawlingStrategy.link_discovery only checked `visited` without
updating it, so a URL discovered by two sibling pages processed in the
same batch was scored and pushed onto the priority queue twice before
either copy was dequeued. Track newly discovered URLs in a separate
`_enqueued` set (kept distinct from `visited`, which the dequeue loop
relies on to mean "already processed") so a repeat discovery within the
same crawl is skipped.

Fixes unclecode#2242
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: BFSDeepCrawlStrategy re-scans the full level for parent lookup (O(n²)); BestFirstCrawlingStrategy can enqueue the same URL twice

1 participant