| 10:21 | <snek> | in this code is there a particularly correct way to introduce a week "offset"? like to look at the previous week or the next week or 12 weeks from now. can i just append add({ weeks: n }) to the startOfWeek expression and all the rest of the math will still work? |
| 21:46 | <Mathieu Hofman> | @bakkot:matrix.org: btw the resolver holding the promise is not the only leak affecting Promise.race, it's just the most egregious. The other one is the resolver functions of the race result being held by a long-lived raced promise. This is common for these to accumulate in cases where you have an "on exit" type promise that gets continuously raced. This case can be generalized as: a resolver function which no longer has any observable effect (because its linked promise was resolved) being held internally in some spec structure (in this case the reaction list of a long lived promise). |
| 21:58 | <bakkot> | Yeah that's a lot more annoying to fix and also try functions themselves are unlikely to hold much data so it matters a lot less |
| 22:00 | <bakkot> | Like if you think about what it would take to solve it, you end up at a complete refactor of either the promise machinery or GC |
| 22:00 | <bakkot> | So it's probably never happening |
| 22:52 | <Mathieu Hofman> | The race case doesn't need a full refactor as it can do its own bookkeeping. That's what we do in https://github.com/endojs/endo/blob/master/packages/promise-kit/src/memo-race.js |
| 22:54 | <Mathieu Hofman> | But also changing the promise machinery to better track promises resolved to other promises (or conditionally resolved in the case of race) is something I've been wanting to explore for a while. I wrote a prototype promise implementation that does some of that a few years ago. Bonus: it would properly detect resolution cycles |
| 23:12 | <bakkot> | You'd need Promises to know when callbacks previously passed to their .thens have become no-ops (e.g. because they are resolvers for already-resolved functions) and mark those as available for GC, and given how engines do GC that's a huge lift |
| 23:19 | <bakkot> | incidentally, Safari has the same .race leak I'm fixing in Chrome |
| 23:20 | <bakkot> | I am like 80% sure a sufficient fix is to null out the JSFunctionWithFields::Field::ResolvingPromise on both callee and other after these lines https://github.com/WebKit/WebKit/blob/574fccad44f052870183b8d89b7f2f7df2ecfcb9/Source/JavaScriptCore/runtime/JSPromise.cpp#L401-L405 |
| 23:20 | <bakkot> | I will try to get to this at some point but have no experience contributing to Webkit, so if anyone else does have such experience and wants to beat me to the punch please do so (and ping me so I don't duplicate effort) |
| 23:21 | <bakkot> | I don't think this would break anything but I haven't carefully traced how ResolvingPromise is used yet |
| 23:49 | <Mathieu Hofman> | You'd need Promises to know when callbacks previously passed to their |
| 23:53 | <Mathieu Hofman> | But if we were avoiding this in general we'd need to avoid using resolver functions as reactions when the resolution value is a native promise. Shouldn't require any special GC behavior. |
| 23:55 | <Mathieu Hofman> | incidentally, Safari has the same |