Load docstrings lazily from TASTy #20148
Conversation
Now we are always able to load docstings from TASTy, even if `-Yread-docs` is not set. The `-Yread-docs` flag loads the doc strings eagerly, as it did before. Fixes scala#20106
nicolasstucki
force-pushed
the
fix-20106
branch
from
b47ed81
to
4a11a61
Compare
Reading it from the tree would be unreliable as doc might not have been loaded yet. Doc stings should be accessed from the `ContextDocstrings`.
There was a problem hiding this comment.
When do we actually read comments? My concern is that creates a huge map in all cases now. We carefully engineered the Tasty unpickler to be fast and not create too much memory. Now we are throwing that out of the window for something as trivial as reading comments. I believe if we do that we have to first optimize comment handling as thoroughly as the rest of the representations. Right mow it is a completely different standard, nobody cared very much about efficiency, since we believed that in high-stress scenarios (clean build of a large code base) comments would not be read anyway.
Unrelated: I have a similar concern for unused warnings which do something relatively "easy" yet need a significant part of the compilation time, simply because they have not been optimized well.
Alternatives: Can we create/populate the map only once the first comment is read? Can we read the Tasty buffer twice to get comments? Or maybe have a more compact representation of where comments start, so that comments can be easily read on demand afterwards? Note that using a util.IntMap does not allocate any value objects. Maybe one could use that to get the comment start of a symbol.
| @@ -31,7 +31,7 @@ class SnippetCompiler( | |||
| rootCtx.setSetting(rootCtx.settings.experimental, true) | |||
| rootCtx.setSetting(rootCtx.settings.YretainTrees, true) | |||
| rootCtx.setSetting(rootCtx.settings.YcookComments, true) | |||
| rootCtx.setSetting(rootCtx.settings.YreadComments, true) | |||
| rootCtx.setSetting(rootCtx.settings.YreadDocsEagerly, true) | |||
There was a problem hiding this comment.
What is a usage scenario where I might want readDocsEargerly?
There was a problem hiding this comment.
None. I just kept to be able to benchmark the difference in performance.
SethTisue
changed the title
|
We need to address the concerns and explore the alternatives described in #20148 (review) |
Now we are always able to load docstrings from TASTy, even if
-Yread-docsis not set. The-Yread-docsflag loads the doc strings eagerly, as it did before.Fixes #20106