Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2442993002:
API for the new AnalysisDriver. (Closed)
|
Created:
4 years, 2 months ago by scheglov Modified:
4 years, 1 month ago CC:
reviews_dartlang.org Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAPI for the new AnalysisDriver.
TBR
R=brianwilkerson@google.com, paulberry@google.com
BUG=
Committed: https://github.com/dart-lang/sdk/commit/c3e7be4f122d0520ef57660a3f294c3dbaad56c5
Patch Set 1 #Patch Set 2 : Unsaved changes. #
Total comments: 38
Messages
Total messages: 10 (1 generated)
Description was changed from ========== API for the new AnalysisDriver. TBR R=brianwilkerson@google.com, paulberry@google.com BUG= ========== to ========== API for the new AnalysisDriver. TBR R=brianwilkerson@google.com, paulberry@google.com BUG= Committed: https://github.com/dart-lang/sdk/commit/c3e7be4f122d0520ef57660a3f294c3dbaad56c5 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) manually as c3e7be4f122d0520ef57660a3f294c3dbaad56c5 (presubmit successful).
Message was sent while issue was closed.
lgtm https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... File pkg/analyzer/lib/src/dart/analysis/byte_store.dart (right): https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/byte_store.dart:11: * associated with a key can change or become `null` at any point of time. s/of/in/ https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/byte_store.dart:13: abstract class ByteStore { Since this is a synchronous interface, it means that it would be impossible to write implementations of [ByteStore] in the future that: (a) share data across isolates. (b) store data in a remote location (such as a cache located on a machine that is shared by multiple users). Would it be worth switching this to an asynchronous interface? Since AnalysisDriver is already asynchronous it sounds like we would not lose much. And the future benefits might be really big. https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/byte_store.dart:18: List<int> get(String key); Since FileByteStore uses the key as a filename, we should document that the key should be suitable for use as a filename (i.e. shouldn't be too long, shouldn't contain characters like "/" or "\", shouldn't be "." or "..", etc.) https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/byte_store.dart:27: * [ByteStore] with LRU cache. When I first read this comment it wasn't clear to me whether this was (a) an implementation of [ByteStore] that stores data purely in memory, and discards it when it is not recently used, or (b) a wrapper around another [ByteStore] which adds in-memory LRU caching behavior to it. I now see that it's (b). Maybe change the comment to something like `A wrapper around [ByteStore] which adds an in-memory LRU cache to it.` https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/byte_store.dart:31: final int maxEntries; As a future improvement, consider measuring the limit in total stored bytes rather than number of entries. That way we will be able to make much stronger guarantees about memory usage. https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... File pkg/analyzer/lib/src/dart/analysis/driver.dart (right): https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/driver.dart:20: * It can be shared between other [AnalysisDriver]s. s/between/with/. Also, yay! I like how you've designed for cross-context caching from the beginning. https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/driver.dart:45: * between priority files, not between them and not priority files. s/not priority/non-priority/ https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/driver.dart:59: * guarantee that this will ever happen. I think we should make a stronger guarantee than this. To do so we have to define some additional terminology. How about something like this: Let the set of "explicitly analyzed files" denote the set of paths that have been passed to [addFile] but not subsequently passed to [removeFile]. Let the "current analysis results" denote the map from the set of explicitly analyzed files to the most recent [AnalysisResult] delivered to [results] for each file. Let the "current file state" represent a map from file path to the file contents most recently read from that file, or fetched from the content cache (considering all possible possible file paths, regardless of whether they're in the set of explicitly analyzed files). Let the "analysis state" be either "analyzing" or "idle". (These are theoretical constructs; they may not necessarily reflect data structures maintained explicitly by the driver). Then we make the following guarantees: - Whenever the analysis state is idle, the current analysis results are consistent with the current file state. - A call to [addFile] or [changeFile] causes the analysis state to transition to "analyzing", and schedules the file contents to be read into the current file state prior to the next time the analysis state transitions back to "idle". - If at any time the client stops making calls to [addFile], [changeFile], and [deleteFile], the analysis state will eventually transition back to "idle" after a finite amount of processing. As a result of these guarantees, a client may ensure that the analysis results are "eventually consistent" with the filesystem by simply calling [changeFile] any time the contents of a file on the file system have changed. https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/driver.dart:90: * nor for any other file. If you take my suggestion above, you could replace this paragraph with something like: Causes the analysis state to transition to "analyzing" (if it is not in that state already). Schedules the file contents for [path] to be read into the current file state prior to the next time the analysis state transitions to "idle". https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/driver.dart:96: */ Is it ok to pass a path to [changeFile] that is not in the set of explicitly analyzed files? I hope so because it would be nice to be able to force analysis to be updated when an implicitly analyzed file changes. https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/driver.dart:105: * The [path] must be absolute and normalized. Is it required that the path is in the set of explicitly analyzed files? https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/driver.dart:111: * state of the files. Can we make a stronger guarantee when the analysis state is idle? https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/driver.dart:130: } Additional API that we may need to add in the future: - A stream of "analysis status" information so that the client can know when the analysis state is idle. - Some way to indicate when analysis state transitions between "idle" and "analyzing". - Some way for the client to know when a file is added to or removed from the set of implicitly analyzed files, in case it wants to watch implicitly analyzed files for changes. (Note: in order to avoid a gap where the file is not being watched, this would need to be a callback rather than a stream, so that the client can be sure it starts watching the file before the driver reads it). - A single method call which is equivalent to calling [changeFile] on all reachable files (both explicit and implicit). This would allow the client to recover from file watching bugs. - The ability to abstract the file system using pkg/front_end/lib/file_system.dart https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... File pkg/analyzer/lib/src/dart/analysis/file_byte_store.dart (right): https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/file_byte_store.dart:9: * [ByteStore] that stores values as [File]s. Should this class implement some sort of LRU policy so that the folder doesn't grow without bound?
Message was sent while issue was closed.
Thank you for the comments, Paul. I updated the documentation and will upload a new CL shortly. https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... File pkg/analyzer/lib/src/dart/analysis/byte_store.dart (right): https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/byte_store.dart:11: * associated with a key can change or become `null` at any point of time. On 2016/10/23 10:39:19, Paul Berry wrote: > s/of/in/ Done. https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/byte_store.dart:13: abstract class ByteStore { On 2016/10/23 10:39:19, Paul Berry wrote: > Since this is a synchronous interface, it means that it would be impossible to > write implementations of [ByteStore] in the future that: > (a) share data across isolates. > (b) store data in a remote location (such as a cache located on a machine that > is shared by multiple users). > > Would it be worth switching this to an asynchronous interface? Since > AnalysisDriver is already asynchronous it sounds like we would not lose much. > And the future benefits might be really big. I thought about this. Maybe even global, LAN or company wide distributed analysis cache. And I wrote it this way initially. And tested performance difference between reading files sync and async. If we use async File.readAsBytes(), it takes 3 times longer to read 500 files than when we use File.readAsBytesSync(). OTOH, we can cheat and provide async interface using sync implementation. Then the difference is small, IIRC about 10%. I will switch it to async once I will have some basic implementation. This will cause some ripples, but probably not too bad. https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/byte_store.dart:18: List<int> get(String key); On 2016/10/23 10:39:19, Paul Berry wrote: > Since FileByteStore uses the key as a filename, we should document that the key > should be suitable for use as a filename (i.e. shouldn't be too long, shouldn't > contain characters like "/" or "\", shouldn't be "." or "..", etc.) Done. In theory as long as keys are short enough, it does not actually matters what they consist of - we can always transform keys into a form that is appropriate for the medium - file system, key/value store, etc. But we might never need this flexibility so yes, let's set limits. https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/byte_store.dart:27: * [ByteStore] with LRU cache. On 2016/10/23 10:39:19, Paul Berry wrote: > When I first read this comment it wasn't clear to me whether this was (a) an > implementation of [ByteStore] that stores data purely in memory, and discards it > when it is not recently used, or (b) a wrapper around another [ByteStore] which > adds in-memory LRU caching behavior to it. I now see that it's (b). > > Maybe change the comment to something like `A wrapper around [ByteStore] which > adds an in-memory LRU cache to it.` Done. https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/byte_store.dart:31: final int maxEntries; On 2016/10/23 10:39:19, Paul Berry wrote: > As a future improvement, consider measuring the limit in total stored bytes > rather than number of entries. That way we will be able to make much stronger > guarantees about memory usage. Agreed. We could look at Google Guava for inspiration :-) https://github.com/google/guava/wiki/CachesExplained https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... File pkg/analyzer/lib/src/dart/analysis/driver.dart (right): https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/driver.dart:20: * It can be shared between other [AnalysisDriver]s. On 2016/10/23 10:39:19, Paul Berry wrote: > s/between/with/. > > Also, yay! I like how you've designed for cross-context caching from the > beginning. Done. https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/driver.dart:45: * between priority files, not between them and not priority files. On 2016/10/23 10:39:19, Paul Berry wrote: > s/not priority/non-priority/ Done. https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/driver.dart:59: * guarantee that this will ever happen. On 2016/10/23 10:39:19, Paul Berry wrote: > I think we should make a stronger guarantee than this. To do so we have to > define some additional terminology. How about something like this: > > Let the set of "explicitly analyzed files" denote the set of paths that have > been passed to [addFile] but not subsequently passed to [removeFile]. Let the > "current analysis results" denote the map from the set of explicitly analyzed > files to the most recent [AnalysisResult] delivered to [results] for each file. > Let the "current file state" represent a map from file path to the file contents > most recently read from that file, or fetched from the content cache > (considering all possible possible file paths, regardless of whether they're in > the set of explicitly analyzed files). Let the "analysis state" be either > "analyzing" or "idle". > > (These are theoretical constructs; they may not necessarily reflect data > structures maintained explicitly by the driver). > > Then we make the following guarantees: > > - Whenever the analysis state is idle, the current analysis results are > consistent with the current file state. > > - A call to [addFile] or [changeFile] causes the analysis state to transition to > "analyzing", and schedules the file contents to be read into the current file > state prior to the next time the analysis state transitions back to "idle". > > - If at any time the client stops making calls to [addFile], [changeFile], and > [deleteFile], the analysis state will eventually transition back to "idle" after > a finite amount of processing. > > As a result of these guarantees, a client may ensure that the analysis results > are "eventually consistent" with the filesystem by simply calling [changeFile] > any time the contents of a file on the file system have changed. Thank you, Paul. I added this comment to the class documentation with a tiny change - when [addFile] or [changeFile] are called, I explicitly stated that only content of the given files will be read again. https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/driver.dart:90: * nor for any other file. On 2016/10/23 10:39:19, Paul Berry wrote: > If you take my suggestion above, you could replace this paragraph with something > like: > > Causes the analysis state to transition to "analyzing" (if it is not in that > state already). Schedules the file contents for [path] to be read into the > current file state prior to the next time the analysis state transitions to > "idle". Done. https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/driver.dart:96: */ On 2016/10/23 10:39:19, Paul Berry wrote: > Is it ok to pass a path to [changeFile] that is not in the set of explicitly > analyzed files? I hope so because it would be nice to be able to force analysis > to be updated when an implicitly analyzed file changes. Yes, it is OK to notify about implicitly analyzed files changes! I updated the comment. https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/driver.dart:105: * The [path] must be absolute and normalized. On 2016/10/23 10:39:19, Paul Berry wrote: > Is it required that the path is in the set of explicitly analyzed files? No, it isn't. The path can be any file - explicitly or implicitly analyzed, or neither. The users might navigate to an implicitly analyzed file, e.g. a used library in a package, and we need to produce the fully resolved unit for it. But the used might then open a sibling file in the package, which she does not use yet, but might want look on. So, we need to analyze and produce a fully resolved unit, at least in one of the contexts. The context need to "contain" the file and its dependencies in its SourceFactory. We might need a way to prevent "poisoning" the context with these "ad hoc" analyzed files -they might extend the set of implicitly analyzed files, even though the original file has never been an explicitly analyzed one, and the user closed the editor tab with it 5 seconds after opening. Actually, this is a problem even for explicitly analyzed files - libraries that were imported and the removed from the imports list. I will add TODO. https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/driver.dart:111: * state of the files. On 2016/10/23 10:39:19, Paul Berry wrote: > Can we make a stronger guarantee when the analysis state is idle? Done. https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/driver.dart:130: } On 2016/10/23 10:39:19, Paul Berry wrote: > Additional API that we may need to add in the future: Thanks, I will add this to the list. > > - A stream of "analysis status" information so that the client can know when the > analysis state is idle. > > - Some way to indicate when analysis state transitions between "idle" and > "analyzing". What is the difference between the first and the second? > > - Some way for the client to know when a file is added to or removed from the > set of implicitly analyzed files, in case it wants to watch implicitly analyzed > files for changes. (Note: in order to avoid a gap where the file is not being > watched, this would need to be a callback rather than a stream, so that the > client can be sure it starts watching the file before the driver reads it). > > - A single method call which is equivalent to calling [changeFile] on all > reachable files (both explicit and implicit). This would allow the client to > recover from file watching bugs. > > - The ability to abstract the file system using > pkg/front_end/lib/file_system.dart https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... File pkg/analyzer/lib/src/dart/analysis/file_byte_store.dart (right): https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/file_byte_store.dart:9: * [ByteStore] that stores values as [File]s. On 2016/10/23 10:39:19, Paul Berry wrote: > Should this class implement some sort of LRU policy so that the folder doesn't > grow without bound? Yes, we should. I'm adding TODO.
Message was sent while issue was closed.
https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... File pkg/analyzer/lib/src/dart/analysis/driver.dart (right): https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/driver.dart:130: } On 2016/10/23 20:54:34, scheglov wrote: > On 2016/10/23 10:39:19, Paul Berry wrote: > > > > - A stream of "analysis status" information so that the client can know when > the > > analysis state is idle. > > > > - Some way to indicate when analysis state transitions between "idle" and > > "analyzing". > > What is the difference between the first and the second? Oops, sorry. No difference. I'm not sure what I was thinking.
Message was sent while issue was closed.
Tests of the concrete byte stores would be good (if they weren't in some other CL that I haven't gotten to yet). Otherwise, lgtm. https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... File pkg/analyzer/lib/src/dart/analysis/byte_store.dart (right): https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/byte_store.dart:13: abstract class ByteStore { Those performance measures are consistent with Bob's earlier findings. https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/byte_store.dart:47: void put(String key, List<int> bytes) { Is there a need to be able to remove items from the store? As it is, the _keys and _map collections can only grow in size, which sounds like a memory leak in server. https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... File pkg/analyzer/lib/src/dart/analysis/driver.dart (right): https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/driver.dart:16: class AnalysisDriver { It looks like AnalysisDriver is the replacement for AnalysisContext. Is that a fair understanding? https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/driver.dart:78: void addFile(String path) { I think we'll want addFiles (plural) for performance reasons. I think we need to add URI's (or Sources), given that two different URI's can map to the same file.
Message was sent while issue was closed.
https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... File pkg/analyzer/lib/src/dart/analysis/byte_store.dart (right): https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/byte_store.dart:47: void put(String key, List<int> bytes) { On 2016/10/25 17:24:12, Brian Wilkerson wrote: > Is there a need to be able to remove items from the store? As it is, the _keys > and _map collections can only grow in size, which sounds like a memory leak in > server. AFAIK _evict() ensures that there is a limit on the number of elements in _keys and _map. https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... File pkg/analyzer/lib/src/dart/analysis/driver.dart (right): https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/driver.dart:16: class AnalysisDriver { On 2016/10/25 17:24:12, Brian Wilkerson wrote: > It looks like AnalysisDriver is the replacement for AnalysisContext. Is that a > fair understanding? It seems so - it produces analysis results, clients notify it about changes. https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/driver.dart:78: void addFile(String path) { On 2016/10/25 17:24:12, Brian Wilkerson wrote: > I think we'll want addFiles (plural) for performance reasons. > I think we need to add URI's (or Sources), given that two different URI's can > map to the same file. I don't know yet if we need addFiles(). Maybe as a potential optimization in the future. For now, we won't get any. void addFile(String path) { _explicitFiles.add(path); _filesToAnalyze.add(path); _hasWorkStreamController.add('do it!'); } I don't understand yet about different URIs for the same file. You have the file system, and you tell the driver to analyzer a file. This file is either in lib/ (so package:x/y.dart) or outside of it (so file://path/to/the/file.dart).
Message was sent while issue was closed.
https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... File pkg/analyzer/lib/src/dart/analysis/driver.dart (right): https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/driver.dart:78: void addFile(String path) { > I don't understand yet about different URIs for the same file. // foo/lib/a.dart import 'file:///c.dart' // foo/lib/b.dart import 'package:foo/c.dart' Unless we change the ecosystem, these imports have different URI's that both resolve to the same file, and we are required to treat them as importing different (albeit equivalent) libraries.
Message was sent while issue was closed.
https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... File pkg/analyzer/lib/src/dart/analysis/driver.dart (right): https://codereview.chromium.org/2442993002/diff/20001/pkg/analyzer/lib/src/da... pkg/analyzer/lib/src/dart/analysis/driver.dart:78: void addFile(String path) { On 2016/10/26 06:54:47, Brian Wilkerson wrote: > > I don't understand yet about different URIs for the same file. > > // foo/lib/a.dart > import 'file:///c.dart' > > // foo/lib/b.dart > import 'package:foo/c.dart' > > Unless we change the ecosystem, these imports have different URI's that both > resolve to the same file, and we are required to treat them as importing > different (albeit equivalent) libraries. OK, I see. Let's discuss this when we're all in the office. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
