Uh oh!
There was an error while loading. Please reload this page.
src: add internal isArrayBufferDetached - #45568
Conversation
7d64817 to
dcee530Comparenodejs-github-bot
commented
Nov 21, 2022
dcee530 to
4d5d53bComparenodejs-github-bot
commented
Nov 22, 2022
| function isArrayBufferDetached(value) { | ||
| if (ArrayBufferPrototypeGetByteLength(value) === 0) { | ||
| return _isArrayBufferDetached(value); |
There was a problem hiding this comment.
Crossing the JS-C++ boundary is gonna be slower than the current implementation although I'm not sure if this call in the zero length case is gonna be the primary bottleneck for any user code.
Did you consider implementing @jasnell's suggestion in #45512 (comment)? Seems like that approach would solve this problem without adding any performance overhead in any case.
One thing we might be able to do is add a hidden symbol to ArrayBuffer instances when we detach them in c++ land. Then we can have an isDetached check in js that checks the length and the presence of that symbol.
There was a problem hiding this comment.
IMHO, that is the perfect solution for performance, but I believe it's too hacky to maintain in the long term. I can't think of a good/maintainable way to enforce attaching the hidden symbol as a rule.
If it's the consensus @jasnell thinks is the best way, I'll be happy to implement it regardless.
nodejs-github-bot
commented
Nov 23, 2022
LiviaMedeiros
left a comment
There was a problem hiding this comment.
LGTM with or without a nit.
Adding a test would also be nice.
| if (ArrayBufferPrototypeGetByteLength(value) === 0) { | ||
| return _isArrayBufferDetached(value); | ||
| } | ||
| return false; |
There was a problem hiding this comment.
Nit: the flow might look semantically better like this
| if(ArrayBufferPrototypeGetByteLength(value)===0){ | |
| return_isArrayBufferDetached(value); | |
| } | |
| returnfalse; | |
| if(ArrayBufferPrototypeGetByteLength(value)!==0){ | |
| returnfalse; | |
| } | |
| return_isArrayBufferDetached(value); |
nodejs-github-bot
commented
Nov 24, 2022
nodejs-github-bot
commented
Nov 27, 2022
Landed in 9a8d82b |
PR-URL: #45568 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: LiviaMedeiros <livia@cirno.name>
PR-URL: #45568 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: LiviaMedeiros <livia@cirno.name>
PR-URL: #45568 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: LiviaMedeiros <livia@cirno.name>
PR-URL: #45568 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: LiviaMedeiros <livia@cirno.name>
PR-URL: #45568 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: LiviaMedeiros <livia@cirno.name>
Follow up on #45512