[XL] Change in dart/sdk[main]: [vm] Remove the VM isolate.
Ryan Macnak (Gerrit)
Ryan Macnak added 3 comments![]( "Open in Gerrit")
// VMOptions=--hash_map_probes_limit=3000(Somehow caused by the symbol aliases.)
Related details
- Alexander Markov
- 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. |
SLSA Policy Verification Service (Gerrit)
SLSA Policy Verification Service voted SLSA-Policy-Verified+1![]( "Open in Gerrit")
| SLSA-Policy-Verified | +1 |
SLSA Policy Verification Service (Gerrit)
SLSA Policy Verification Service voted SLSA-Policy-Verified+1![]( "Open in Gerrit")
| SLSA-Policy-Verified | +1 |
Related details
- Alexander Markov
- 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. |
Alexander Markov (Gerrit)
Alexander Markov voted and added 11 comments![]( "Open in Gerrit")
Votes added by Alexander Markov
| Code-Review | +1 |
11 comments
Nice!
Please address the remaining failures before landing.
/// "_kVmSnapshotInstructions", "_kVmIsolateData" and "_kVmIsolateInstructions"Nit: please update
// VMOptions=--hash_map_probes_limit=2500025k sounds like a lot of hash collisions. We should probably check which hash map caused so many collisions and improve corresponding hashcode.
(Consider filing an issue / increasing to a smaller value.)
static char* InitializeGroupFlagsFromSnapshot(const Snapshot* snapshot);Nit: IsolateGroup
// if (!current_table.IsNull()) {Nit: please do not leave commented out code (either delete or uncomment).
if (false && code == StubCode::LazyCompile().ptr()) {Delete this code; consider dropping special index 0 and `1 +` below.
const bool stubs_in_vm_isolate = false;Nit: remove (also on other architectures)
void FixNulls();Nit: it is not clear what this function is doing. Consider adding a comment or picking a more descriptive name.
if (IsUnknownDartCode(code)) return kUwordMax;ditto
if (IsUnknownDartCode(code)) return 0;Why is this removed? `UnknownDartCode` is used in AOT when `Code` object is discarded. It can be used for any function in the program, so its payload start/size are defined to include the whole address space.
#if !defined(PRODUCT)Why do we need local var descriptors in AOT?
Related details
- Ryan Macnak
- 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. |
SLSA Policy Verification Service (Gerrit)
SLSA Policy Verification Service voted SLSA-Policy-Verified+1![]( "Open in Gerrit")
| SLSA-Policy-Verified | +1 |
Related details
- Ryan Macnak
- 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. |
SLSA Policy Verification Service (Gerrit)
SLSA Policy Verification Service voted SLSA-Policy-Verified+1![]( "Open in Gerrit")
| SLSA-Policy-Verified | +1 |
Related details
- Ryan Macnak
- 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. |
Ryan Macnak (Gerrit)
Ryan Macnak voted and added 10 comments![]( "Open in Gerrit")
Votes added by Ryan Macnak
| Commit-Queue | +1 |
10 comments
/// "_kVmSnapshotInstructions", "_kVmIsolateData" and "_kVmIsolateInstructions"Ryan MacnakNit: please update
Done
// VMOptions=--hash_map_probes_limit=2500025k sounds like a lot of hash collisions. We should probably check which hash map caused so many collisions and improve corresponding hashcode.
(Consider filing an issue / increasing to a smaller value.)
It has to do with the symbol aliases for kDartIsolateSnapshotData -> kDartSnapshotData and by_label_index. In the child CL that removes the aliases, this is set back to the current value.
static char* InitializeGroupFlagsFromSnapshot(const Snapshot* snapshot);Ryan MacnakNit: IsolateGroup
Done
Nit: please do not leave commented out code (either delete or uncomment).
Restored check for AOT and JIT snapshots.
if (false && code == StubCode::LazyCompile().ptr()) {Delete this code; consider dropping special index 0 and `1 +` below.
Done
Nit: remove (also on other architectures)
Done
Nit: it is not clear what this function is doing. Consider adding a comment or picking a more descriptive name.
Done
if (IsUnknownDartCode(code)) return kUwordMax;Ryan Macnakditto
Acknowledged
if (IsUnknownDartCode(code)) return 0;Why is this removed? `UnknownDartCode` is used in AOT when `Code` object is discarded. It can be used for any function in the program, so its payload start/size are defined to include the whole address space.
The UnknownDartCode stub itself has instructions / non-zero size. If it claims to have 0 size, this confuses things like filling in InstructionsTable after deserialization. I think this only worked by accident before because the UnknownDartCode is the very last stub in the VM isolate.
Why do we need local var descriptors in AOT?
We don't need the var descriptors. What we need here is agreement between the serializer and deserializer about how many fields Bytecode has. In practice, this is only going to cover ~dozen bytecode stubs, so a more complicated set of ifdef/to_snapshot(kind) would only save a ~dozen bytes in the snapshot.
Related details
- Alexander Markov
- 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. |
Alexander Markov (Gerrit)
Alexander Markov voted and added 2 comments![]( "Open in Gerrit")
Votes added by Alexander Markov
| Code-Review | +1 |
2 comments
if (IsUnknownDartCode(code)) return 0;Ryan MacnakWhy is this removed? `UnknownDartCode` is used in AOT when `Code` object is discarded. It can be used for any function in the program, so its payload start/size are defined to include the whole address space.
The UnknownDartCode stub itself has instructions / non-zero size. If it claims to have 0 size, this confuses things like filling in InstructionsTable after deserialization. I think this only worked by accident before because the UnknownDartCode is the very last stub in the VM isolate.
Note that it's a `PayloadStartOf`, not size. UnknownDartCode starts at 0 and has a size of kUwordMax, so it claims the whole address space 0...kUwordMax range to contain all possible code. We can special case it during deserialization if needed.
(The instructions of UnknownDartCode are not actually used. The Code object of this stub is used as Function::CurrentCode() when their own Code object is discarded. It's a snapshot size & memory usage optimization in AOT/PRODUCT mode.)
#if !defined(PRODUCT)Ryan MacnakWhy do we need local var descriptors in AOT?
We don't need the var descriptors. What we need here is agreement between the serializer and deserializer about how many fields Bytecode has. In practice, this is only going to cover ~dozen bytecode stubs, so a more complicated set of ifdef/to_snapshot(kind) would only save a ~dozen bytes in the snapshot.
There could be many `Bytecode` objects at runtime (e.g. if dynamic module(s) are loaded). In such a case, extra field in Bytecode object in AOT mode would result in the unnecessary increase in memory usage. Can we omit it?
Related details
- Ryan Macnak
- 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. |
SLSA Policy Verification Service (Gerrit)
SLSA Policy Verification Service voted SLSA-Policy-Verified+1![]( "Open in Gerrit")
| SLSA-Policy-Verified | +1 |
Related details
- Ryan Macnak
- 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. |
SLSA Policy Verification Service (Gerrit)
SLSA Policy Verification Service voted SLSA-Policy-Verified+1![]( "Open in Gerrit")
| SLSA-Policy-Verified | +1 |
Related details
- Ryan Macnak
- 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. |
Ryan Macnak (Gerrit)
Ryan Macnak voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ryan Macnak
| Commit-Queue | +1 |
1 comment
#if !defined(PRODUCT)Ryan MacnakWhy do we need local var descriptors in AOT?
Alexander MarkovWe don't need the var descriptors. What we need here is agreement between the serializer and deserializer about how many fields Bytecode has. In practice, this is only going to cover ~dozen bytecode stubs, so a more complicated set of ifdef/to_snapshot(kind) would only save a ~dozen bytes in the snapshot.
There could be many `Bytecode` objects at runtime (e.g. if dynamic module(s) are loaded). In such a case, extra field in Bytecode object in AOT mode would result in the unnecessary increase in memory usage. Can we omit it?
Oh, right, the heap size for the ones not in the snapshot. Done.
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. |
SLSA Policy Verification Service (Gerrit)
SLSA Policy Verification Service voted SLSA-Policy-Verified+1![]( "Open in Gerrit")
- Ryan Macnak
- 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. |
SLSA Policy Verification Service (Gerrit)
SLSA Policy Verification Service voted SLSA-Policy-Verified+1![]( "Open in Gerrit")
| SLSA-Policy-Verified | +1 |
Related details
- Ryan Macnak
- 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")
I'm still looking through this, but just wanted to note some things as I'm going.
// For the build ID, we generate a 128-bit hash, where each 32 bits is a hash of
// the contents of the following segments in order:
//
// .text(Isolate) | .rodata(Isolate)Not a blocker, but we should create an issue to fix up the hashing so that we're not just generating constant 0s as part of the build ID.
A quick stop-gap would be to change `BitsContainer::Hash` to return a uint64_t and just hash each half of the contents separately using `Utils::StringHash`, then concatenate the two results. It's not perfect but it'll do for now.
uint32_t hashes[4];
COMPILE_ASSERT(sizeof(hashes) == 16); // 128-bit UUID
hashes[0] = 0;
hashes[1] = HashPortion(*isolate_instructions);
hashes[2] = 0;
hashes[3] = HashPortion(*isolate_data);Ditto from `elf.cc`, just repeating the contents here:
Not a blocker, but we should create an issue to fix up the hashing so that we're not just generating constant 0s as part of the build ID.
A quick stop-gap would be to change `HashPortion` to return a uint64_t and have it just hash each half of the contents separately using `Utils::StringHash`, then concatenate the two results for the return value. It's not perfect but it'll do for now.
(Specific to `mach_o.cc`: And changing this part to be `uint64_t hashes[2];` ofc.)
Related details
- Ryan Macnak
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |