[defer-import-eval] implement deferred import evaluation for ES modules [v8/v8 : main]
Caio Lima (Gerrit)
Caio Lima voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Leszek Swirski
- Olivier Flückiger
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Caio Lima (Gerrit)
Caio Lima added 1 comment![]( "Open in Gerrit")
&is_deferred_namespace);I’m wondering how the impact of this check would impact non-deferred module accesses. I still need to run Speedometer and JetStream here, but would it be possible to trigger a pinpoint to check it here?
Related details
- Leszek Swirski
- Olivier Flückiger
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/15354465d10000
Related details
- Caio Lima
- Leszek Swirski
- Olivier Flückiger
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/111e6402d10000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/13fdb721d10000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/jetstream-main.crossbench complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/14c22269d10000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/16080bc2d10000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/14e08379d10000
Olivier Flückiger (Gerrit)
Olivier Flückiger added 1 comment![]( "Open in Gerrit")
&is_deferred_namespace);I’m wondering how the impact of this check would impact non-deferred module accesses. I still need to run Speedometer and JetStream here, but would it be possible to trigger a pinpoint to check it here?
Can we just not configure the IC for kModuleExport as long as the module is not evaluated? Basically here we wouly only CSA_DCHECK that the module is evaluated. And in the IC update machinery we should not configure the IC with modules which are not evaluated yet, but instead leave it empty. Maybe I miss some detail that makes this impossible?
Related details
- Caio Lima
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Caio Lima (Gerrit)
Caio Lima added 1 comment![]( "Open in Gerrit")
&is_deferred_namespace);Olivier FlückigerI’m wondering how the impact of this check would impact non-deferred module accesses. I still need to run Speedometer and JetStream here, but would it be possible to trigger a pinpoint to check it here?
Can we just not configure the IC for kModuleExport as long as the module is not evaluated? Basically here we wouly only CSA_DCHECK that the module is evaluated. And in the IC update machinery we should not configure the IC with modules which are not evaluated yet, but instead leave it empty. Maybe I miss some detail that makes this impossible?
No, you are right here and I'm the one who actually missed something. I was with the case in mind where ModuleNamespace objects share their maps, thinking in the scenario where 2 different deferred modules having the same exported names. However, I just figured out that it's never the case[1]. Given that, you are right and I can actually do the check when installing the IC handler. I'm going to change it and thank you for the question.
[1] - https://github.com/v8/v8/blob/main/src/objects/module.cc#L383
Related details
- Leszek Swirski
- Olivier Flückiger
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Flückiger (Gerrit)
Olivier Flückiger added 1 comment![]( "Open in Gerrit")
handler = MaybeObjectHandle(LoadHandler::LoadSlow(isolate()));Is this the case where the value itself then triggers the evaluation?
It would be nice if the IC is not permanently slow, but rather we wait with updating it until the module is evaluated.
Related details
- Caio Lima
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Caio Lima (Gerrit)
Caio Lima added 1 comment![]( "Open in Gerrit")
handler = MaybeObjectHandle(LoadHandler::LoadSlow(isolate()));Is this the case where the value itself then triggers the evaluation?
It would be nice if the IC is not permanently slow, but rather we wait with updating it until the module is evaluated.
Is this the case where the value itself then triggers the evaluation?
I'm not sure if I understand the question, but the case here is that a missing access to a deferred namespace should trigger the evaluation to then return undefined. The logic behind this part of the spec is that it triggers the evaluation because it needs to load all exported names.
Regarding slow path installation, the hypothesis I'm thinking here is that when ICs start to get filled, the module is quite likely to be evaluated already, due to `--lazy-feedback-allocation`. IIUC, on default configuration the vector is allocated when tiering up to Sparkplug and it's likely that an access to the module already happened at this point. So when `status != kEvaluated` here, probably is due to an error on evaluation, and since we need to throw on each access, I thought it would be a good time to give up on IC. One alternative is to actually just install slow path if `status == kError`. WDYT?
Related details
- Leszek Swirski
- Olivier Flückiger
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Flückiger (Gerrit)
Olivier Flückiger added 1 comment![]( "Open in Gerrit")
handler = MaybeObjectHandle(LoadHandler::LoadSlow(isolate()));Caio LimaIs this the case where the value itself then triggers the evaluation?
It would be nice if the IC is not permanently slow, but rather we wait with updating it until the module is evaluated.
Is this the case where the value itself then triggers the evaluation?
I'm not sure if I understand the question, but the case here is that a missing access to a deferred namespace should trigger the evaluation to then return undefined. The logic behind this part of the spec is that it triggers the evaluation because it needs to load all exported names.Regarding slow path installation, the hypothesis I'm thinking here is that when ICs start to get filled, the module is quite likely to be evaluated already, due to `--lazy-feedback-allocation`. IIUC, on default configuration the vector is allocated when tiering up to Sparkplug and it's likely that an access to the module already happened at this point. So when `status != kEvaluated` here, probably is due to an error on evaluation, and since we need to throw on each access, I thought it would be a good time to give up on IC. One alternative is to actually just install slow path if `status == kError`. WDYT?
I think only updating on error would be optimal. Without an error we should be guaranteed to have an evaluated module on the next access, or not? so updating again should definitely pay off.
Related details
- Caio Lima
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Flückiger (Gerrit)
Olivier Flückiger added 1 comment![]( "Open in Gerrit")
handler = MaybeObjectHandle(LoadHandler::LoadSlow(isolate()));Caio LimaIs this the case where the value itself then triggers the evaluation?
It would be nice if the IC is not permanently slow, but rather we wait with updating it until the module is evaluated.
Olivier FlückigerIs this the case where the value itself then triggers the evaluation?
I'm not sure if I understand the question, but the case here is that a missing access to a deferred namespace should trigger the evaluation to then return undefined. The logic behind this part of the spec is that it triggers the evaluation because it needs to load all exported names.Regarding slow path installation, the hypothesis I'm thinking here is that when ICs start to get filled, the module is quite likely to be evaluated already, due to `--lazy-feedback-allocation`. IIUC, on default configuration the vector is allocated when tiering up to Sparkplug and it's likely that an access to the module already happened at this point. So when `status != kEvaluated` here, probably is due to an error on evaluation, and since we need to throw on each access, I thought it would be a good time to give up on IC. One alternative is to actually just install slow path if `status == kError`. WDYT?
I think only updating on error would be optimal. Without an error we should be guaranteed to have an evaluated module on the next access, or not? so updating again should definitely pay off.
Ah btw. your understanding of lazy fbv allocation is correct. But it could still happen that the access is e.g., in a branch that was not executed so far.
Olivier Flückiger (Gerrit)
Olivier Flückiger added 1 comment![]( "Open in Gerrit")
I looked a bit closer at the CL now and realized that we basically have to put `JSDeferredModuleNamespace::MaybeEvaluateDeferredModule(it)` for any kind of access. Granted this is in the runtime, but still these checks add up since they are basically on every access. My suspicion is that the red trend on your speedometer runs are actually from this.
So what I have been thinking, maybe we should make unevaluated modules interceptors after all. This will have the upside that lazy modules do not make normal data property accessors or `has` slower. and it might even simplify the implementation since we should never see unevaluated modules in ICs.
Igor Sheludko (Gerrit)
Igor Sheludko added 4 comments![]( "Open in Gerrit")
some comments
handler = MaybeObjectHandle(LoadHandler::LoadSlow(isolate()));Caio LimaIs this the case where the value itself then triggers the evaluation?
It would be nice if the IC is not permanently slow, but rather we wait with updating it until the module is evaluated.
Olivier FlückigerIs this the case where the value itself then triggers the evaluation?
I'm not sure if I understand the question, but the case here is that a missing access to a deferred namespace should trigger the evaluation to then return undefined. The logic behind this part of the spec is that it triggers the evaluation because it needs to load all exported names.Regarding slow path installation, the hypothesis I'm thinking here is that when ICs start to get filled, the module is quite likely to be evaluated already, due to `--lazy-feedback-allocation`. IIUC, on default configuration the vector is allocated when tiering up to Sparkplug and it's likely that an access to the module already happened at this point. So when `status != kEvaluated` here, probably is due to an error on evaluation, and since we need to throw on each access, I thought it would be a good time to give up on IC. One alternative is to actually just install slow path if `status == kError`. WDYT?
Olivier FlückigerI think only updating on error would be optimal. Without an error we should be guaranteed to have an evaluated module on the next access, or not? so updating again should definitely pay off.
Ah btw. your understanding of lazy fbv allocation is correct. But it could still happen that the access is e.g., in a branch that was not executed so far.
+1 to this question. IIUC we are loading a property from the module and by this time I think `LookupIterator` itself or `LookupForRead` should have already triggered module evaluation and thus we should already know whether the property is there or not (on this branch - it's not found). I think there should be no special handling of modules in `if (!lookup->IsFound()) {` branch.
return MaybeObjectHandle(LoadHandler::LoadSlow(isolate()));Same concern as above - I think by this time the module should already be evaluated (by `LookupIterator` or by `LookupForRead`).
}It feels like this should be done inside lookup iterator under the hood because AFAICT handling of deferred modules is always the same.
If I'm wrong then please consider adding a `DEFERRED_MODULE` state (similar to `ACCESS_CHECK`) which would imply triggering required actions depending on the operation type.
Related details
- Caio Lima
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Caio Lima (Gerrit)
Caio Lima added 3 comments![]( "Open in Gerrit")
handler = MaybeObjectHandle(LoadHandler::LoadSlow(isolate()));Caio LimaIs this the case where the value itself then triggers the evaluation?
It would be nice if the IC is not permanently slow, but rather we wait with updating it until the module is evaluated.
Olivier FlückigerIs this the case where the value itself then triggers the evaluation?
I'm not sure if I understand the question, but the case here is that a missing access to a deferred namespace should trigger the evaluation to then return undefined. The logic behind this part of the spec is that it triggers the evaluation because it needs to load all exported names.Regarding slow path installation, the hypothesis I'm thinking here is that when ICs start to get filled, the module is quite likely to be evaluated already, due to `--lazy-feedback-allocation`. IIUC, on default configuration the vector is allocated when tiering up to Sparkplug and it's likely that an access to the module already happened at this point. So when `status != kEvaluated` here, probably is due to an error on evaluation, and since we need to throw on each access, I thought it would be a good time to give up on IC. One alternative is to actually just install slow path if `status == kError`. WDYT?
Olivier FlückigerI think only updating on error would be optimal. Without an error we should be guaranteed to have an evaluated module on the next access, or not? so updating again should definitely pay off.
Igor SheludkoAh btw. your understanding of lazy fbv allocation is correct. But it could still happen that the access is e.g., in a branch that was not executed so far.
+1 to this question. IIUC we are loading a property from the module and by this time I think `LookupIterator` itself or `LookupForRead` should have already triggered module evaluation and thus we should already know whether the property is there or not (on this branch - it's not found). I think there should be no special handling of modules in `if (!lookup->IsFound()) {` branch.
I think it's the case, since all async dependencies are already evaluated once such access happens.
The case being considered when `!lookup->IsFound()` is when there's an error while accessing a deferred module. For such case we need to throw the same error on every access. The semantics is the same if the property is present or not. Considering how this implementation is now, if we cache on
!lookup->IsFound() and the evaluation throws an error, next accesses will not throw. This happens because `UpdateCaches` is called before we actually trigger the deferred evaluation.
return MaybeObjectHandle(LoadHandler::LoadSlow(isolate()));Same concern as above - I think by this time the module should already be evaluated (by `LookupIterator` or by `LookupForRead`).
As I mentioned above, module evaluation happens on `Object::GetProperty`, but UpdateCaches is called before it[1].
[1] - https://github.com/v8/v8/blob/main/src/ic/ic.cc#L438-L456
It feels like this should be done inside lookup iterator under the hood because AFAICT handling of deferred modules is always the same.
If I'm wrong then please consider adding a `DEFERRED_MODULE` state (similar to `ACCESS_CHECK`) which would imply triggering required actions depending on the operation type.
I actually started looking into a version using Interceptors and I should upload it later this week. This way this part of implementation is going away, and I also won't need to do the handling in ICs as before.
Related details
- Igor Sheludko
- Leszek Swirski
- Olivier Flückiger
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Igor Sheludko (Gerrit)
Igor Sheludko added 3 comments![]( "Open in Gerrit")
handler = MaybeObjectHandle(LoadHandler::LoadSlow(isolate()));Caio LimaIs this the case where the value itself then triggers the evaluation?
It would be nice if the IC is not permanently slow, but rather we wait with updating it until the module is evaluated.
Olivier FlückigerIs this the case where the value itself then triggers the evaluation?
I'm not sure if I understand the question, but the case here is that a missing access to a deferred namespace should trigger the evaluation to then return undefined. The logic behind this part of the spec is that it triggers the evaluation because it needs to load all exported names.Regarding slow path installation, the hypothesis I'm thinking here is that when ICs start to get filled, the module is quite likely to be evaluated already, due to `--lazy-feedback-allocation`. IIUC, on default configuration the vector is allocated when tiering up to Sparkplug and it's likely that an access to the module already happened at this point. So when `status != kEvaluated` here, probably is due to an error on evaluation, and since we need to throw on each access, I thought it would be a good time to give up on IC. One alternative is to actually just install slow path if `status == kError`. WDYT?
Olivier FlückigerI think only updating on error would be optimal. Without an error we should be guaranteed to have an evaluated module on the next access, or not? so updating again should definitely pay off.
Igor SheludkoAh btw. your understanding of lazy fbv allocation is correct. But it could still happen that the access is e.g., in a branch that was not executed so far.
Caio Lima+1 to this question. IIUC we are loading a property from the module and by this time I think `LookupIterator` itself or `LookupForRead` should have already triggered module evaluation and thus we should already know whether the property is there or not (on this branch - it's not found). I think there should be no special handling of modules in `if (!lookup->IsFound()) {` branch.
I think it's the case, since all async dependencies are already evaluated once such access happens.
The case being considered when `!lookup->IsFound()` is when there's an error while accessing a deferred module. For such case we need to throw the same error on every access. The semantics is the same if the property is present or not. Considering how this implementation is now, if we cache on
!lookup->IsFound() and the evaluation throws an error, next accesses will not throw. This happens because `UpdateCaches` is called before we actually trigger the deferred evaluation.
I see. Then having a separate `LookupIterator` state `DEFERRED_MODULE` or `FAILED_DEFERRED_MODULE` would probably be helpful here. The lookup iterator could transition there in case the holder is the deferred module that failed during evaluation.
return MaybeObjectHandle(LoadHandler::LoadSlow(isolate()));Caio LimaSame concern as above - I think by this time the module should already be evaluated (by `LookupIterator` or by `LookupForRead`).
As I mentioned above, module evaluation happens on `Object::GetProperty`, but UpdateCaches is called before it[1].
[1] - https://github.com/v8/v8/blob/main/src/ic/ic.cc#L438-L456
I was actually suggesting to make sure that the `LookupIterator` or at least `LookupForRead` function trigger module evaluation by the time we reach here.
Caio LimaIt feels like this should be done inside lookup iterator under the hood because AFAICT handling of deferred modules is always the same.
If I'm wrong then please consider adding a `DEFERRED_MODULE` state (similar to `ACCESS_CHECK`) which would imply triggering required actions depending on the operation type.
I actually started looking into a version using Interceptors and I should upload it later this week. This way this part of implementation is going away, and I also won't need to do the handling in ICs as before.
Giving Interceptors a try SGTM, I just think that they will be noticeably slower than what you could get after refining this implementation. Unless you use interceptor mode until the first evaluation and then switch the module object shape to an interceptor-less state somehow so that at least in case of successful evaluation there will be a chance to make all these properties fast.
Related details
- Caio Lima
- Leszek Swirski
- Olivier Flückiger
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Flückiger (Gerrit)
Olivier Flückiger added 1 comment![]( "Open in Gerrit")
Caio LimaIt feels like this should be done inside lookup iterator under the hood because AFAICT handling of deferred modules is always the same.
If I'm wrong then please consider adding a `DEFERRED_MODULE` state (similar to `ACCESS_CHECK`) which would imply triggering required actions depending on the operation type.
Igor SheludkoI actually started looking into a version using Interceptors and I should upload it later this week. This way this part of implementation is going away, and I also won't need to do the handling in ICs as before.
Giving Interceptors a try SGTM, I just think that they will be noticeably slower than what you could get after refining this implementation. Unless you use interceptor mode until the first evaluation and then switch the module object shape to an interceptor-less state somehow so that at least in case of successful evaluation there will be a chance to make all these properties fast.
That was the idea. In most cases (unless the evaluation throws) we should be able to replace the interceptor with a module map once evaluated.
Related details
- Caio Lima
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Caio Lima (Gerrit)
Caio Lima added 1 comment![]( "Open in Gerrit")
Caio LimaIt feels like this should be done inside lookup iterator under the hood because AFAICT handling of deferred modules is always the same.
If I'm wrong then please consider adding a `DEFERRED_MODULE` state (similar to `ACCESS_CHECK`) which would imply triggering required actions depending on the operation type.
Igor SheludkoI actually started looking into a version using Interceptors and I should upload it later this week. This way this part of implementation is going away, and I also won't need to do the handling in ICs as before.
Olivier FlückigerGiving Interceptors a try SGTM, I just think that they will be noticeably slower than what you could get after refining this implementation. Unless you use interceptor mode until the first evaluation and then switch the module object shape to an interceptor-less state somehow so that at least in case of successful evaluation there will be a chance to make all these properties fast.
That was the idea. In most cases (unless the evaluation throws) we should be able to replace the interceptor with a module map once evaluated.
Yes. One question that I have here is if we just transition do common ModuleNamespace map, will we lose track of potential private fields installed on previous map?
I have a very odd edge case in tests where we install private fields in a NamespaceObject and it only triggers evaluation afterwards. Right now what I'm doing is actually copying the current map from deferred module object and then disabling interceptors and properly setting it.
Related details
- Igor Sheludko
- Leszek Swirski
- Olivier Flückiger
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Igor Sheludko (Gerrit)
Igor Sheludko added 1 comment![]( "Open in Gerrit")
Caio LimaIt feels like this should be done inside lookup iterator under the hood because AFAICT handling of deferred modules is always the same.
If I'm wrong then please consider adding a `DEFERRED_MODULE` state (similar to `ACCESS_CHECK`) which would imply triggering required actions depending on the operation type.
Igor SheludkoI actually started looking into a version using Interceptors and I should upload it later this week. This way this part of implementation is going away, and I also won't need to do the handling in ICs as before.
Olivier FlückigerGiving Interceptors a try SGTM, I just think that they will be noticeably slower than what you could get after refining this implementation. Unless you use interceptor mode until the first evaluation and then switch the module object shape to an interceptor-less state somehow so that at least in case of successful evaluation there will be a chance to make all these properties fast.
Caio LimaThat was the idea. In most cases (unless the evaluation throws) we should be able to replace the interceptor with a module map once evaluated.
Yes. One question that I have here is if we just transition do common ModuleNamespace map, will we lose track of potential private fields installed on previous map?
I have a very odd edge case in tests where we install private fields in a NamespaceObject and it only triggers evaluation afterwards. Right now what I'm doing is actually copying the current map from deferred module object and then disabling interceptors and properly setting it.
- Caio Lima
- Leszek Swirski
- Olivier Flückiger
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Caio Lima (Gerrit)
Caio Lima added 7 comments![]( "Open in Gerrit")
I uploaded the version I've worked so far to check if I'm going in the right direction when using Interceptors. In this version, we don't need to change anything regarding IC and the logic is to transition the Map to an interpector-less version as soon as the evaluation happens.
DirectHandle<Map> Factory::EnsureJSDeferredModuleNamespaceMap() {I have some questions regarding this part here. I'm using mostly the FunctionTemplate to configure interceptors, but I'm not sure if this is the proper path.
In addition to that, I want to have just a single Map for `JSDeferredModuleNamespace`, but I didnt' manage to do it on `bootstrapper.cc` due to issues I'm having by adding the following callbacks to external reference list. Do you have some idea if I could fix this somehow?
if (!IsNullMap(*map)) {Here I'm using a bit of a hack using NullMap to mark if we already initialized `js_deferred_module_namespace_map`. I'm not a big fan of this approach, and I'd be happy to hear suggestion on how I could properly set the map here.
Olivier FlückigerI’m wondering how the impact of this check would impact non-deferred module accesses. I still need to run Speedometer and JetStream here, but would it be possible to trigger a pinpoint to check it here?
Caio LimaCan we just not configure the IC for kModuleExport as long as the module is not evaluated? Basically here we wouly only CSA_DCHECK that the module is evaluated. And in the IC update machinery we should not configure the IC with modules which are not evaluated yet, but instead leave it empty. Maybe I miss some detail that makes this impossible?
No, you are right here and I'm the one who actually missed something. I was with the case in mind where ModuleNamespace objects share their maps, thinking in the scenario where 2 different deferred modules having the same exported names. However, I just figured out that it's never the case[1]. Given that, you are right and I can actually do the check when installing the IC handler. I'm going to change it and thank you for the question.
[1] - https://github.com/v8/v8/blob/main/src/objects/module.cc#L383
Done
ReadOnlyRoots(heap()).null_map());This is the part of the hack I mentioned above. If I could solve the issue with external reference and interceptors callbacks, I 'll then be able to properly configure `set_js_deferred_module_namespace_map` at this point.
Map::Copy(isolate, current_map, "RemoveNamedInterceptors");At this point I'm not just moving to JSModuleNamespace map we need to take into account private fields that might be installed in this `ns` up to this point (very hard to happen in real applications).
DirectHandle<JSReceiver> holder = it->GetHolder<JSReceiver>();I needed to add this case here, otherwise the interceptor will trigger evaluation due to `GetPropertyDescriptorWithInterceptor` that happens later.
Related details
- Leszek Swirski
- Olivier Flückiger
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Caio Lima (Gerrit)
Caio Lima added 3 comments![]( "Open in Gerrit")
DirectHandle<Map> Factory::EnsureJSDeferredModuleNamespaceMap() {I have some questions regarding this part here. I'm using mostly the FunctionTemplate to configure interceptors, but I'm not sure if this is the proper path.
In addition to that, I want to have just a single Map for `JSDeferredModuleNamespace`, but I didnt' manage to do it on `bootstrapper.cc` due to issues I'm having by adding the following callbacks to external reference list. Do you have some idea if I could fix this somehow?
Done
Here I'm using a bit of a hack using NullMap to mark if we already initialized `js_deferred_module_namespace_map`. I'm not a big fan of this approach, and I'd be happy to hear suggestion on how I could properly set the map here.
Done
This is the part of the hack I mentioned above. If I could solve the issue with external reference and interceptors callbacks, I 'll then be able to properly configure `set_js_deferred_module_namespace_map` at this point.
Done
Related details
- Leszek Swirski
- Olivier Flückiger
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Flückiger (Gerrit)
Olivier Flückiger added 1 comment![]( "Open in Gerrit")
@Caio sorry I lost track here. Are you waiting on another round of reviews or are you still making changes?
Related details
- Caio Lima
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Caio Lima (Gerrit)
Caio Lima added 1 comment![]( "Open in Gerrit")
@Caio sorry I lost track here. Are you waiting on another round of reviews or are you still making changes?
No worries! I'm waiting for a new round of reviews. The major change is that it's using Interceptors now.
Related details
- Leszek Swirski
- Olivier Flückiger
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Flückiger (Gerrit)
Olivier Flückiger added 1 comment![]( "Open in Gerrit")
Caio Lima@Caio sorry I lost track here. Are you waiting on another round of reviews or are you still making changes?
No worries! I'm waiting for a new round of reviews. The major change is that it's using Interceptors now.
- Caio Lima
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Caio Lima (Gerrit)
Caio Lima added 1 comment![]( "Open in Gerrit")
Map::Copy(isolate, current_map, "RemoveNamedInterceptors");At this point I'm not just moving to JSModuleNamespace map we need to take into account private fields that might be installed in this `ns` up to this point (very hard to happen in real applications).
Actually, Nicolò just showed me https://github.com/tc39/proposal-nonextensible-applies-to-private. What's V8 status for this proposal? Looking `JSReceiver::AddPrivateField`, I don't think it is implemented yet.
But in this case, I think it's fair to implement import-defer with the idea that there's no transition from private fields/brands already in mind. WDYT?
Related details
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Flückiger (Gerrit)
Olivier Flückiger added 1 comment![]( "Open in Gerrit")
Map::Copy(isolate, current_map, "RemoveNamedInterceptors");Caio LimaAt this point I'm not just moving to JSModuleNamespace map we need to take into account private fields that might be installed in this `ns` up to this point (very hard to happen in real applications).
Actually, Nicolò just showed me https://github.com/tc39/proposal-nonextensible-applies-to-private. What's V8 status for this proposal? Looking `JSReceiver::AddPrivateField`, I don't think it is implemented yet.
But in this case, I think it's fair to implement import-defer with the idea that there's no transition from private fields/brands already in mind. WDYT?
We implemented the current version (see https://chromium-review.googlesource.com/c/v8/v8/+/7066839). However we recently discovered https://github.com/tc39/proposal-nonextensible-applies-to-private/issues/6 which means the proposal probably needs to be changed to call IsExtensible instead.
If it's hard to implement I would be fine with leaving a todo (and maybe a dcheck) for now and hope the other proposal advances fast enough.
Related details
- Caio Lima
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Flückiger (Gerrit)
Olivier Flückiger added 7 comments![]( "Open in Gerrit")
thanks, this feels like the right direction to be moving in. seems we can almost entirely rely on the interceptor infra to introduce this without adding any new checks on hot-paths. I like that.
NewJSObjectType::kMaybeEmbedderFieldsAndApiWrapper)));I guess interceptors require this NewJSObjectType? Can we make them work without lying on that type?
IsJSDeferredModuleNamespace(*holder))) {this looks a bit like a leaky abstraction to me. We should really introduce some concept of a "internal" interceptor which does not come from the api. Either by marking such interceptors or by having e.g., a centralized list of instance types which are such internal interceptors.
if (IsJSDeferredModuleNamespace(*holder)) {dito
if (IsJSDeferredModuleNamespace(*object)) {maybe move this down a bit since it's probably not the most common case.
DirectHandle<JSReceiver> holder = it->GetHolder<JSReceiver>();What is the reason for the special casing here instead of using a setter on the interceptor?
'language/import/import-defer/deferred-namespace-object/exotic-object-behavior': [SKIP],just remove these as the default is PASS
Caio Lima (Gerrit)
Caio Lima added 3 comments![]( "Open in Gerrit")
NewJSObjectType::kMaybeEmbedderFieldsAndApiWrapper)));I guess interceptors require this NewJSObjectType? Can we make them work without lying on that type?
Actually, I'm getting this from `Factory::NewJSModuleNamespace` above. I spent some time looking why JSModuleNamespace, use this enum, and looks like it's related with the fact of it being an `JSSpecialObject` (https://source.chromium.org/chromium/chromium/src/+/main:v8/src/objects/js-objects.tq;l=65;drc=776f8c61110d34732318252a9661692fbc285dac).
I don't see any limitation on Interceptors for `NewJSObjectType`, at least not directly. The check for API that I see on Interceptors is from this part https://source.chromium.org/chromium/chromium/src/+/main:v8/src/objects/map-inl.h;l=1049;drc=5e7626b1a30658766d34fd9875fd2bfbdc2ebb3f. However the way I'm initializing things in `bootstrap.cc` satisfy this assertion. It feels a bit strange to me using FunctionTemplateInfo there, however I'm not sure if the alternative I have in mind is better. What I implemented before, but I didn't upload, was a new `JSObjectWithInterceptors` that would store InterceptorInfos, and then in https://source.chromium.org/chromium/chromium/src/+/main:v8/src/objects/map-inl.h;l=156;drc=5e7626b1a30658766d34fd9875fd2bfbdc2ebb3f I 'd get the check if `IsJSObjectWithInterceptors` and get the InterceptorInfo properly. I'd use it as a constructor to have only a single `JSObjectWithInterceptors` shared with all instances of JSDeferredModuleNamespace. However, I decided to rollback from it because it was doing pretty much what `FunctionTemplateInfo` does.
IsJSDeferredModuleNamespace(*holder))) {this looks a bit like a leaky abstraction to me. We should really introduce some concept of a "internal" interceptor which does not come from the api. Either by marking such interceptors or by having e.g., a centralized list of instance types which are such internal interceptors.
Just to make sure if I understand your comment here. If I have something like `JSObjectWithInterceptors` (name can change), and check here for `IsJSObjectWithInterceptors`, would it be fine? I added this check here because even when a property is absent in a Deferred Module, it still triggers evaluation.
DirectHandle<JSReceiver> holder = it->GetHolder<JSReceiver>();What is the reason for the special casing here instead of using a setter on the interceptor?
I don't remember why I decided to not use a setter interceptor here. Maybe I just overlooked it and looks like it will work..
Related details
- Leszek Swirski
- Olivier Flückiger
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Flückiger (Gerrit)
Olivier Flückiger added 2 comments![]( "Open in Gerrit")
NewJSObjectType::kMaybeEmbedderFieldsAndApiWrapper)));Caio LimaI guess interceptors require this NewJSObjectType? Can we make them work without lying on that type?
Actually, I'm getting this from `Factory::NewJSModuleNamespace` above. I spent some time looking why JSModuleNamespace, use this enum, and looks like it's related with the fact of it being an `JSSpecialObject` (https://source.chromium.org/chromium/chromium/src/+/main:v8/src/objects/js-objects.tq;l=65;drc=776f8c61110d34732318252a9661692fbc285dac).
I don't see any limitation on Interceptors for `NewJSObjectType`, at least not directly. The check for API that I see on Interceptors is from this part https://source.chromium.org/chromium/chromium/src/+/main:v8/src/objects/map-inl.h;l=1049;drc=5e7626b1a30658766d34fd9875fd2bfbdc2ebb3f. However the way I'm initializing things in `bootstrap.cc` satisfy this assertion. It feels a bit strange to me using FunctionTemplateInfo there, however I'm not sure if the alternative I have in mind is better. What I implemented before, but I didn't upload, was a new `JSObjectWithInterceptors` that would store InterceptorInfos, and then in https://source.chromium.org/chromium/chromium/src/+/main:v8/src/objects/map-inl.h;l=156;drc=5e7626b1a30658766d34fd9875fd2bfbdc2ebb3f I 'd get the check if `IsJSObjectWithInterceptors` and get the InterceptorInfo properly. I'd use it as a constructor to have only a single `JSObjectWithInterceptors` shared with all instances of JSDeferredModuleNamespace. However, I decided to rollback from it because it was doing pretty much what `FunctionTemplateInfo` does.
Oh, I see. Didn't see that normal modules also set it up like this. Ignore this comment then.
IsJSDeferredModuleNamespace(*holder))) {Caio Limathis looks a bit like a leaky abstraction to me. We should really introduce some concept of a "internal" interceptor which does not come from the api. Either by marking such interceptors or by having e.g., a centralized list of instance types which are such internal interceptors.
Just to make sure if I understand your comment here. If I have something like `JSObjectWithInterceptors` (name can change), and check here for `IsJSObjectWithInterceptors`, would it be fine? I added this check here because even when a property is absent in a Deferred Module, it still triggers evaluation.
Yes, it just feels strange to check for one particular object type here and not for the "concept" (an internal interceptor which can have absent fields).
Related details
- Caio Lima
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Caio Lima (Gerrit)
Caio Lima added 1 comment![]( "Open in Gerrit")
if (IsJSDeferredModuleNamespace(*object)) {maybe move this down a bit since it's probably not the most common case.
What do you think of using V8_UNLIKELY? If I move this bellow to make a meaningful difference, I'll need to duplicate this code in 4 parts (1 for each `if (V8_ENABLE_SWISS_NAME_DICTIONARY_BOOL)` path. It's necessary because I'm just installing properties when `MaybeEvaluateAndTransition` is called, so I need to call it before `GetOwnEnumPropertyDictionaryKeys` or `CollectKeysFromDictionary`. This lazy installation is useful for Interceptors part, because I don't need to worry about `ACCESSOR` while the module was not evaluated yet.
Related details
- Leszek Swirski
- Olivier Flückiger
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Flückiger (Gerrit)
Olivier Flückiger added 1 comment![]( "Open in Gerrit")
if (IsJSDeferredModuleNamespace(*object)) {Caio Limamaybe move this down a bit since it's probably not the most common case.
What do you think of using V8_UNLIKELY? If I move this bellow to make a meaningful difference, I'll need to duplicate this code in 4 parts (1 for each `if (V8_ENABLE_SWISS_NAME_DICTIONARY_BOOL)` path. It's necessary because I'm just installing properties when `MaybeEvaluateAndTransition` is called, so I need to call it before `GetOwnEnumPropertyDictionaryKeys` or `CollectKeysFromDictionary`. This lazy installation is useful for Interceptors part, because I don't need to worry about `ACCESSOR` while the module was not evaluated yet.
sounds good. I think we are actually transitioning to [[unlikely]] predicates for new code.
Related details
- Caio Lima
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Caio Lima (Gerrit)
Caio Lima added 10 comments![]( "Open in Gerrit")
Sorry for taking a bit long to update here. I spent some time trying to figure out the proper way to implement [[Set]] with interceptors.
is_internal: bool: 1 bit;I added this new bitfield so we are able to figure out if an interceptor is internal or not. Such distinction came in hand mainly on `SetPropertyInternal`, but also on other interceptor paths. The overall idea for the creating such distinction is to avoid introducing any observable change for non-internal interceptors.
Caio LimaIt feels like this should be done inside lookup iterator under the hood because AFAICT handling of deferred modules is always the same.
If I'm wrong then please consider adding a `DEFERRED_MODULE` state (similar to `ACCESS_CHECK`) which would imply triggering required actions depending on the operation type.
Igor SheludkoI actually started looking into a version using Interceptors and I should upload it later this week. This way this part of implementation is going away, and I also won't need to do the handling in ICs as before.
Olivier FlückigerGiving Interceptors a try SGTM, I just think that they will be noticeably slower than what you could get after refining this implementation. Unless you use interceptor mode until the first evaluation and then switch the module object shape to an interceptor-less state somehow so that at least in case of successful evaluation there will be a chance to make all these properties fast.
Caio LimaThat was the idea. In most cases (unless the evaluation throws) we should be able to replace the interceptor with a module map once evaluated.
Igor SheludkoYes. One question that I have here is if we just transition do common ModuleNamespace map, will we lose track of potential private fields installed on previous map?
I have a very odd edge case in tests where we install private fields in a NamespaceObject and it only triggers evaluation afterwards. Right now what I'm doing is actually copying the current map from deferred module object and then disabling interceptors and properly setting it.
I think your approach should work.
Done
Caio Limathis looks a bit like a leaky abstraction to me. We should really introduce some concept of a "internal" interceptor which does not come from the api. Either by marking such interceptors or by having e.g., a centralized list of instance types which are such internal interceptors.
Olivier FlückigerJust to make sure if I understand your comment here. If I have something like `JSObjectWithInterceptors` (name can change), and check here for `IsJSObjectWithInterceptors`, would it be fine? I added this check here because even when a property is absent in a Deferred Module, it still triggers evaluation.
Yes, it just feels strange to check for one particular object type here and not for the "concept" (an internal interceptor which can have absent fields).
Acknowledged
if (IsJSDeferredModuleNamespace(*holder)) {Caio Limadito
Acknowledged
Caio Limamaybe move this down a bit since it's probably not the most common case.
Olivier FlückigerWhat do you think of using V8_UNLIKELY? If I move this bellow to make a meaningful difference, I'll need to duplicate this code in 4 parts (1 for each `if (V8_ENABLE_SWISS_NAME_DICTIONARY_BOOL)` path. It's necessary because I'm just installing properties when `MaybeEvaluateAndTransition` is called, so I need to call it before `GetOwnEnumPropertyDictionaryKeys` or `CollectKeysFromDictionary`. This lazy installation is useful for Interceptors part, because I don't need to worry about `ACCESSOR` while the module was not evaluated yet.
sounds good. I think we are actually transitioning to [[unlikely]] predicates for new code.
Acknowledged
DirectHandle<JSReceiver> holder = it->GetHolder<JSReceiver>();Caio LimaI needed to add this case here, otherwise the interceptor will trigger evaluation due to `GetPropertyDescriptorWithInterceptor` that happens later.
Done
DirectHandle<JSReceiver> holder = it->GetHolder<JSReceiver>();Caio LimaWhat is the reason for the special casing here instead of using a setter on the interceptor?
I don't remember why I decided to not use a setter interceptor here. Maybe I just overlooked it and looks like it will work..
Done
it->GetInterceptor()->is_internal()) {This is one of the reasons why it took me a while to upload a new patch. I didn't fully understand why there's a `it->HolderIsReceiverOrHiddenPrototype()` check before calling `JSObject::SetPropertyWithInterceptor`, but this gave me some trouble to properly implement `JSDeferredModuleNamespace` [[Set]] without triggering an evaluation. I added a condition on internal interceptors to avoid introduce breaking changes to current interceptors API, since there'll be a trap on `JSObject::GetPropertyAttributesWithInterceptor` if `it->HolderIsReceiverOrHiddenPrototype() == false`. However, I'm curious to know why we are just calling `SetProperty` interceptor when `holder == receiver`.
'language/import/import-defer/deferred-namespace-object/exotic-object-behavior': [SKIP],just remove these as the default is PASS
- Igor Sheludko
- Leszek Swirski
- Olivier Flückiger
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Caio Lima (Gerrit)
Caio Lima added 2 comments![]( "Open in Gerrit")
handler = MaybeObjectHandle(LoadHandler::LoadSlow(isolate()));Caio LimaIs this the case where the value itself then triggers the evaluation?
It would be nice if the IC is not permanently slow, but rather we wait with updating it until the module is evaluated.
Olivier FlückigerIs this the case where the value itself then triggers the evaluation?
I'm not sure if I understand the question, but the case here is that a missing access to a deferred namespace should trigger the evaluation to then return undefined. The logic behind this part of the spec is that it triggers the evaluation because it needs to load all exported names.Regarding slow path installation, the hypothesis I'm thinking here is that when ICs start to get filled, the module is quite likely to be evaluated already, due to `--lazy-feedback-allocation`. IIUC, on default configuration the vector is allocated when tiering up to Sparkplug and it's likely that an access to the module already happened at this point. So when `status != kEvaluated` here, probably is due to an error on evaluation, and since we need to throw on each access, I thought it would be a good time to give up on IC. One alternative is to actually just install slow path if `status == kError`. WDYT?
Olivier FlückigerI think only updating on error would be optimal. Without an error we should be guaranteed to have an evaluated module on the next access, or not? so updating again should definitely pay off.
Igor SheludkoAh btw. your understanding of lazy fbv allocation is correct. But it could still happen that the access is e.g., in a branch that was not executed so far.
Caio Lima+1 to this question. IIUC we are loading a property from the module and by this time I think `LookupIterator` itself or `LookupForRead` should have already triggered module evaluation and thus we should already know whether the property is there or not (on this branch - it's not found). I think there should be no special handling of modules in `if (!lookup->IsFound()) {` branch.
Igor SheludkoI think it's the case, since all async dependencies are already evaluated once such access happens.
The case being considered when `!lookup->IsFound()` is when there's an error while accessing a deferred module. For such case we need to throw the same error on every access. The semantics is the same if the property is present or not. Considering how this implementation is now, if we cache on
!lookup->IsFound() and the evaluation throws an error, next accesses will not throw. This happens because `UpdateCaches` is called before we actually trigger the deferred evaluation.
I see. Then having a separate `LookupIterator` state `DEFERRED_MODULE` or `FAILED_DEFERRED_MODULE` would probably be helpful here. The lookup iterator could transition there in case the holder is the deferred module that failed during evaluation.
Done
return MaybeObjectHandle(LoadHandler::LoadSlow(isolate()));Caio LimaSame concern as above - I think by this time the module should already be evaluated (by `LookupIterator` or by `LookupForRead`).
Igor SheludkoAs I mentioned above, module evaluation happens on `Object::GetProperty`, but UpdateCaches is called before it[1].
[1] - https://github.com/v8/v8/blob/main/src/ic/ic.cc#L438-L456
I was actually suggesting to make sure that the `LookupIterator` or at least `LookupForRead` function trigger module evaluation by the time we reach here.
Done
Olivier Flückiger (Gerrit)
Olivier Flückiger added 3 comments![]( "Open in Gerrit")
is_internal: bool: 1 bit;I added this new bitfield so we are able to figure out if an interceptor is internal or not. Such distinction came in hand mainly on `SetPropertyInternal`, but also on other interceptor paths. The overall idea for the creating such distinction is to avoid introducing any observable change for non-internal interceptors.
I think it makes sense.
if (interceptor->is_internal() && IsNull(*result)) {Maybe `false` would be a more natural marker value?
it->GetInterceptor()->is_internal()) {This is one of the reasons why it took me a while to upload a new patch. I didn't fully understand why there's a `it->HolderIsReceiverOrHiddenPrototype()` check before calling `JSObject::SetPropertyWithInterceptor`, but this gave me some trouble to properly implement `JSDeferredModuleNamespace` [[Set]] without triggering an evaluation. I added a condition on internal interceptors to avoid introduce breaking changes to current interceptors API, since there'll be a trap on `JSObject::GetPropertyAttributesWithInterceptor` if `it->HolderIsReceiverOrHiddenPrototype() == false`. However, I'm curious to know why we are just calling `SetProperty` interceptor when `holder == receiver`.
@ish...@chromium.org do you know why? Maybe we cannot guarantee that we check for interceptors on all stores in optimized code?
Related details
- Caio Lima
- Igor Sheludko
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Igor Sheludko (Gerrit)
Igor Sheludko added 1 comment![]( "Open in Gerrit")
it->GetInterceptor()->is_internal()) {Olivier FlückigerThis is one of the reasons why it took me a while to upload a new patch. I didn't fully understand why there's a `it->HolderIsReceiverOrHiddenPrototype()` check before calling `JSObject::SetPropertyWithInterceptor`, but this gave me some trouble to properly implement `JSDeferredModuleNamespace` [[Set]] without triggering an evaluation. I added a condition on internal interceptors to avoid introduce breaking changes to current interceptors API, since there'll be a trap on `JSObject::GetPropertyAttributesWithInterceptor` if `it->HolderIsReceiverOrHiddenPrototype() == false`. However, I'm curious to know why we are just calling `SetProperty` interceptor when `holder == receiver`.
@ish...@chromium.org do you know why? Maybe we cannot guarantee that we check for interceptors on all stores in optimized code?
This new behavior contradicts JS semantics of `[[Set]]` operation. Imagine the following setup:
```
var p = {x:42};
var obj = {__proto__: p};
obj.x = 153;
console.log(obj.x); // 153
Object.defineProperty(p, "y", {value:55, writable: false});
obj.y = 153;
console.log(obj.y); // 55
```
When we store a property to an object and there's data property with the same name in prototype chain then `p.x/p.y` will never be updated but the writability of `p.x/p.y` will define whether the property will be added to `obj` or not (JS spec does not allow to "override" non-writable properties, and we need to throw TypeError if the override attempt happens in strict mode).
What's happening with the module evaluation? Do we trigger evaluation via prototype chain? Can a JSModule become a prototype of arbitrary object in user code?
**TLDR:** In general we are moving towards limiting the power of interceptors Api to serve just as a dynamic collection of *data* properties unlike the `Proxy`-like Api which might serve as anything. The reason is that interceptors were introduced for Chrome which uses only half of the provided functionality while it has to pay the full price of passing unnecessary values (for example, receiver) to the callbacks. The new interceptors Api (once we get there) will allow to create only data properties.
I'm sorry I haven't foreseen this issue before you implemented everything.
Now I'm leaning even more towards the idea of adding a `LookupIterator::DEFERRED_MODULE` state for which Get/Set methods should just trigger the module object evaluation (which would populate the object with respective data properties), handle potential exception and just proceed lookup based on the new state of the module object. This new state would be similar to `LookupIterator::ACCESS_CHECK`.
Please give this idea a though, I think with this approach the implementation would be way simpler and not require any hacks.
Related details
- Caio Lima
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicolò Ribaudo (Gerrit)
Nicolò Ribaudo added 1 comment![]( "Open in Gerrit")
it->GetInterceptor()->is_internal()) {Olivier FlückigerThis is one of the reasons why it took me a while to upload a new patch. I didn't fully understand why there's a `it->HolderIsReceiverOrHiddenPrototype()` check before calling `JSObject::SetPropertyWithInterceptor`, but this gave me some trouble to properly implement `JSDeferredModuleNamespace` [[Set]] without triggering an evaluation. I added a condition on internal interceptors to avoid introduce breaking changes to current interceptors API, since there'll be a trap on `JSObject::GetPropertyAttributesWithInterceptor` if `it->HolderIsReceiverOrHiddenPrototype() == false`. However, I'm curious to know why we are just calling `SetProperty` interceptor when `holder == receiver`.
Igor Sheludko@ish...@chromium.org do you know why? Maybe we cannot guarantee that we check for interceptors on all stores in optimized code?
This new behavior contradicts JS semantics of `[[Set]]` operation. Imagine the following setup:
```
var p = {x:42};
var obj = {__proto__: p};
obj.x = 153;
console.log(obj.x); // 153
Object.defineProperty(p, "y", {value:55, writable: false});
obj.y = 153;
console.log(obj.y); // 55
```
When we store a property to an object and there's data property with the same name in prototype chain then `p.x/p.y` will never be updated but the writability of `p.x/p.y` will define whether the property will be added to `obj` or not (JS spec does not allow to "override" non-writable properties, and we need to throw TypeError if the override attempt happens in strict mode).What's happening with the module evaluation? Do we trigger evaluation via prototype chain? Can a JSModule become a prototype of arbitrary object in user code?
**TLDR:** In general we are moving towards limiting the power of interceptors Api to serve just as a dynamic collection of *data* properties unlike the `Proxy`-like Api which might serve as anything. The reason is that interceptors were introduced for Chrome which uses only half of the provided functionality while it has to pay the full price of passing unnecessary values (for example, receiver) to the callbacks. The new interceptors Api (once we get there) will allow to create only data properties.
I'm sorry I haven't foreseen this issue before you implemented everything.
Now I'm leaning even more towards the idea of adding a `LookupIterator::DEFERRED_MODULE` state for which Get/Set methods should just trigger the module object evaluation (which would populate the object with respective data properties), handle potential exception and just proceed lookup based on the new state of the module object. This new state would be similar to `LookupIterator::ACCESS_CHECK`.
Please give this idea a though, I think with this approach the implementation would be way simpler and not require any hacks.
When we store a property to an object and there's data property with the same name in prototype chain then p.x/p.y will never be updated but the writability of p.x/p.y will define whether the property will be added to obj or not (JS spec does not allow to "override" non-writable properties, and we need to throw TypeError if the override attempt happens in strict mode).
> What's happening with the module evaluation? Do we trigger evaluation via prototype chain? Can a JSModule become a prototype of arbitrary object in user code?
When you have this code:
```
import defer * as ns from "mod"; // assume it exports `const x = 2`
var obj = { __proto__: ns };
obj.x = 3;
```
What happens is that:
- `obj.x` runs `obj.[[Set]]("x", 3, obj)
- it calls `OrdinarySet(obj, "x", 3, obj)`
- it calls `obj.[[GetOwnProperty]]("x")`, which returns undefined because `x` is not an own property
- then, it moves to the prototype (in OrdinarySetWithOwnDescriptor): it calls `ns.[[Set]]("x", 3, obj)`, which just returns `false` and we are done
`[[Set]]` does not "check the descriptor on the prototype". `[[Set]]` just calls `[[Set]]` from the prototype, without ever reading properties from it.
If `ns` was an ordinary object, it would still be `ns.[[Set]]` that checks its own descriptor and then installs the property on `obj`, and not `obj.[[Set]]` itself.
Related details
- Caio Lima
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Flückiger (Gerrit)
Olivier Flückiger added 1 comment![]( "Open in Gerrit")
@caio...@igalia.com did you already try to handle deferred modules with a dedicated iterator state and hit any insurmountable issues? From looking at the CL as it is now, I wonder if the actual code and logic wouldn't almost stay the same. Of course the interceptor implementation would move to the respective cases in the various switches handling the lookup iterator states instead.
Making the unevaluated module a special receiver map might help (see https://source.chromium.org/chromium/chromium/src/+/main:v8/src/objects/lookup.cc;l=86?q=file:lookup.cc&ss=chromium)
Igor Sheludko (Gerrit)
Igor Sheludko added 1 comment![]( "Open in Gerrit")
@nrib...@igalia.com , ah, you are right! Thanks!
Anyway, adding a new `LookupIterator` state is still the easiest way to go.
Caio Lima (Gerrit)
Caio Lima added 1 comment![]( "Open in Gerrit")
Did you already try to handle deferred modules with a dedicated iterator state and hit any insurmountable issues?
I tried it before, and doesn't look like it's impossible. My feeling after implementing with interceptors is that most of the code I needed to put in each callback should go somewhere else when implementing with `LookupIterator`. Another thing I need to keep in mind using `LookupIterator` is the new check we need to do to see if we are passing through `JSDeferredModuleNamespace`. I can give another real try there.
Related details
- Igor Sheludko
- Leszek Swirski
- Nicolò Ribaudo
- Olivier Flückiger
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Igor Sheludko (Gerrit)
Igor Sheludko added 1 comment![]( "Open in Gerrit")
Some hints, just in case:
- [this predicate](https://source.chromium.org/chromium/chromium/src/+/main:v8/src/objects/lookup.cc;l=86?q=%22if%20(IsSpecialReceiverMap(map))%20%7B%22&ss=chromium) should return true for deferred module objects (this would also make sure that `AccessorAssembler::GenericPropertyLoad()` handles such objects correctly),
- detection of an non-evaluated deferred module should go somewhere [here](https://source.chromium.org/chromium/chromium/src/+/main:v8/src/objects/lookup.cc;l=1329?q=%22LookupIterator::State%20LookupIterator::LookupInSpecialHolder),
- you'll need to add a bunch of `case LookupIterator::DEFERRED_MODULE:` into various switches, some of them would be UNREACHABLE and some would have to trigger module evaluation and proceed lookup.
Related details
- Caio Lima
Caio Lima (Gerrit)
Caio Lima added 1 comment![]( "Open in Gerrit")
Thanks a lot for the guidance here. I prepared a separated CL that implements it using a dedicated LookupIterator here (https://chromium-review.googlesource.com/c/v8/v8/+/7247865) and only implements [[Get]] to make it simpler. The reason to separate it is that I think handling all operations in one single patch feels like too much, and my idea is to split it on Patches for each internal operation ([[Get]], [[Set]], [[Delete]], [[GetOwnProperty]], etc). The first patch with [[Get]] is probably going to be the biggest one, since we need to include all the logic to create JSDeferredModuleNamespace, but I think the following CLs will be much focused and better to reason/discuss.
Related details
- Igor Sheludko
Caio Lima (Gerrit)
Caio Lima abandoned this change![]( "Open in Gerrit")
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |