Move subtree sizes over to TreeAndSubtrees, using the different
structure to represent the additional parse work that occurs, as well as
making it clear which functions require the extra information. My intent
is to make it hard to use this by accident.
The subtree size is still tracked during Parse::Tree construction. I
think a lot of that can be cleaned up, although we use it during
placeholder assignment so it may take some work. I wanted to see what
people thought about this before taking action on such a change.
I'm using a 1m line source file generated by #4124 for testing. Command
is `time bazel-bin/toolchain/install/prefix_root/bin/carbon compile
--phase=check --dump-mem-usage ~/tmp/data.carbon`
At head, what I'm seeing is:
```
...
parse_tree_.node_impls_:
used_bytes: 61516116
reserved_bytes: 61516116
...
Total:
used_bytes: 447814230
reserved_bytes: 551663894
...
1.43s user 0.14s system 99% cpu 1.565 total
```
With `Tree::Verify` disabled completely, it looks like:
```
parse_tree_.node_impls_:
used_bytes: 41010744
reserved_bytes: 41010744
...
Total:
used_bytes: 427308858
reserved_bytes: 531158522
...
1.20s user 0.13s system 99% cpu 1.332 total
```
Re-enabling just the basic verification (what is now `Tree::Verify`),
I'm seeing maybe 0.05s slower, but that's within noise for my system. I
do see variability in my timing results, and overall I think this is a
0.2s +/- 0.1s improvement versus the earlier (always testing `Extract`
code) implementation. That's opt; debug builds will be unaffected,
because the same checking occurs as before.
Note, the subtree size is a third of the node representation, which is
why I'm showing the decrease in memory usage here.
The purpose of this change is to allow something such as a FunctionDecl
instruction to note an imported instruction as the "loc_id". Note that
doesn't occur here: this change is already very sweeping in edits. There
is no testdata affected, intended to show equivalent behavior.
We might want to consolidate NodeId references towards LocationId, but
if that's preferred, I'd still like to split it out. A lot of this just
piping through LocationId where it's a build error otherwise, enough
that imports should be able to start using it for diagnostics.
ValueStores are added but still unused -- just flushing out structure
for review.
Restructuring SemIRLocation is necessary to use LocationId this way. For
TokenOnly, it's not getting used in Parse, so I migrated it to Check and
it's now specific to SemIRLocation.
I also considered making LocationId reference an InstId (which would
need to be an ImportRef) instead of an ImportIRInstId. However, that
would've required import.cpp to add instructions for decls which are
reached during resolution -- we typically don't have an inst ready for
use. An extra inst is essentially 16 bytes in InstId's ValueStore + 4
bytes in LocationId's ValueStore, whereas this is 8 bytes per.
As discussed around #3792, identify the import a diagnostic message came
from prior to the diagnostic message itself. This occurs during location
translation so that the logic can be central.
I'd considered associating the parse node with ImportRef instructions,
but I realized about halfway through that because I need to store the
ImportDirectiveId on the ImportIR for cross-package imports, it's there
for use in location translation without extra work. That saves a fair
amount of stringing it through declarations, as well as an oddity where
ImportRef instructions would have a node that didn't really represent
them.
Since the addition of TranslateArg, I don't think this type is going to
go away (cutting a TODO). Refactoring names slightly to fit the current
role, and adding const to ConvertLocation.
Jon Ross-Perkins