Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correct looping bug in library discovery #3574

Merged
merged 4 commits into from
Nov 6, 2023

Conversation

srawlins
Copy link
Member

@srawlins srawlins commented Nov 5, 2023

The current discover-libraries-in-a-loop code has a little bug in how libraries from new packages are tracked: Currently the loop is based on whether any new packages are discovered. So if a library from a new package is added in one loop, but the rest of the files which that library references (imports/exports) from the same package are found in the next loop, the package-tracking variables current and lastPass don't catch this, and the new libraries from the same package are not processed.

This bug is masked by the fact that we call _parseLibraries twice, once for elements-to-document, and once for "special classes" (Object, Incerceptor, Pragma, ...). I won't get into the whole idea of two _parseLibraries calls here, as I'm just learning about the special libraries. In any case, the bug is hidden by this second call; those libraries that were left behind are now parsed/resolved in the second call, as they have been tracked with a field, _knownFiles. The field is not reset between calls, so the missed libraries are found in the end. The bug does not manifest itself in the second call, because the only elements we wish to parse in that second call are in the Dart SDK.

The bug does lead to performance degradation!! Or more specifically, the fact that _knownFiles is tracked in a field that isn't cleared between calls, causes a performance degradation. In that second call to _parseLibraries, we re-resolve all known libraries!

In this PR, I refactor _knownFiles away from being a field, and I track the _parseLibraries loop with individual file sets, rather that package sets.

This saves, on my M1 laptop, 10 seconds in documenting the googleapis package, and between 100 and 200 seconds in documenting Flutter (!!).

A few other notes:

  • _parseLibraries had two function callback parameters which made the code hard for me to understand. I changed one to a addingSpecials bool parameter, and the function itself now controls how to filter added libraries, based on the bool.
  • Make the filterExcludes parameter on _findFilesToDocumentInPackage required; it was hard to reason at the call-sites what was being performed, when this parameter was optional.
  • More comments.

  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

@srawlins srawlins requested a review from kallentu November 6, 2023 14:35
Copy link
Member

@kallentu kallentu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is such a crazy part of DartDoc imo, I didn't know it did all this.
Thanks for refactoring it though, it's a lot easier to reason with now.

lib/src/model/package_builder.dart Outdated Show resolved Hide resolved
@srawlins
Copy link
Member Author

srawlins commented Nov 6, 2023

Yeah I think a lot of this is because of Dartdoc's need to know the universe before it can document a single file. Also a lot of this is needed for Flutter's use case where they want to include the documentation of every transitive package they depend on, at api.flutter.dev.

@srawlins srawlins merged commit 69302eb into dart-lang:main Nov 6, 2023
9 checks passed
@srawlins srawlins deleted the faster-special-classes branch November 6, 2023 23:42
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.

2 participants