I opened a PR related to this to fix node-ci compilation problem in https://github.com/v8/node/pull/231
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thanks, some initial comments
kEvaluation,
Unfortunately this won't work as we are changing the API in a way that breaks clients. We need to expose a new enum, a new enum callback type and and setter. Only this way we can move clients to the new api.
if (node->phase() == ModuleImportPhase::kSource) {
nit: can we convert this to a switch?
flags: Smi;
That does not seem right. If it is Smi tagged it means we can't use the lowest bit (as it needs to be 0 in the case of Smis). So either this is a raw field and the GC needs to treat it correctly. Or it stays a SmiTagged and fits into 31 bits. Can we steal a bit from position?
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
kEvaluation,
Unfortunately this won't work as we are changing the API in a way that breaks clients. We need to expose a new enum, a new enum callback type and and setter. Only this way we can move clients to the new api.
Btw. we also need to ensure that we don't accidentally pass the new enum to an old style callback. I suggest that for the transition period we will keep two separate callbacks that can be installed (with a check that not both are present). And a check to ensure we don't pass the new value to the old-style callback.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thank you very much for the reviews. Before sending a new Patchset, I have some minor questions.
kEvaluation,
Olivier FlückigerUnfortunately this won't work as we are changing the API in a way that breaks clients. We need to expose a new enum, a new enum callback type and and setter. Only this way we can move clients to the new api.
Btw. we also need to ensure that we don't accidentally pass the new enum to an old style callback. I suggest that for the transition period we will keep two separate callbacks that can be installed (with a check that not both are present). And a check to ensure we don't pass the new value to the old-style callback.
Oh, this path is a bit unfortunate, but it does make sense. I'm reworking in this change and I have 2 questions:
1. Would a `ModuleImportPhaseV2` be a good name for this new enum?
2. Implementing this new enum, I didn't need to change anything on `HostImportModuleWithPhaseDynamicallyCallback` yet. The changes are more internal and not exposed to embedders. Would you like me to add it in this patch, or should it come when implementing `import.defer` logic, when we'll need to add support for it on d8.cc?
My plan there is to add a new `HostImportModuleWithPhaseV2DynamicallyCallback`, setters, and the logic you are proposing.
flags: Smi;
That does not seem right. If it is Smi tagged it means we can't use the lowest bit (as it needs to be 0 in the case of Smis). So either this is a raw field and the GC needs to treat it correctly. Or it stays a SmiTagged and fits into 31 bits. Can we steal a bit from position?
I misunderstood Smis, my bad. Your review makes sense, and it's probably safe to steal 1 bit for position, since it will be able to go up to `536870912`. It seems a reasonable max number to be represent a position.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
kEvaluation,
Olivier FlückigerUnfortunately this won't work as we are changing the API in a way that breaks clients. We need to expose a new enum, a new enum callback type and and setter. Only this way we can move clients to the new api.
Caio LimaBtw. we also need to ensure that we don't accidentally pass the new enum to an old style callback. I suggest that for the transition period we will keep two separate callbacks that can be installed (with a check that not both are present). And a check to ensure we don't pass the new value to the old-style callback.
Oh, this path is a bit unfortunate, but it does make sense. I'm reworking in this change and I have 2 questions:
1. Would a `ModuleImportPhaseV2` be a good name for this new enum?
2. Implementing this new enum, I didn't need to change anything on `HostImportModuleWithPhaseDynamicallyCallback` yet. The changes are more internal and not exposed to embedders. Would you like me to add it in this patch, or should it come when implementing `import.defer` logic, when we'll need to add support for it on d8.cc?My plan there is to add a new `HostImportModuleWithPhaseV2DynamicallyCallback`, setters, and the logic you are proposing.
1. I like adding V2
2. Up to you, but adding the
kEvaluation,
Olivier FlückigerUnfortunately this won't work as we are changing the API in a way that breaks clients. We need to expose a new enum, a new enum callback type and and setter. Only this way we can move clients to the new api.
Caio LimaBtw. we also need to ensure that we don't accidentally pass the new enum to an old style callback. I suggest that for the transition period we will keep two separate callbacks that can be installed (with a check that not both are present). And a check to ensure we don't pass the new value to the old-style callback.
Oh, this path is a bit unfortunate, but it does make sense. I'm reworking in this change and I have 2 questions:
1. Would a `ModuleImportPhaseV2` be a good name for this new enum?
2. Implementing this new enum, I didn't need to change anything on `HostImportModuleWithPhaseDynamicallyCallback` yet. The changes are more internal and not exposed to embedders. Would you like me to add it in this patch, or should it come when implementing `import.defer` logic, when we'll need to add support for it on d8.cc?My plan there is to add a new `HostImportModuleWithPhaseV2DynamicallyCallback`, setters, and the logic you are proposing.
If you have an idea on how to get around it I am all ears. We didn't come up with one since we can never know if the client already supports the new enum or not.
1. Sounds good.
2. Up to you, but adding the callback already here would make sense.
flags: Smi;
Caio LimaThat does not seem right. If it is Smi tagged it means we can't use the lowest bit (as it needs to be 0 in the case of Smis). So either this is a raw field and the GC needs to treat it correctly. Or it stays a SmiTagged and fits into 31 bits. Can we steal a bit from position?
I misunderstood Smis, my bad. Your review makes sense, and it's probably safe to steal 1 bit for position, since it will be able to go up to `536870912`. It seems a reasonable max number to be represent a position.
Yeah, a Smi is an int31 embedded into an int32. The lowest bit is set to 0. This is used by the GC to know that it can ignore the value. A SmiTagged basically means, we are abusing a Smi here to be able to store a raw value of some other type into 31bits and stick a Smi tag on it, so the GC ignores it.
Stealing from position sounds good then.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
kEvaluation,
Olivier FlückigerUnfortunately this won't work as we are changing the API in a way that breaks clients. We need to expose a new enum, a new enum callback type and and setter. Only this way we can move clients to the new api.
Caio LimaBtw. we also need to ensure that we don't accidentally pass the new enum to an old style callback. I suggest that for the transition period we will keep two separate callbacks that can be installed (with a check that not both are present). And a check to ensure we don't pass the new value to the old-style callback.
Olivier FlückigerOh, this path is a bit unfortunate, but it does make sense. I'm reworking in this change and I have 2 questions:
1. Would a `ModuleImportPhaseV2` be a good name for this new enum?
2. Implementing this new enum, I didn't need to change anything on `HostImportModuleWithPhaseDynamicallyCallback` yet. The changes are more internal and not exposed to embedders. Would you like me to add it in this patch, or should it come when implementing `import.defer` logic, when we'll need to add support for it on d8.cc?My plan there is to add a new `HostImportModuleWithPhaseV2DynamicallyCallback`, setters, and the logic you are proposing.
If you have an idea on how to get around it I am all ears. We didn't come up with one since we can never know if the client already supports the new enum or not.
1. Sounds good.
2. Up to you, but adding the callback already here would make sense.
No, I don't have a proper solution here. What I had in mind before was to do a breaking change, since Source Phase Imports still is in stage 3. However I do think it's better to avoid the breaking changes in the API.
if (node->phase() == ModuleImportPhase::kSource) {
nit: can we convert this to a switch?
Acknowledged
Caio LimaThat does not seem right. If it is Smi tagged it means we can't use the lowest bit (as it needs to be 0 in the case of Smis). So either this is a raw field and the GC needs to treat it correctly. Or it stays a SmiTagged and fits into 31 bits. Can we steal a bit from position?
Olivier FlückigerI misunderstood Smis, my bad. Your review makes sense, and it's probably safe to steal 1 bit for position, since it will be able to go up to `536870912`. It seems a reasonable max number to be represent a position.
Yeah, a Smi is an int31 embedded into an int32. The lowest bit is set to 0. This is used by the GC to know that it can ignore the value. A SmiTagged basically means, we are abusing a Smi here to be able to store a raw value of some other type into 31bits and stick a Smi tag on it, so the GC ignores it.
Stealing from position sounds good then.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
kEvaluation,
Olivier FlückigerUnfortunately this won't work as we are changing the API in a way that breaks clients. We need to expose a new enum, a new enum callback type and and setter. Only this way we can move clients to the new api.
Caio LimaBtw. we also need to ensure that we don't accidentally pass the new enum to an old style callback. I suggest that for the transition period we will keep two separate callbacks that can be installed (with a check that not both are present). And a check to ensure we don't pass the new value to the old-style callback.
Olivier FlückigerOh, this path is a bit unfortunate, but it does make sense. I'm reworking in this change and I have 2 questions:
1. Would a `ModuleImportPhaseV2` be a good name for this new enum?
2. Implementing this new enum, I didn't need to change anything on `HostImportModuleWithPhaseDynamicallyCallback` yet. The changes are more internal and not exposed to embedders. Would you like me to add it in this patch, or should it come when implementing `import.defer` logic, when we'll need to add support for it on d8.cc?My plan there is to add a new `HostImportModuleWithPhaseV2DynamicallyCallback`, setters, and the logic you are proposing.
Caio LimaIf you have an idea on how to get around it I am all ears. We didn't come up with one since we can never know if the client already supports the new enum or not.
1. Sounds good.
2. Up to you, but adding the callback already here would make sense.
No, I don't have a proper solution here. What I had in mind before was to do a breaking change, since Source Phase Imports still is in stage 3. However I do think it's better to avoid the breaking changes in the API.
The new delay phase will hide behind the flag `js_defer_import_eval`. Embedders do not set this flag will not receive thew new enum value though.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
kEvaluation,
Olivier FlückigerUnfortunately this won't work as we are changing the API in a way that breaks clients. We need to expose a new enum, a new enum callback type and and setter. Only this way we can move clients to the new api.
Caio LimaBtw. we also need to ensure that we don't accidentally pass the new enum to an old style callback. I suggest that for the transition period we will keep two separate callbacks that can be installed (with a check that not both are present). And a check to ensure we don't pass the new value to the old-style callback.
Olivier FlückigerOh, this path is a bit unfortunate, but it does make sense. I'm reworking in this change and I have 2 questions:
1. Would a `ModuleImportPhaseV2` be a good name for this new enum?
2. Implementing this new enum, I didn't need to change anything on `HostImportModuleWithPhaseDynamicallyCallback` yet. The changes are more internal and not exposed to embedders. Would you like me to add it in this patch, or should it come when implementing `import.defer` logic, when we'll need to add support for it on d8.cc?My plan there is to add a new `HostImportModuleWithPhaseV2DynamicallyCallback`, setters, and the logic you are proposing.
Caio LimaIf you have an idea on how to get around it I am all ears. We didn't come up with one since we can never know if the client already supports the new enum or not.
1. Sounds good.
2. Up to you, but adding the callback already here would make sense.
Chengzhong WuNo, I don't have a proper solution here. What I had in mind before was to do a breaking change, since Source Phase Imports still is in stage 3. However I do think it's better to avoid the breaking changes in the API.
The new delay phase will hide behind the flag `js_defer_import_eval`. Embedders do not set this flag will not receive thew new enum value though.
Ah, that is actually an excellent point that I had not considered.
Maybe we can get around the breakage in the downstream builds if we temporarily annotate the enum with `__attribute__((enum_extensibility(open)))` then.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
kEvaluation,
Olivier FlückigerUnfortunately this won't work as we are changing the API in a way that breaks clients. We need to expose a new enum, a new enum callback type and and setter. Only this way we can move clients to the new api.
Caio LimaBtw. we also need to ensure that we don't accidentally pass the new enum to an old style callback. I suggest that for the transition period we will keep two separate callbacks that can be installed (with a check that not both are present). And a check to ensure we don't pass the new value to the old-style callback.
Olivier FlückigerOh, this path is a bit unfortunate, but it does make sense. I'm reworking in this change and I have 2 questions:
1. Would a `ModuleImportPhaseV2` be a good name for this new enum?
2. Implementing this new enum, I didn't need to change anything on `HostImportModuleWithPhaseDynamicallyCallback` yet. The changes are more internal and not exposed to embedders. Would you like me to add it in this patch, or should it come when implementing `import.defer` logic, when we'll need to add support for it on d8.cc?My plan there is to add a new `HostImportModuleWithPhaseV2DynamicallyCallback`, setters, and the logic you are proposing.
Caio LimaIf you have an idea on how to get around it I am all ears. We didn't come up with one since we can never know if the client already supports the new enum or not.
1. Sounds good.
2. Up to you, but adding the callback already here would make sense.
Chengzhong WuNo, I don't have a proper solution here. What I had in mind before was to do a breaking change, since Source Phase Imports still is in stage 3. However I do think it's better to avoid the breaking changes in the API.
Olivier FlückigerThe new delay phase will hide behind the flag `js_defer_import_eval`. Embedders do not set this flag will not receive thew new enum value though.
Ah, that is actually an excellent point that I had not considered.
Maybe we can get around the breakage in the downstream builds if we temporarily annotate the enum with `__attribute__((enum_extensibility(open)))` then.
After reading this I'm also thinking that using a build time flag would also work at some extent. However, I'm not sure how you feel about adding a new build flag for it. This would require some ifdefs here and there, which I'm a bit against, but might be doable.
Maybe we can get around the breakage in the downstream builds if we temporarily annotate the enum with __attribute__((enum_extensibility(open))) then.
I'm not sure if I follow here. IIUC, if we annotated `ModuleImportPhase`, this still causes breakage on switches that doesn't have a default clause, right? One example is that this would still break https://github.com/nodejs/node/blob/0c35aaf55f489abb55c9990d0bbeacdf4b2b0e62/src/module_wrap.cc#L561. Without patching Node (or other clients), I didn't get how we can avoid the breaking change.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ok, yes, my idea doesn't work and ifdefs sounds annoying indeed.
Adding a V2 api might be the easiest path. But we don't need a second callback slot. Both api's can store at the same callback location (casting to the correct signature) and on callback check that the flag is enabled before passing the new value back.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
If we are going to add a new callback signature, can we make the callback's parameter phase to be an `int` so that we don't need to change the callback again when we add a new phase?
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fixing the switch-case in Node.js: https://github.com/nodejs/node/pull/60261
I would prefer to keep it an enum even if it adds a bit of overhead.
That said it seems like chrome does not care about the added enum value (I just checked). So if you want to wait until the node fix rolls into our ci, that is also an option for me.
FWIW Node.js does not enable switch exhaustive check so it's going to be just a warning. I don't think Node.js is blocking us from adding a value in existing enum either.
Or does V8 nodejs ci enables `-Wswitch-enum` as errors? From what I can tell, the Node.js' own CI does not treat this as an error.
Good to know. So let's avoid creating a new enum then. I'll revert those changes and wait until the node-ci gets the patch as well.
Or does V8 nodejs ci enables -Wswitch-enum as errors? From what I can tell, the Node.js' own CI does not treat this as an error.
What I see in the node-ci is that it uses `-Wswitch`, but not `-Wswitch-enum`. So once your patch lands in Node-ci, we won't have this issue anymore.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sorry for the back and forth... I looked into updating node in node-ci and realized that it will take some time and we don't want to do it more than necessary. Your initial plan with https://github.com/v8/node/pull/231 might have been the best option after all. If you re-open it I can get it merged.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
I sent a new Patchset to solve merge conflicts with main. The Code Review flag vanished for this new Patchset, so I don't know if I need to get the review again.
kEvaluation,
I noticed that my message stayed in draft. thanks a lot for merging the PR and also updating DEPS!
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Code-Review | +1 |
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Code-Review | +0 |
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Code-Review | +1 |
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
[defer-import-eval] Parse import defer syntax
Add support for `import defer * as ns` and `import.defer` syntax that
is behind the runtime flag named `--js-defer-import-eval`. Deferred imports
are marked as ModuleImportPhase::kDefer, that is going to be used on
other parts of runtime to properly handle new deferred semantics.
Only namespace imports can be marked as deferred.
Design doc: https://docs.google.com/document/d/1DKss76uvFDEy3UHgc2Vtmy1hw5SNTCrEJqUF2aKATxQ
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |