Skip to content

Convert to using a single AnalysisDriver in place of a single AnalysisContext. #1598

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Feb 2, 2018
Merged

Convert to using a single AnalysisDriver in place of a single AnalysisContext. #1598

merged 10 commits into from
Feb 2, 2018

Conversation

jcollins-g
Copy link
Contributor

@jcollins-g jcollins-g commented Jan 30, 2018 •
edited
Loading

Fixes #1586.

Dartdoc has never correctly instantiated groups of AnalysisContexts, and still doesn't correctly instantiate groups of AnalysisDrivers. But we do actually instantiate a single AnalysisDriver now instead of AnalysisContext, so that's progress.

@googlebot googlebot added the cla: yes Google CLA check succeeded. label Jan 30, 2018
@jcollins-g
Copy link
Contributor Author

jcollins-g commented Jan 30, 2018 •
edited
Loading

Comparison with master branch shows dartdoc as significantly slower (~15%) with this change. Haven't dug into why yet. It might be that one of the workarounds is imposing a penalty, or the AnalysisDriver itself could just be a bit slower.

From grind compare-sdk-warnings:

build-sdk-docs-current: Generating docs for library dart:web_gl from dart:web_gl...
build-sdk-docs-current: Generating docs for library dart:web_sql from dart:web_sql...
build-sdk-docs-current: Validating docs...
build-sdk-docs-original: found 24 warnings and 0 errors
build-sdk-docs-original: Documented 19 public libraries in 218.2 seconds
build-sdk-docs-original: Success! Docs generated into /tmp/sdkdocsAIQJGE
build-sdk-docs-current: found 24 warnings and 0 errors
build-sdk-docs-current: Documented 19 public libraries in 253.0 seconds
build-sdk-docs-current: Success! Docs generated into /tmp/sdkdocsAIQJGE
*** SDK docs : No difference in warning output from original (master) (24 warnings found)

@jcollins-g
Copy link
Contributor Author

@scheglov I'm no expert at Observatory, but poking around with it while comparing versions implicates computeNode() as being much slower under the AnalysisDriver vs AnalysisContext. This makes sense given that the initial analysis isn't the only thing that's slower. I think I can take that as motivation to get rid of computeNode in more places, and since the performance hit overall is less than 20% it's probably worth it to convert to Driver.

screenshot from 2018-01-31 08-27-02

@jcollins-g
Copy link
Contributor Author

GCNewSpace from the PR:

screenshot from 2018-01-31 08-30-59

GCOldSpace doesn't even appear on the list for the old version, and GCNewSpace looks much different on the master branch -- taking less time, and almost all of it spent inside the mustache library:

screenshot from 2018-01-31 08-32-31

@jcollins-g
Copy link
Contributor Author

Hypothesis seems to hold up -- removing computeNode from annotation building (which is where we used it most heavily) cuts the performance penalty to 5%. Fortunately, that seems possible now and apparently with equal accuracy to using computeNode.

@jcollins-g
Copy link
Contributor Author

Did a little bit more cleanup; should be stable for review again once green.

@devoncarew
Copy link
Member

devoncarew commented Feb 2, 2018 •
edited
Loading

@scheglov, @bwilkerson, can you review? This is converting dartdoc to use the analysis driver.

scheglov
scheglov approved these changes Feb 2, 2018
View reviewed changes
lib/src/model.dart Outdated
@@ -5122,8 +5130,8 @@ class PackageBuilder {
/// Parse a single library at [filePath] using the current analysis context.
Copy link
Contributor

Choose a reason for hiding this comment

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

analysis driver?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

lib/src/model.dart
@@ -5140,15 +5148,16 @@ class PackageBuilder {
if (uri != null) {
source = new FileBasedSource(javaFile, uri);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still use java files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently so. It looks like it might be possible to eliminate some of the usage (the TODO above), but we still need them for finding the analysis errors we care about later on.

@jcollins-g jcollins-g merged commit 9da617b into master Feb 2, 2018
@jcollins-g jcollins-g deleted the analysis-driver branch February 2, 2018 23:29
@jcollins-g jcollins-g mentioned this pull request Feb 5, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scheglov scheglov scheglov approved these changes

@devoncarew devoncarew Awaiting requested review from devoncarew

@bwilkerson bwilkerson Awaiting requested review from bwilkerson

@keertip keertip Awaiting requested review from keertip

Assignees
No one assigned
Labels
cla: yes Google CLA check succeeded.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jcollins-g @devoncarew @scheglov @googlebot