Skip to content

Optimize peak heap usage #1858

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

Merged
merged 35 commits into from
Dec 4, 2018
Merged

Conversation

jcollins-g
Copy link
Contributor

@jcollins-g jcollins-g commented Dec 1, 2018

For testing/landing purposes, this includes #1851, but is only different from that PR by the final 3 commits, starting with: 2afd621.

While this eliminates some extra traversal of ResolvedUnitResults and saves on peak memory, the cost is a slightly more complicated PackageGraph initialization that requires some callbacks in getLibraries.

jcollins-g and others added 30 commits November 21, 2018 12:38
@googlebot googlebot added the cla: yes Google CLA check succeeded. label Dec 1, 2018
@jcollins-g jcollins-g requested review from bwilkerson and stereotype441 and removed request for stereotype441 and bwilkerson December 1, 2018 00:10
@stereotype441
Copy link
Member

@bwilkerson can you review this?

@jcollins-g
Copy link
Contributor Author

jcollins-g commented Dec 4, 2018

This is now ready for review. I recommend using this link to show the differences from #1851.

https://github.com/dart-lang/dartdoc/pull/1858/files/88c67be8c85f140398df854741e75b3663e1a586..7ac903a3f0d2ec8bbf5191c0f3a31e35dd9e33ac

Copy link
Member

@bwilkerson bwilkerson left a comment

Choose a reason for hiding this comment

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

LGTM. I'm fine with having the imports replaced in a follow-on PR.

@@ -10,6 +10,7 @@ import 'dart:collection' show UnmodifiableListView;
import 'dart:convert';
import 'dart:io';

import 'package:analyzer/analyzer.dart';
Copy link
Member

Choose a reason for hiding this comment

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

This library is deprecated and will hopefully be removed soon. Please replace this import with imports of more specific libraries, even if they are private libraries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

class _HashableChildLibraryElementVisitor
extends GeneralizingElementVisitor<void> {
final void Function(Element) libraryProcessor;
_HashableChildLibraryElementVisitor(this.libraryProcessor) {}
Copy link
Member

Choose a reason for hiding this comment

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

style nit: consider replacing "{}" with ";" (here and elsewhere)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -6652,7 +6722,7 @@ class PackageBuilder {
}
}
} while (!lastPass.containsAll(current));
return libraries;
return;
Copy link
Member

Choose a reason for hiding this comment

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

style nit: consider removing this line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -7,6 +7,7 @@ library dartdoc.model_utils;
import 'dart:convert';
import 'dart:io';

import 'package:analyzer/analyzer.dart';
Copy link
Member

Choose a reason for hiding this comment

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

Please also replace this import.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@jcollins-g
Copy link
Contributor Author

Going to land this by force-landing #1851 and then doing the git dance to land this one.

@jcollins-g jcollins-g merged commit bf3c487 into master Dec 4, 2018
@jcollins-g jcollins-g deleted the pga+rc+removensquared+astconverter branch December 4, 2018 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Google CLA check succeeded.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants