| 21:51 | <ljharb> | shu: i have a package that sets Symbol.isConcatSpreadable on a few objects, for the purpose of ensuring that array concat still works as expected even if someone does a user is reporting that v8 becomes permanently slow if Symbol.isConcatSpreadable is ever set, even once, on any object - and deleting it later doesn't fix it. is there anything that could be done here to make it less pathological to set it on an arbitrary object? |
| 21:53 | <ljharb> | https://github.com/ljharb/safe-array-concat/pull/3#issuecomment-1707355732 for context |
| 22:10 | <shu> | this i can easily confirm: assigning a property on any object with a name of Symbol.isConcatSpreadable triggers the protector |
| 22:10 | <shu> | protectors are 1-way, there's no way to un-trigger them |
| 22:13 | <shu> | the short answer is i don't know to make it less pathological, because the concat spreadable behavior is that we need to check it on arbitrary objects |
| 22:13 | <bakkot> | https://chromium.googlesource.com/v8/v8.git/+/HEAD/src/builtins/builtins-array.cc#1276 |
| 22:13 | <shu> | there might be something possible here to carve out more fast paths |
| 22:13 | <bakkot> | "Slow path if @@isConcatSpreadable has been used" |
| 22:15 | <shu> | the current tradeoff is that Symbol.isConcatSpreadable is rare enough to just have a blanket slow path |
| 22:28 | <shu> | here's the data: https://chromestatus.com/metrics/feature/timeline/popularity/3261 |
| 22:28 | <shu> | it's more than i thought actually |
| 22:28 | <shu> | but still not that much |
| 22:29 | <shu> | the main problem i see is that the slow path for Array.concat is really slow, like, it uses a hash table for the numbers for the resulting array |
| 22:30 | <shu> | there's probably something to do there for concat spreadable so the cliff isn't quite so big |
| 22:30 | <shu> | you could also, like, directly implement concat though? |
| 22:30 | <shu> | instead of setting @@isConcatSpreadable |
| 22:37 | <bakkot> | just for interest: JSC doesn't have a fast path at all, and Firefox has a per-object "can't have interesting symbol" fast path |
| 23:09 | <ljharb> | a per-object fast path makes sense to me |
| 23:09 | <ljharb> | and yeah i suppose i could reimplement concat, but i suspect my concat implementation would be slower than the fast concat? |
| 23:10 | <ljharb> | as for "rare", this package has 10.3M weekly downloads by transitive usage, so it's not all that rare, but i'm going to be shipping the "fix" soon which is that it will only assign isConcatSpreadable if someone else has already done so |
| 23:18 | <shu> | i gave you data quantifying what i mean by rare... |
| 23:20 | <shu> | i also wasn't talking about the popularity of your package, but all instances where someone mutated @@isConcatSpreadable |