Conversation
bigerl
left a comment
bigerl
left a comment
There was a problem hiding this comment.
Thx for the PR. I didn't have the time to faithfully review this PR fully by myself. So, all but one comment are essentially from Claude. I also asked claude to prepare a PR #500 with regression tests for the issues it reported. Feel free to merge or cherry pick it.
I went through the comments. They seem to be legit.
There was a problem hiding this comment.
Thank you for addressing. I have re-run the review skill. Please find the comments below.
New regression tests for eight of the comments are in PR #502. Closing #500 as obsolete.
Things found outside the changes:
-
docs/source/users_guide.rstline 52 andREADME.mdline 14 still say that remote contexts and@importare not fetched.
| } | ||
|
|
||
| // 5.2.3 | ||
| if (p.remote_contexts.size() > remote_context_size_limit) { |
There was a problem hiding this comment.
A remote context is processed again for every occurrence, and only the length of remote_contexts is bounded. The work doubles with each extra copy on the way down.
A chain of small remote documents where each one names its predecessor twice takes 693 ms at 18 documents, 2.2 s at 20, and does not finish in 25 s at 30 (1.8 kB of remote data in total, each URL is requested once because the cache keeps the body). A single self-reference is not the problem: it stops after 101 loads with "context overflow".
Bound the total number of remote context loads per parse, not only the depth. Skipping URLs that are already in remote_contexts is not enough: the same blowup works with distinct URLs on every path (each document names two children, both name the next, 5.6 s at depth 20 with 41 distinct URLs).
There was a problem hiding this comment.
If the number of entries in the remote contexts array exceeds a processor defined limit, a context overflow error has been detected and processing is aborted; otherwise, add context to remote contexts.
It does not sound like i would be allowed to make that change.
2 participants
close #431