[S] Change in dart/sdk[main]: [vm,dyn_modules] Remove CompilerState use from CallMarshaller.
Tess Strickland (Gerrit)
Tess Strickland added 3 comments![]( "Open in Gerrit")
Have moved the class from the compiler state to the object store after trying the cid check version, but then realizing that doesn't work since this is about types that use the abstract classes from `typed_data.dart`, not instances of the concrete classes in the VM patch which get cids from that range. PTAL again!
const auto& typed_data_cls = compiler_state.TypedDataClass();Tess StricklandThis looks like a workaround and not a real fix. Instead, could you change `IsTypedDataPointer` to use cid instead of a Class object (or move TypedDataClass from CompilerState to ObjectStore and initialize it eagerly)?
Done.
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 2 comments![]( "Open in Gerrit")
auto& typed_data_cls = Class::Handle(zone_, object_store->typed_data_class());Where do we set ObjectStore::typed_data_class()? It seems like it is always null.
typed_data_cls = lib.LookupClass(Symbols::TypedData());We should ensure that this lazy lookup does not happen during NoSafepointScope in the interpreted FFI call, so we should probably lookup this class eagerly, e.g. during ObjectStore initialization.
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")
Thanks, Alex! PTAL again, in particular at the response to your second comment.
auto& typed_data_cls = Class::Handle(zone_, object_store->typed_data_class());Where do we set ObjectStore::typed_data_class()? It seems like it is always null.
Done
typed_data_cls = lib.LookupClass(Symbols::TypedData());We should ensure that this lazy lookup does not happen during NoSafepointScope in the interpreted FFI call, so we should probably lookup this class eagerly, e.g. during ObjectStore initialization.
Originally tried adding to `ObjectStore::InitKnownObjects()`, but apparently that's too early as looking up the `TypedData` class then returns null.
I'd make a `LAZY_TYPED_DATA` part of the ObjectStore, but then that would have the same issue. Looking over `Library::LookupClass` and the methods it calls, though, I don't see anything that would cause an issue with being under a `NoSafepointScope` (and the fact that the tests now work on `vm-dyn-linux-debug-x64` without triggering the no safepoint scope check in debug mode suggests I'm correct).
If this is still a concern, though, I guess I could add a check in the body of the `BaseMarshaller` constructor to see if it's null and do the lookup/cache there? LMK if so.
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. |
Tess Strickland (Gerrit)
Tess Strickland added 1 comment![]( "Open in Gerrit")
typed_data_cls = lib.LookupClass(Symbols::TypedData());Tess StricklandWe should ensure that this lazy lookup does not happen during NoSafepointScope in the interpreted FFI call, so we should probably lookup this class eagerly, e.g. during ObjectStore initialization.
Originally tried adding to `ObjectStore::InitKnownObjects()`, but apparently that's too early as looking up the `TypedData` class then returns null.
I'd make a `LAZY_TYPED_DATA` part of the ObjectStore, but then that would have the same issue. Looking over `Library::LookupClass` and the methods it calls, though, I don't see anything that would cause an issue with being under a `NoSafepointScope` (and the fact that the tests now work on `vm-dyn-linux-debug-x64` without triggering the no safepoint scope check in debug mode suggests I'm correct).
If this is still a concern, though, I guess I could add a check in the body of the `BaseMarshaller` constructor to see if it's null and do the lookup/cache there? LMK if so.
(That said, I could still move it to a `LAZY_TYPED_DATA` part of the `ObjectStore` if we think it might get used elsewhere as well, but since currently it's only used by the `BaseMarshaller` I figured doing the lookup/cache there is fine until it has another use to justify adding a new argument to `OBJECT_STORE_FIELD_LIST` and its uses.)
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. |
Tess Strickland (Gerrit)
Tess Strickland voted and added 1 comment![]( "Open in Gerrit")
Votes added by Tess Strickland
| Commit-Queue | +1 |
1 comment
typed_data_cls = lib.LookupClass(Symbols::TypedData());Tess StricklandWe should ensure that this lazy lookup does not happen during NoSafepointScope in the interpreted FFI call, so we should probably lookup this class eagerly, e.g. during ObjectStore initialization.
Tess StricklandOriginally tried adding to `ObjectStore::InitKnownObjects()`, but apparently that's too early as looking up the `TypedData` class then returns null.
I'd make a `LAZY_TYPED_DATA` part of the ObjectStore, but then that would have the same issue. Looking over `Library::LookupClass` and the methods it calls, though, I don't see anything that would cause an issue with being under a `NoSafepointScope` (and the fact that the tests now work on `vm-dyn-linux-debug-x64` without triggering the no safepoint scope check in debug mode suggests I'm correct).
If this is still a concern, though, I guess I could add a check in the body of the `BaseMarshaller` constructor to see if it's null and do the lookup/cache there? LMK if so.
(That said, I could still move it to a `LAZY_TYPED_DATA` part of the `ObjectStore` if we think it might get used elsewhere as well, but since currently it's only used by the `BaseMarshaller` I figured doing the lookup/cache there is fine until it has another use to justify adding a new argument to `OBJECT_STORE_FIELD_LIST` and its uses.)
Though if we do add a `LAZY_TYPED_DATA`, then we'd need to add a load in the `BaseMarshaller` constructor because the `LazyInitXMembers` use a `SafepointWriteRwLocker` for their work, which would trigger the issue again, so I think the current version is better until there's another client of that class for some reason.
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 voted and added 2 comments![]( "Open in Gerrit")
Votes added by Alexander Markov
| Code-Review | +1 |
2 comments
typed_data_cls = lib.LookupClass(Symbols::TypedData());Tess StricklandWe should ensure that this lazy lookup does not happen during NoSafepointScope in the interpreted FFI call, so we should probably lookup this class eagerly, e.g. during ObjectStore initialization.
Tess StricklandOriginally tried adding to `ObjectStore::InitKnownObjects()`, but apparently that's too early as looking up the `TypedData` class then returns null.
I'd make a `LAZY_TYPED_DATA` part of the ObjectStore, but then that would have the same issue. Looking over `Library::LookupClass` and the methods it calls, though, I don't see anything that would cause an issue with being under a `NoSafepointScope` (and the fact that the tests now work on `vm-dyn-linux-debug-x64` without triggering the no safepoint scope check in debug mode suggests I'm correct).
If this is still a concern, though, I guess I could add a check in the body of the `BaseMarshaller` constructor to see if it's null and do the lookup/cache there? LMK if so.
Tess Strickland(That said, I could still move it to a `LAZY_TYPED_DATA` part of the `ObjectStore` if we think it might get used elsewhere as well, but since currently it's only used by the `BaseMarshaller` I figured doing the lookup/cache there is fine until it has another use to justify adding a new argument to `OBJECT_STORE_FIELD_LIST` and its uses.)
Though if we do add a `LAZY_TYPED_DATA`, then we'd need to add a load in the `BaseMarshaller` constructor because the `LazyInitXMembers` use a `SafepointWriteRwLocker` for their work, which would trigger the issue again, so I think the current version is better until there's another client of that class for some reason.
There is another test (`ffi/address_of_struct_generated_test`, https://github.com/dart-lang/sdk/issues/61913) which hits the lazy initialization problem. It seems like it hasn't started passing on this CL.
We can trigger lazy initialization at the beginning of FfiCall runtime entry by getting this class from object store to make sure the class is available inside NoSafepointScope.
RW(Class, typed_data_class) \Should this be `ARW_AR` as it can be accessed from multiple isolates concurrently?
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")
Thanks, Alex!
typed_data_cls = lib.LookupClass(Symbols::TypedData());Tess StricklandWe should ensure that this lazy lookup does not happen during NoSafepointScope in the interpreted FFI call, so we should probably lookup this class eagerly, e.g. during ObjectStore initialization.
Tess StricklandOriginally tried adding to `ObjectStore::InitKnownObjects()`, but apparently that's too early as looking up the `TypedData` class then returns null.
I'd make a `LAZY_TYPED_DATA` part of the ObjectStore, but then that would have the same issue. Looking over `Library::LookupClass` and the methods it calls, though, I don't see anything that would cause an issue with being under a `NoSafepointScope` (and the fact that the tests now work on `vm-dyn-linux-debug-x64` without triggering the no safepoint scope check in debug mode suggests I'm correct).
If this is still a concern, though, I guess I could add a check in the body of the `BaseMarshaller` constructor to see if it's null and do the lookup/cache there? LMK if so.
Tess Strickland(That said, I could still move it to a `LAZY_TYPED_DATA` part of the `ObjectStore` if we think it might get used elsewhere as well, but since currently it's only used by the `BaseMarshaller` I figured doing the lookup/cache there is fine until it has another use to justify adding a new argument to `OBJECT_STORE_FIELD_LIST` and its uses.)
Alexander MarkovThough if we do add a `LAZY_TYPED_DATA`, then we'd need to add a load in the `BaseMarshaller` constructor because the `LazyInitXMembers` use a `SafepointWriteRwLocker` for their work, which would trigger the issue again, so I think the current version is better until there's another client of that class for some reason.
There is another test (`ffi/address_of_struct_generated_test`, https://github.com/dart-lang/sdk/issues/61913) which hits the lazy initialization problem. It seems like it hasn't started passing on this CL.
We can trigger lazy initialization at the beginning of FfiCall runtime entry by getting this class from object store to make sure the class is available inside NoSafepointScope.
That test [isn't currently failing](https://dart-current-results.web.app/?filter=ffi/address_of_struct_generated&showAll=true) (probably is flaky in the same way `ffi/address_of_array_generated_test` was in the past as well, not sure why since either it is a leaf call, in which case it should always trigger unless a non-leaf call ran before it to prime the cache, or it's not, in which case it shouldn't because there's no `NoSafepointScope`).
I've added it to the TEST= line accordingly though.
Should this be `ARW_AR` as it can be accessed from multiple isolates concurrently?
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 voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
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. |
dart-scoped@luci-project-accounts.iam.gserviceaccount.com (Gerrit)
dart-...@luci-project-accounts.iam.gserviceaccount.com submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
12 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: runtime/vm/object_store.h
Insertions: 1, Deletions: 1.
@@ -213,7 +213,7 @@
RW(Array, saved_unlinked_calls) \
RW(GrowableObjectArray, megamorphic_cache_table) \
RW(GrowableObjectArray, ffi_callback_code) \
- RW(Class, typed_data_class) \
+ ARW_AR(Class, typed_data_class) \
RW(Array, ffi_callback_functions) \
/* Roots for JIT/AOT snapshots are up until here (see to_snapshot() below)*/ \
RW(Array, dispatch_table_code_entries) \
```
Change information
[vm,dyn_modules] Remove CompilerState use from CallMarshaller.
The CallMarshaller is used not only from the compiler, but also
from the FfiCall runtime entry used by the interpreter. Since it
only has one use of the thread's CompilerState, looking up the
TypedData class, move the storage of that class from the compiler
state to the object store and remove this dependency.
TEST=ffi/address_of_array_generated_test
ffi/address_of_cast_test
fii/address_of_struct_generated_test
ffi/address_of_typeddata_generated_test
- M runtime/vm/compiler/compiler_state.cc
- M runtime/vm/compiler/compiler_state.h
- M runtime/vm/compiler/ffi/marshaller.cc
- M runtime/vm/object_store.h
- M runtime/vm/runtime_entry.cc
Code-Review: +1 by Alexander Markov
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |