Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2220703002:
Initial implementation of pub summary manager. (Closed)
|
Created:
4 years, 4 months ago by scheglov Modified:
4 years, 4 months ago CC:
reviews_dartlang.org Base URL:
git@github.com:dart-lang/sdk.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionInitial implementation of pub summary manager.
For now we just generate unlinked, spec summaries.
We don't use them.
R=brianwilkerson@google.com, paulberry@google.com
BUG=
Committed: https://github.com/dart-lang/sdk/commit/c35fb3c79dc4406666591bf530771bb3958d5deb
Patch Set 1 #Patch Set 2 : tweak #
Total comments: 13
Patch Set 3 : Fix for URI on Windows. #Patch Set 4 : Rework to better fit actual use. #
Total comments: 9
Patch Set 5 : Parse without context, write atomically. #
Total comments: 2
Messages
Total messages: 14 (1 generated)
https://codereview.chromium.org/2220703002/diff/10001/pkg/analysis_server/lib... File pkg/analysis_server/lib/src/pub_summary.dart (right): https://codereview.chromium.org/2220703002/diff/10001/pkg/analysis_server/lib... pkg/analysis_server/lib/src/pub_summary.dart:52: * Class the manages summaries for pub packages. s/the/that/ https://codereview.chromium.org/2220703002/diff/10001/pkg/analysis_server/lib... pkg/analysis_server/lib/src/pub_summary.dart:193: String uriStr = 'package:${package.name}/$pathInLib'; This looks incorrect for Windows, since on Windows, pathInLib will contain backslashes. https://codereview.chromium.org/2220703002/diff/10001/pkg/analysis_server/lib... pkg/analysis_server/lib/src/pub_summary.dart:242: * If the given [uri] has the `package` scheme, return the names of the s/names/name/
PTAL https://codereview.chromium.org/2220703002/diff/10001/pkg/analysis_server/lib... File pkg/analysis_server/lib/src/pub_summary.dart (right): https://codereview.chromium.org/2220703002/diff/10001/pkg/analysis_server/lib... pkg/analysis_server/lib/src/pub_summary.dart:52: * Class the manages summaries for pub packages. On 2016/08/05 21:43:02, Paul Berry wrote: > s/the/that/ Done. https://codereview.chromium.org/2220703002/diff/10001/pkg/analysis_server/lib... pkg/analysis_server/lib/src/pub_summary.dart:193: String uriStr = 'package:${package.name}/$pathInLib'; On 2016/08/05 21:43:02, Paul Berry wrote: > This looks incorrect for Windows, since on Windows, pathInLib will contain > backslashes. Fixed. https://codereview.chromium.org/2220703002/diff/10001/pkg/analysis_server/lib... pkg/analysis_server/lib/src/pub_summary.dart:242: * If the given [uri] has the `package` scheme, return the names of the On 2016/08/05 21:43:02, Paul Berry wrote: > s/names/name/ Done.
lgtm
I don't like the fact that this code is specific to the analysis server; I'd much rather have it in analyzer so that we could use the same class when running the command-line analyzer. It looks like the only place it's using the server is to (a) get told when a context is created so it can register for analysis events and (b) to get the SDK manager. I think the registration belongs outside this class and the manager could be passed in in place of the server. Also, I have some concerns about how this works when we have two or more tools all trying to manage the summary files (two servers, or a server and a command-line tool, etc.). It looks like there can be races. https://codereview.chromium.org/2220703002/diff/10001/pkg/analysis_server/lib... File pkg/analysis_server/lib/src/pub_summary.dart (right): https://codereview.chromium.org/2220703002/diff/10001/pkg/analysis_server/lib... pkg/analysis_server/lib/src/pub_summary.dart:30: @visibleForTesting nit: There have been requests for this annotation; seems like we should move it into the meta package and fully support it if we're going to use it. https://codereview.chromium.org/2220703002/diff/10001/pkg/analysis_server/lib... pkg/analysis_server/lib/src/pub_summary.dart:54: class PubSummaryManager { I'd love to see an example of how clients are expected to use this class (maybe as part of the docs for the class). https://codereview.chromium.org/2220703002/diff/10001/pkg/analysis_server/lib... pkg/analysis_server/lib/src/pub_summary.dart:147: _scheduleNextPackageSummary(); If a package 'a' depends on a package 'b', do we need to create a summary for 'b' before we create a summary for 'a', or is that not a constraint when building unlinked summaries? https://codereview.chromium.org/2220703002/diff/10001/pkg/analysis_server/lib... pkg/analysis_server/lib/src/pub_summary.dart:180: <UriResolver>[new DartUriResolver(server.sdkManager.anySdk)]); This won't let us distinguish between packages built for web and packages built for flutter. Maybe that doesn't matter for an unliked summary. Also, don't we need a PackageUriResolver for 'package:' URI's used within the package being summarized? Or do we not need it because we're producing an unlinked summary? https://codereview.chromium.org/2220703002/diff/10001/pkg/analysis_server/lib... pkg/analysis_server/lib/src/pub_summary.dart:212: for (Resource child in children) { Does the order in which we add the children matter (do children in a folder need to be added before children in subfolders)? If not, consider combining these two loops.
https://codereview.chromium.org/2220703002/diff/10001/pkg/analysis_server/lib... File pkg/analysis_server/lib/src/pub_summary.dart (right): https://codereview.chromium.org/2220703002/diff/10001/pkg/analysis_server/lib... pkg/analysis_server/lib/src/pub_summary.dart:147: _scheduleNextPackageSummary(); On 2016/08/05 22:09:13, Brian Wilkerson wrote: > If a package 'a' depends on a package 'b', do we need to create a summary for > 'b' before we create a summary for 'a', or is that not a constraint when > building unlinked summaries? That is not a constraint when building unlinked summaries. https://codereview.chromium.org/2220703002/diff/10001/pkg/analysis_server/lib... pkg/analysis_server/lib/src/pub_summary.dart:180: <UriResolver>[new DartUriResolver(server.sdkManager.anySdk)]); On 2016/08/05 22:09:14, Brian Wilkerson wrote: > This won't let us distinguish between packages built for web and packages built > for flutter. Maybe that doesn't matter for an unliked summary. I'm not aware of any constraints Flutter imposes that would make a difference to an unlinked summary, so I think this is ok. > > Also, don't we need a PackageUriResolver for 'package:' URI's used within the > package being summarized? Or do we not need it because we're producing an > unlinked summary? Correct, we don't need this because the act of producing an unlinked summary doesn't do any URI resolution. (If it did, it would defeat the purpose, because part of the idea of an unlinked summary is that it can be safely re-used in a situation where URI resolution is different).
On 2016/08/05 22:09:14, Brian Wilkerson wrote: > I don't like the fact that this code is specific to the analysis server; I'd > much rather have it in analyzer so that we could use the same class when running > the command-line analyzer. It looks like the only place it's using the server is > to (a) get told when a context is created so it can register for analysis events > and (b) to get the SDK manager. I think the registration belongs outside this > class and the manager could be passed in in place of the server. Thank you, Brian. Thinking about actual use prompted a simpler implementation. I moved PubSummaryManager to analyzer. The class changed pretty significantly. PTAL
lgtm https://codereview.chromium.org/2220703002/diff/50001/pkg/analyzer/lib/src/su... File pkg/analyzer/lib/src/summary/pub_summary.dart (right): https://codereview.chromium.org/2220703002/diff/50001/pkg/analyzer/lib/src/su... pkg/analyzer/lib/src/summary/pub_summary.dart:118: List<PackageBundle> getLinkedBundles(AnalysisContext context) { Sorry for not picking up on this during the last review, but a concern I have about this API is that it forces linked bundle computation to be done eagerly. I believe that in the typical use case the number of linked bundles that will actually be needed for a given instantiation of analysis server is going to be small, so it might make more sense to compute them on demand at the time they are needed. (Note: we will probably want to try both alternatives and make some profiling measurements, though). Consider shifting to an API that would allow for the possibility of changing the implementation in the future to computing the linked bundles on demand instead of eagerly. Note also that I am in the process of adding information to summaries to make it easier for the analysis server to locate the summaries they depend on (and to figure out the mapping between those summaries and the URIs they contain information for); we should talk sometime today about this and see if there's a way this information can save you some I/O in the case where not all the summaries are needed. (I don't want to hold up progress, though. If you'd prefer to address this at a later date, that's ok with me). https://codereview.chromium.org/2220703002/diff/50001/pkg/analyzer/lib/src/su... pkg/analyzer/lib/src/summary/pub_summary.dart:128: Map<String, PackageBundle> getUnlinkedBundles(AnalysisContext context) { Similar concern here; also ok if you want to defer until later.
Seems like a design discussion is needed. https://codereview.chromium.org/2220703002/diff/50001/pkg/analysis_server/lib... File pkg/analysis_server/lib/src/analysis_server.dart (right): https://codereview.chromium.org/2220703002/diff/50001/pkg/analysis_server/lib... pkg/analysis_server/lib/src/analysis_server.dart:374: resourceProvider, DirectoryBasedDartSdk.defaultSdk); Does the SDK selected here matter (there can be more than one known to server depending on things such as flutter/non-flutter or strong/spec mode)? If so, this might need to be more complex. But if not, this should be "sdkManager.anySdk". https://codereview.chromium.org/2220703002/diff/50001/pkg/analyzer/lib/src/su... File pkg/analyzer/lib/src/summary/pub_summary.dart (right): https://codereview.chromium.org/2220703002/diff/50001/pkg/analyzer/lib/src/su... pkg/analyzer/lib/src/summary/pub_summary.dart:193: CompilationUnit unit = context.computeResult(source, PARSED_UNIT); Do we want to discard all of the data computed while parsing, or do we want to capture the context in which the work is to be done? Even if all of the data is recoverable from the summary (is LineInfo recoverable?) it seems a shame to compute it, discard it, and then deserialize it. But maybe it's fast enough to not be worth the extra effort. https://codereview.chromium.org/2220703002/diff/50001/pkg/analyzer/lib/src/su... pkg/analyzer/lib/src/summary/pub_summary.dart:253: if (packagesToComputeUnlinked.length == 1) { Given that `packagesToComputeUnlinked` is a set, I think this allows us to schedule multiple times if the same package is requested multiple times. Not sure whether it matters (although we might get multiple "completed" notifications), but I think we need to test whether the set was empty before adding the package. https://codereview.chromium.org/2220703002/diff/50001/pkg/analyzer/lib/src/su... pkg/analyzer/lib/src/summary/pub_summary.dart:268: new Future.delayed(new Duration(milliseconds: 10), _computeNextUnlinked); It seems odd to me that we're scheduling work here. Seems like the constraints on whether and when to perform work need to be under the client's control. For example, in the command-line analyzer (assuming we want to generate summaries for the next run) there isn't any reason to delay the computation.
Brian, I made the changes we discussed today. https://codereview.chromium.org/2220703002/diff/50001/pkg/analysis_server/lib... File pkg/analysis_server/lib/src/analysis_server.dart (right): https://codereview.chromium.org/2220703002/diff/50001/pkg/analysis_server/lib... pkg/analysis_server/lib/src/analysis_server.dart:374: resourceProvider, DirectoryBasedDartSdk.defaultSdk); On 2016/08/08 14:32:55, Brian Wilkerson wrote: > Does the SDK selected here matter (there can be more than one known to server > depending on things such as flutter/non-flutter or strong/spec mode)? If so, > this might need to be more complex. > > But if not, this should be "sdkManager.anySdk". Actually we almost don't need any DartSdk. The only reason why we use it now is that DartParseTask also performs URIs resolution. We might want to fix this by extracting URIs resolution into a separate task and drop using DartSdk here altogether. https://codereview.chromium.org/2220703002/diff/50001/pkg/analyzer/lib/src/su... File pkg/analyzer/lib/src/summary/pub_summary.dart (right): https://codereview.chromium.org/2220703002/diff/50001/pkg/analyzer/lib/src/su... pkg/analyzer/lib/src/summary/pub_summary.dart:118: List<PackageBundle> getLinkedBundles(AnalysisContext context) { On 2016/08/08 12:24:59, Paul Berry wrote: > Sorry for not picking up on this during the last review, but a concern I have > about this API is that it forces linked bundle computation to be done eagerly. > I believe that in the typical use case the number of linked bundles that will > actually be needed for a given instantiation of analysis server is going to be > small, so it might make more sense to compute them on demand at the time they > are needed. (Note: we will probably want to try both alternatives and make some > profiling measurements, though). > > Consider shifting to an API that would allow for the possibility of changing the > implementation in the future to computing the linked bundles on demand instead > of eagerly. > > Note also that I am in the process of adding information to summaries to make it > easier for the analysis server to locate the summaries they depend on (and to > figure out the mapping between those summaries and the URIs they contain > information for); we should talk sometime today about this and see if there's a > way this information can save you some I/O in the case where not all the > summaries are needed. > > (I don't want to hold up progress, though. If you'd prefer to address this at a > later date, that's ok with me). Yes, I plan to perform profiling on some dependencies-heavy applications to see whether loading summaries contributes much to the total analysis time. https://codereview.chromium.org/2220703002/diff/50001/pkg/analyzer/lib/src/su... pkg/analyzer/lib/src/summary/pub_summary.dart:253: if (packagesToComputeUnlinked.length == 1) { On 2016/08/08 14:32:56, Brian Wilkerson wrote: > Given that `packagesToComputeUnlinked` is a set, I think this allows us to > schedule multiple times if the same package is requested multiple times. Not > sure whether it matters (although we might get multiple "completed" > notifications), but I think we need to test whether the set was empty before > adding the package. Fixed.
lgtm https://codereview.chromium.org/2220703002/diff/70001/pkg/analyzer/lib/src/su... File pkg/analyzer/lib/src/summary/pub_summary.dart (right): https://codereview.chromium.org/2220703002/diff/70001/pkg/analyzer/lib/src/su... pkg/analyzer/lib/src/summary/pub_summary.dart:271: AnalysisErrorListener errorListener = AnalysisErrorListener.NULL_LISTENER; Do we want to check that there are no parse errors and not generate a summary when there are errors?
Description was changed from ========== Initial implementation of pub summary manager. For now we just generate unlinked, spec summaries. We don't use them. R=brianwilkerson@google.com, paulberry@google.com BUG= ========== to ========== Initial implementation of pub summary manager. For now we just generate unlinked, spec summaries. We don't use them. R=brianwilkerson@google.com, paulberry@google.com BUG= Committed: https://github.com/dart-lang/sdk/commit/c35fb3c79dc4406666591bf530771bb3958d5deb ==========
Message was sent while issue was closed.
Committed patchset #5 (id:70001) manually as c35fb3c79dc4406666591bf530771bb3958d5deb (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/2220703002/diff/70001/pkg/analyzer/lib/src/su... File pkg/analyzer/lib/src/summary/pub_summary.dart (right): https://codereview.chromium.org/2220703002/diff/70001/pkg/analyzer/lib/src/su... pkg/analyzer/lib/src/summary/pub_summary.dart:271: AnalysisErrorListener errorListener = AnalysisErrorListener.NULL_LISTENER; On 2016/08/09 14:09:00, Brian Wilkerson wrote: > Do we want to check that there are no parse errors and not generate a summary > when there are errors? I don't think so. The client would see the same AST structure as the summary manager, with or without errors. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
