[M] Change in dart/sdk[main]: [vm,dyn_modules] Update some coverage information for bytecode.
Tess Strickland (Gerrit)
Tess Strickland voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Alexander Markov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Markov (Gerrit)
Alexander Markov added 4 comments![]( "Open in Gerrit")
writer.writePackedUInt30(flags);Should we also set `hasConstConstructorCoverageFlag` flag here?
if ((flags & kHasConstConstructorCoverageFlag) != 0) {We might need to read constructors lazily, only when requested. Otherwise we risk trying to read constructor references even before reading members of classes (as we may read source file info earlier, in order to create Script object for a Library or a Class).
TypedDataViewPtr kernel_constant_coverage() const;Can we have shared public API to query covered constant constructors with private kernel- and bytecode- specific implementation?
Related details
- Tess Strickland
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tess Strickland (Gerrit)
Tess Strickland added 5 comments![]( "Open in Gerrit")
Thanks, Alex! I'll ping again once I've reworked things to support lazy reading of the const constructor coverage info from bytecode and resolved the rest of the comments.
Should we also set `hasConstConstructorCoverageFlag` flag here?
Done
Should we also set `hasConstConstructorCoverageFlag` flag here?
Done
if ((flags & kHasConstConstructorCoverageFlag) != 0) {We might need to read constructors lazily, only when requested. Otherwise we risk trying to read constructor references even before reading members of classes (as we may read source file info earlier, in order to create Script object for a Library or a Class).
This makes sense, and explains why the kernel is read lazily. We can do the same for the bytecode, but we'll need some way to be able to get back to the bytecode from the script. Perhaps an alternate version of `KernelProgramInfo`, `BytecodeProgramInfo`, where the two have a common superclass that can be used for types of fields containing either? Hmm, will have to think about this.
#endifTess StricklandNit:
```
#else
UNREACHABLE();
```
Done
Related details
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Markov (Gerrit)
Alexander Markov added 1 comment![]( "Open in Gerrit")
if ((flags & kHasConstConstructorCoverageFlag) != 0) {Tess StricklandWe might need to read constructors lazily, only when requested. Otherwise we risk trying to read constructor references even before reading members of classes (as we may read source file info earlier, in order to create Script object for a Library or a Class).
This makes sense, and explains why the kernel is read lazily. We can do the same for the bytecode, but we'll need some way to be able to get back to the bytecode from the script. Perhaps an alternate version of `KernelProgramInfo`, `BytecodeProgramInfo`, where the two have a common superclass that can be used for types of fields containing either? Hmm, will have to think about this.
Yeah, I agree that reading the list of constructors lazily is also problematic and requires retaining at least object table indefinitely.
I have been thinking about this problem, and I'm wondering if we could replace this whole list of references to constructors with just a `CoveredConstConstructor` flag on a constructor. When reading functions, we could look at this flag and add constructor's `Function` object to a growable list of covered constructors in its script. WDYT? Could you try that?
Re: `BytecodeProgramInfo`: we can potentially keep BytecodeComponentData (`Array`) for incremental reading, but I think this should be a last resort. If the idea with `CoveredConstConstructor` won't work, then we can also try reading those constructor lists right after reading all classes in a library to avoid keeping any extra info after bytecode is loaded.
Related details
- Tess Strickland
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tess Strickland (Gerrit)
Tess Strickland added 3 comments![]( "Open in Gerrit")
Okay, have done the suggested refactoring, PTAL.
if ((flags & kHasConstConstructorCoverageFlag) != 0) {Tess StricklandWe might need to read constructors lazily, only when requested. Otherwise we risk trying to read constructor references even before reading members of classes (as we may read source file info earlier, in order to create Script object for a Library or a Class).
Alexander MarkovThis makes sense, and explains why the kernel is read lazily. We can do the same for the bytecode, but we'll need some way to be able to get back to the bytecode from the script. Perhaps an alternate version of `KernelProgramInfo`, `BytecodeProgramInfo`, where the two have a common superclass that can be used for types of fields containing either? Hmm, will have to think about this.
Yeah, I agree that reading the list of constructors lazily is also problematic and requires retaining at least object table indefinitely.
I have been thinking about this problem, and I'm wondering if we could replace this whole list of references to constructors with just a `CoveredConstConstructor` flag on a constructor. When reading functions, we could look at this flag and add constructor's `Function` object to a growable list of covered constructors in its script. WDYT? Could you try that?
Re: `BytecodeProgramInfo`: we can potentially keep BytecodeComponentData (`Array`) for incremental reading, but I think this should be a last resort. If the idea with `CoveredConstConstructor` won't work, then we can also try reading those constructor lists right after reading all classes in a library to avoid keeping any extra info after bytecode is loaded.
Done in PS11.
TypedDataViewPtr kernel_constant_coverage() const;Can we have shared public API to query covered constant constructors with private kernel- and bytecode- specific implementation?
`CollectConstConstructorCoverageFrom` is the already existing shared public API. I've marked the other functions as private, with an appropriate friend declaration for `bytecode::BytecodeReaderHelper`.
Related details
- Alexander Markov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Markov (Gerrit)
Alexander Markov added 4 comments![]( "Open in Gerrit")
if (options.emitSourcePositions && includeSourceInfo) {
if (source.lineStarts == null) {
LineStarts lineStarts = new LineStarts(astSource.lineStarts!);
bytecodeComponent.lineStarts.add(lineStarts);
source.lineStarts = lineStarts;
}Nit: seems like an unnecessary change, can it be reverted?
static const isCoveredConstConstructorFlag = 1 << 24;Please add the new flag to bytecode.md.
constant_coverage = Array::Grow(constant_coverage, new_index + 1);Growing this array every time would result in O(N^2) reading time, where N is a number of covered const constructors. Can we use `GrowableObjectArray` instead?
script.set_collected_constant_coverage(constant_coverage);I'm wondering if we could make a yet another step in this direction and add a flag to the VM's `Function`, maybe to `STATE_BITS_LIST`? This way we won't need to store the list of covered constructors at all and can collect them from `Class::functions()` at any time.
Related details
- Tess Strickland
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tess Strickland (Gerrit)
Tess Strickland added 2 comments![]( "Open in Gerrit")
if (options.emitSourcePositions && includeSourceInfo) {
if (source.lineStarts == null) {
LineStarts lineStarts = new LineStarts(astSource.lineStarts!);
bytecodeComponent.lineStarts.add(lineStarts);
source.lineStarts = lineStarts;
}Nit: seems like an unnecessary change, can it be reverted?
Done
static const isCoveredConstConstructorFlag = 1 << 24;Please add the new flag to bytecode.md.
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tess Strickland (Gerrit)
Tess Strickland added 2 comments![]( "Open in Gerrit")
constant_coverage = Array::Grow(constant_coverage, new_index + 1);Growing this array every time would result in O(N^2) reading time, where N is a number of covered const constructors. Can we use `GrowableObjectArray` instead?
Mooted due to other changes described below.
script.set_collected_constant_coverage(constant_coverage);I'm wondering if we could make a yet another step in this direction and add a flag to the VM's `Function`, maybe to `STATE_BITS_LIST`? This way we won't need to store the list of covered constructors at all and can collect them from `Class::functions()` at any time.
I did this change, but while doing it and looking at the kernel version, I realized that we _can't_ mark the constructor as being constant evaluated at bytecode generation time, or at least we need to be able to update whether it's constant evaluated due to _other_ scripts. This is why the kernel version is lazy--because you have to check the constant evaluation information for all loaded scripts, not just the ones containing const constructors, to determine if any const constructors were constant evaluated in the front end.
That also means making it a state bit on the function is probably not a good idea, because whether or not the constructor was evaluated at const evaluation time depends on which scripts are loaded: for example, let's say a const constructor `C` is defined (but unused) in `script1` and the code in `script2` uses it while creating a constant. Then we can have the following sequence of events:
- `script1` is loaded
- coverage info is requested, `C` is reported as unexecuted
- `script2` is loaded
- coverage info is re-requested, now `C` is reported as executed
- `script2` is reloaded with a version that removes the use of `C`
- coverage info is re-requested, now `C` should be reported as unexecuted again.
- If we make it a state bit on the function, then unless this bit is appropriately recalculated on all functions whenever any script is unloaded/reloaded, then we could have bad coverage information.
This also means we almost certainly need the last resort you mentioned in https://dart-review.googlesource.com/c/sdk/+/449900/comment/b971d1d4_d1be06f3/, so we can appropriately compute this information as needed.
I'll ping you to take a look after I've done that.
Related details
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |