Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2871783002:
Start using FileState/FileSystemState to provide consistent view. (Closed)
|
Created:
3 years, 7 months ago by scheglov Modified:
3 years, 7 months ago CC:
reviews_dartlang.org, dart-fe-team+reviews_google.com Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionStart using FileState/FileSystemState to provide consistent view.
R=paulberry@google.com, sigmund@google.com
BUG=
Committed: https://github.com/dart-lang/sdk/commit/6173ac476bf399bd89e65ca70834642cefc31580
Patch Set 1 #
Total comments: 32
Patch Set 2 : Updates for review comments. #
Messages
Total messages: 8 (1 generated)
lgtm. Your call on how to address the comments below. https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... File pkg/front_end/lib/src/incremental/file_state.dart (right): https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... pkg/front_end/lib/src/incremental/file_state.dart:30: String _content; Is it necessary to keep both _contentBytes and _content? Seems like a waste of memory. https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... pkg/front_end/lib/src/incremental/file_state.dart:97: for (String import in listener.imports) { Can we rename "import" to "importedFile" or something? Seeing the keyword "import" in this line and the one below is really confusing. https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... pkg/front_end/lib/src/incremental/file_state.dart:100: for (String export in listener.exports) { Similar issue here https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... pkg/front_end/lib/src/incremental/file_state.dart:103: for (String export in listener.parts) { Here we should rename "export" to something like "partFile" https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... pkg/front_end/lib/src/incremental/file_state.dart:106: // TODO(scheglov) make this optional Why? All files import 'dart:core' (either implicitly or explicitly), so there's no circumstance where we can truly drop the dependency. The only benefit would be to optimize the case case where the file explicitly imports 'dart:core', but it's hard to imagine that having a measurable performance impact. https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... pkg/front_end/lib/src/incremental/file_state.dart:209: throw new UnimplementedError(); Maybe throw a StateError() here with a message saying something like "FileSystemViewEntry modification stamp should not be queried", just so that someone doesn't come along in the future and think that they need to implement this method. (I'm still hoping we can eliminate this method from the API so that the front_end never thinks about timestamps. Rationale: some of the concrete filesystems we might imagine hooking up to in the future don't provide timestamps (e.g. reading files using HTTP get requests)) https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... File pkg/front_end/lib/src/incremental_kernel_generator_impl.dart (right): https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:121: /// cannot be asynchronous. Alternatively, we could change IncrementalKernelGenerator's factory constructor to a static method returning Future<IncrementalKernelGenerator>. The implementation of this method could get the asynchronous data from options before constructing the IncrementalKernelGeneratorImpl.
lgtm for me too - I mainly have nits and small suggestions. https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... File pkg/front_end/lib/src/incremental/file_state.dart (right): https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... pkg/front_end/lib/src/incremental/file_state.dart:15: /// Information about a file being analyzed, explicitly or implicitly. nit: now that this includes compilation maybe "analyzed" is too specific to analyzer :)? Other ideas "compiled" or "processed"? https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... pkg/front_end/lib/src/incremental/file_state.dart:34: List<FileState> _partedFiles; nit: rename to _partfiles https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... pkg/front_end/lib/src/incremental/file_state.dart:76: // Read the content. I expect `refresh` will only get called on files that someone marked via "IncrementalAPI.invalidate", correct (that's way if I'm watching for modifications outside the front_end to provide fine-grain invalidate() calls, then there wont be a referesh for unmodified files?) https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... pkg/front_end/lib/src/incremental/file_state.dart:97: for (String import in listener.imports) { On 2017/05/08 22:10:49, Paul Berry wrote: > Can we rename "import" to "importedFile" or something? Seeing the keyword > "import" in this line and the one below is really confusing. nit: I'd use `var` instead of `String` For the variable name: `uri` seems descriptive enoguh for all three loops. https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... pkg/front_end/lib/src/incremental/file_state.dart:101: await _addFileForRelativeUri(_exportedFiles, export); optional - it appears to me that it is safe to parallelize here. For example, do: addImport(uri) => _addFileForRelativeUri(_importedFiles, uri); _importedFiles = await Future.wait(listener.imports.map(addImport).toList()); addExport(uri) => _addFileForRelativeUri(_exportedFiles, uri); _exportedFiles = await Future.wait(listener.exports.map(addExport).toList()); addPart(uri) => _addFileForRelativeUri(_partFiles, uri); _partFiles = await Future.wait(listener.parts.map(addPart).toList()); (technically we could also do all three group in parallel too). https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... pkg/front_end/lib/src/incremental/file_state.dart:120: if (relativeUri.isEmpty) { optional nit: I tend to prefer no braces on these one-line ifs, especially for `return` statements (see second example of https://www.dartlang.org/guides/language/effective-dart/style#do-use-curly-br...) https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... pkg/front_end/lib/src/incremental/file_state.dart:136: Uri fileUri = _fsState.uriTranslator.translate(absoluteUri); note that this shadows FielState.fileUri - it might be worth using a different name here. Possibly: resolvedUri? https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... pkg/front_end/lib/src/incremental/file_state.dart:137: if (fileUri == null) { same here about one-line if ("if (fileUri == null) return;")
https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... File pkg/front_end/lib/src/incremental/file_state.dart (right): https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... pkg/front_end/lib/src/incremental/file_state.dart:15: /// Information about a file being analyzed, explicitly or implicitly. On 2017/05/08 23:55:17, Siggi Cherem (dart-lang) wrote: > nit: now that this includes compilation maybe "analyzed" is too specific to > analyzer :)? > > Other ideas "compiled" or "processed"? I will changed it to "compiled". https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... pkg/front_end/lib/src/incremental/file_state.dart:30: String _content; On 2017/05/08 22:10:48, Paul Berry wrote: > Is it necessary to keep both _contentBytes and _content? Seems like a waste of > memory. I think you're right. I will fix this in the next CL. https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... pkg/front_end/lib/src/incremental/file_state.dart:34: List<FileState> _partedFiles; On 2017/05/08 23:55:18, Siggi Cherem (dart-lang) wrote: > nit: rename to _partfiles Done. https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... pkg/front_end/lib/src/incremental/file_state.dart:76: // Read the content. On 2017/05/08 23:55:17, Siggi Cherem (dart-lang) wrote: > I expect `refresh` will only get called on files that someone marked via > "IncrementalAPI.invalidate", correct (that's way if I'm watching for > modifications outside the front_end to provide fine-grain invalidate() calls, > then there wont be a referesh for unmodified files?) Correct. We read each file at least once when refresh() the entry point and create the graph of FileState instances (see FileSystemState.getFile()). Then any file is re-read only if it is invalidated explicitly. If it is imported by an invalidated file, and is not invalidated itself, then it will not be re-read. https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... pkg/front_end/lib/src/incremental/file_state.dart:97: for (String import in listener.imports) { On 2017/05/08 22:10:49, Paul Berry wrote: > Can we rename "import" to "importedFile" or something? Seeing the keyword > "import" in this line and the one below is really confusing. Done. https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... pkg/front_end/lib/src/incremental/file_state.dart:97: for (String import in listener.imports) { On 2017/05/08 23:55:18, Siggi Cherem (dart-lang) wrote: > On 2017/05/08 22:10:49, Paul Berry wrote: > > Can we rename "import" to "importedFile" or something? Seeing the keyword > > "import" in this line and the one below is really confusing. > > nit: I'd use `var` instead of `String` > > For the variable name: `uri` seems descriptive enoguh for all three loops. I don't see the type of "listener.imports". It is not created, and it is not a typed local variable. I agree about `uri`. https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... pkg/front_end/lib/src/incremental/file_state.dart:100: for (String export in listener.exports) { On 2017/05/08 22:10:49, Paul Berry wrote: > Similar issue here Done. https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... pkg/front_end/lib/src/incremental/file_state.dart:101: await _addFileForRelativeUri(_exportedFiles, export); On 2017/05/08 23:55:17, Siggi Cherem (dart-lang) wrote: > optional - it appears to me that it is safe to parallelize here. For example, > do: > > > addImport(uri) => _addFileForRelativeUri(_importedFiles, uri); > _importedFiles = await Future.wait(listener.imports.map(addImport).toList()); > > addExport(uri) => _addFileForRelativeUri(_exportedFiles, uri); > _exportedFiles = await Future.wait(listener.exports.map(addExport).toList()); > > addPart(uri) => _addFileForRelativeUri(_partFiles, uri); > _partFiles = await Future.wait(listener.parts.map(addPart).toList()); > > (technically we could also do all three group in parallel too). That's an interesting idea. I will keep it in mind while experimenting with performance optimizations. https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... pkg/front_end/lib/src/incremental/file_state.dart:103: for (String export in listener.parts) { On 2017/05/08 22:10:49, Paul Berry wrote: > Here we should rename "export" to something like "partFile" Done. https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... pkg/front_end/lib/src/incremental/file_state.dart:106: // TODO(scheglov) make this optional On 2017/05/08 22:10:49, Paul Berry wrote: > Why? All files import 'dart:core' (either implicitly or explicitly), so there's > no circumstance where we can truly drop the dependency. The only benefit would > be to optimize the case case where the file explicitly imports 'dart:core', but > it's hard to imagine that having a measurable performance impact. Well, yeah, I thought about adding it only if dart:core is not imported explicitly. I will change to make it imported unconditionally. https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... pkg/front_end/lib/src/incremental/file_state.dart:120: if (relativeUri.isEmpty) { On 2017/05/08 23:55:17, Siggi Cherem (dart-lang) wrote: > optional nit: I tend to prefer no braces on these one-line ifs, especially for > `return` statements (see second example of > https://www.dartlang.org/guides/language/effective-dart/style#do-use-curly-br...) Done. https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... pkg/front_end/lib/src/incremental/file_state.dart:136: Uri fileUri = _fsState.uriTranslator.translate(absoluteUri); On 2017/05/08 23:55:17, Siggi Cherem (dart-lang) wrote: > note that this shadows FielState.fileUri - it might be worth using a different > name here. Possibly: resolvedUri? > Done. https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... pkg/front_end/lib/src/incremental/file_state.dart:137: if (fileUri == null) { On 2017/05/08 23:55:17, Siggi Cherem (dart-lang) wrote: > same here about one-line if ("if (fileUri == null) return;") Done. https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... pkg/front_end/lib/src/incremental/file_state.dart:209: throw new UnimplementedError(); On 2017/05/08 22:10:49, Paul Berry wrote: > Maybe throw a StateError() here with a message saying something like > "FileSystemViewEntry modification stamp should not be queried", just so that > someone doesn't come along in the future and think that they need to implement > this method. > > (I'm still hoping we can eliminate this method from the API so that the > front_end never thinks about timestamps. Rationale: some of the concrete > filesystems we might imagine hooking up to in the future don't provide > timestamps (e.g. reading files using HTTP get requests)) Done. I agree, this API might not always be accessible. https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... File pkg/front_end/lib/src/incremental_kernel_generator_impl.dart (right): https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:121: /// cannot be asynchronous. On 2017/05/08 22:10:49, Paul Berry wrote: > Alternatively, we could change IncrementalKernelGenerator's factory constructor > to a static method returning Future<IncrementalKernelGenerator>. The > implementation of this method could get the asynchronous data from options > before constructing the IncrementalKernelGeneratorImpl. ACK That's what I had in mind too.
Description was changed from ========== Start using FileState/FileSystemState to provide consistent view. R=paulberry@google.com, sigmund@google.com BUG= ========== to ========== Start using FileState/FileSystemState to provide consistent view. R=paulberry@google.com, sigmund@google.com BUG= Committed: https://github.com/dart-lang/sdk/commit/6173ac476bf399bd89e65ca70834642cefc31580 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) manually as 6173ac476bf399bd89e65ca70834642cefc31580 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... File pkg/front_end/lib/src/incremental/file_state.dart (right): https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... pkg/front_end/lib/src/incremental/file_state.dart:97: for (String import in listener.imports) { On 2017/05/09 00:16:28, scheglov wrote: > On 2017/05/08 23:55:18, Siggi Cherem (dart-lang) wrote: > > On 2017/05/08 22:10:49, Paul Berry wrote: > > > Can we rename "import" to "importedFile" or something? Seeing the keyword > > > "import" in this line and the one below is really confusing. > > > > nit: I'd use `var` instead of `String` > > > > For the variable name: `uri` seems descriptive enoguh for all three loops. > > I don't see the type of "listener.imports". > It is not created, and it is not a typed local variable. > I agree about `uri`. Not sure why that's not working: listener.imports is declared as a Set<String> in DirectiveListener, so I expect this should be inferred here :-/
Message was sent while issue was closed.
https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... File pkg/front_end/lib/src/incremental/file_state.dart (right): https://codereview.chromium.org/2871783002/diff/1/pkg/front_end/lib/src/incre... pkg/front_end/lib/src/incremental/file_state.dart:97: for (String import in listener.imports) { On 2017/05/09 00:26:15, Siggi Cherem (dart-lang) wrote: > On 2017/05/09 00:16:28, scheglov wrote: > > On 2017/05/08 23:55:18, Siggi Cherem (dart-lang) wrote: > > > On 2017/05/08 22:10:49, Paul Berry wrote: > > > > Can we rename "import" to "importedFile" or something? Seeing the keyword > > > > "import" in this line and the one below is really confusing. > > > > > > nit: I'd use `var` instead of `String` > > > > > > For the variable name: `uri` seems descriptive enoguh for all three loops. > > > > I don't see the type of "listener.imports". > > It is not created, and it is not a typed local variable. > > I agree about `uri`. > > Not sure why that's not working: listener.imports is declared as a Set<String> > in DirectiveListener, so I expect this should be inferred here :-/ Ah, sorry, I should have been more explicit. The type is inferred. But I don't know it just by looking on code, without using IDE to learn it. I'm fine with switching to not typing such cycles, if this is the code style we want in front_end. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
