Requestify ExportedSourceFile parsing #66029
Conversation
hamishknight
requested review from
ahoppen,
bnbarham,
rintaro,
artemcm,
hborla,
slavapestov,
xedin,
CodaFi and
DougGregor
as code owners
hamishknight
force-pushed
the
you-shall-not-parse
branch
from
83f2b4f to
37ec11a
Compare
| !Context.LangOpts.hasFeature(Feature::ParserASTGen)) { | ||
| // We only need to do parsing if we either have ASTGen enabled, or want the | ||
| // new parser diagnostics. | ||
| auto needToParse = [&]() { |
There was a problem hiding this comment.
Was this to avoid negating the if's here 😅?
There was a problem hiding this comment.
Yeah I originally wrote it as:
if (!parsingOpts.contains(ParsingFlags::EnableASTGen) &&
!(parsingOpts.contains(ParsingFlags::EnableASTGenDiagnostics) && !suppressDiagnostics))
return;and then I said to myself: "I'd hate to read that code if I ever saw it in the wild", and so rewrote it as this 😅
There was a problem hiding this comment.
You could split it across two if and just negate with an or instead:
if (!parsingOpts.contains(ParsingFlags::EnableASTGen)) {
if (!parsingOpts.contains(ParsingFlags::EnableASTGenDiagnostics) || suppressDiagnostics)
return
}
I don't have a strong preference. I think yours is probably clearer.
There was a problem hiding this comment.
Hmm I kind of also dislike that too
test/Macros/lazy_parsing.swift
Outdated
| // RUN: %target-swift-frontend -typecheck -swift-version 5 -load-plugin-library %t/%target-library-name(MacroDefinition) -stats-output-dir %t/stats-no-lookup -primary-file %t/b.swift %t/a.swift | ||
| // RUN: %target-swift-frontend -typecheck -swift-version 5 -load-plugin-library %t/%target-library-name(MacroDefinition) -stats-output-dir %t/stats-lookup -primary-file %t/c.swift %t/a.swift | ||
|
|
||
| // RUN: %{python} %utils/process-stats-dir.py --evaluate-delta 'ExportedSourceFileRequest > 0' %t/stats-no-lookup %t/stats-lookup |
There was a problem hiding this comment.
Am I correct in my understanding that this is checking that there's more ExportedSourceFileRequests in lookup than no lookup? Ie. it's possible we could still have lookups in both? Is it worth doing a check similar to ensure_no_astgen.swift for the no lookup case?
There was a problem hiding this comment.
Am I correct in my understanding that this is checking that there's more ExportedSourceFileRequests in lookup than no lookup?
Yeah
it's possible we could still have lookups in both?
I guess it would be possible if we had a rogue SourceFile or something, or the caching of the request was broken, but otherwise the maximum should be 2, right?
I was trying to write this test bearing in mind that the numbers will change based on debug/release due to the round-trip checking. That being said, I realize now it won't work as expected because process-stats-dir doesn't handle missing counters well :/ Let me see if I can come up with something better.
There was a problem hiding this comment.
Figured I should just go ahead and fix the stat printing logic to output 0 counters, so updated this to check the absolute counters for both runs (but we still need to use <= to account for difference in debug vs release)
hamishknight
force-pushed
the
you-shall-not-parse
branch
from
37ec11a to
5988d4e
Compare
hamishknight
force-pushed
the
you-shall-not-parse
branch
2 times, most recently
from
4316a7a to
6d81466
Compare
|
Decided to remove the ASTGen enablement flags from the parsing options as we weren't taking advantage of customizing them per SourceFile, so keeping them on LangOptions seems fine. I expect we'll have fixed the performance issues with the new parser before we ever flip ASTGen on by default. |
@DougGregor Yeah that's unfortunate. I took a quick look at this, and it looks like we'll need to find a way to thread the ASTContext through to the printing logic, which doesn't seem entirely trivial, so I'm going to leave that as future work for now. |
This matches the behavior of `printAlwaysOnStatsAndTimers`, which we use in a release build. This fixes the diverging behavior, and ensures process-stats-dir can handle comparing deltas between runs where one of the runs had a 0 counter.
Avoid parsing the syntax tree up-front, and instead only parse it when required, which happens when either: 1. ASTGen parsing is enabled (currently disabled by default) 2. Round trip checking is enabled for a primary file (enabled by default in a debug build, except when dep scanning or doing an IDE operation) 3. We need to evaluate a macro in that file This change therefore means that we now no longer need to parse the syntax tree for secondary files by default unless we specifically need to evaluate a macro in them (e.g if we need to lookup a member on a decl with an attached macro). And the same for primaries in release builds. rdar://109283847
hamishknight
force-pushed
the
you-shall-not-parse
branch
from
6d81466 to
58d6694
Compare
|
@swift-ci please test |
Or something other than |
|
Yeah that's a good point, we'd still need to find a way to thread the Evaluator through tho |
Avoid parsing the syntax tree up-front, and instead only parse it when required, which happens when either:
This change therefore means that we now no longer need to parse the syntax tree for secondary files by default unless we specifically need to evaluate a macro in them (e.g if we need to lookup a member on a decl with an attached macro). And the same for primaries in release builds.
rdar://109283847