209398 2020-03-22 04:39:34 -0700 [JSC] Add JSC::keepAlive(JSValue) 2020-03-23 10:17:21 -0700 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 ysuzuki ysuzuki darin ews-watchlist ggaren keith_miller mark.lam msaboff saam tzagallo webkit-bug-importer oldest_to_newest 1632623 0 ysuzuki 2020-03-22 04:39:34 -0700 [JSC] Add JSC::keepAlive(JSValue) 1632624 1 394212 ysuzuki 2020-03-22 04:40:31 -0700 Created attachment 394212 Patch 1632625 2 ysuzuki 2020-03-22 04:40:46 -0700 Will be used in <rdar://problem/60541567> 1632633 3 394212 mark.lam 2020-03-22 08:05:30 -0700 Comment on attachment 394212 Patch r=me. It would have been nice if there‘s some way to share the implementation instead of having 2 copies, but I couldn’t think of a clean way to express that: probably more hassle than it’s worth. 1632711 4 ysuzuki 2020-03-22 17:39:10 -0700 (In reply to Mark Lam from comment #3) > Comment on attachment 394212 [details] > Patch > > r=me. It would have been nice if there‘s some way to share the > implementation instead of having 2 copies, but I couldn’t think of a clean > way to express that: probably more hassle than it’s worth. Yeah, I was thinking about sharing implementation, but there is many conditions like CPU(ADDRESS64), USE(JSVALUE64) etc. And I think just writing this simple function here would be clear than sharing the implementation by putting a lot of ifdefs. 1632712 5 ysuzuki 2020-03-22 17:40:59 -0700 Committed r258824: <https://trac.webkit.org/changeset/258824> 1632713 6 webkit-bug-importer 2020-03-22 17:41:15 -0700 <rdar://problem/60752174> 1632727 7 394212 ggaren 2020-03-22 20:20:59 -0700 Comment on attachment 394212 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394212&action=review > Source/JavaScriptCore/runtime/JSCJSValue.h:641 > +ALWAYS_INLINE void keepAlive(JSValue value) This function does not seem to keep 'value' alive, so "keep alive" is not a great name for it. It appears that you're trying to show the compiler that a given value is used at the point of the call. A better name for that might simply be "use" or "compilerUse". 1632728 8 394212 ysuzuki 2020-03-22 20:31:55 -0700 Comment on attachment 394212 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394212&action=review >> Source/JavaScriptCore/runtime/JSCJSValue.h:641 >> +ALWAYS_INLINE void keepAlive(JSValue value) > > This function does not seem to keep 'value' alive, so "keep alive" is not a great name for it. > > It appears that you're trying to show the compiler that a given value is used at the point of the call. A better name for that might simply be "use" or "compilerUse". I think this "keepAlive" is keeping 'value' alive. While the implementation of this is `compilerUse` or `use`, the effect and goal of this function is keeping 'value' alive by keeping it in conservative roots at the calling point. So, this "keepAlive" name is describing the purpose and effect of this function, no? 1632729 9 394212 ysuzuki 2020-03-22 20:36:32 -0700 Comment on attachment 394212 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394212&action=review >>> Source/JavaScriptCore/runtime/JSCJSValue.h:641 >>> +ALWAYS_INLINE void keepAlive(JSValue value) >> >> This function does not seem to keep 'value' alive, so "keep alive" is not a great name for it. >> >> It appears that you're trying to show the compiler that a given value is used at the point of the call. A better name for that might simply be "use" or "compilerUse". > > I think this "keepAlive" is keeping 'value' alive. > While the implementation of this is `compilerUse` or `use`, the effect and goal of this function is keeping 'value' alive by keeping it in conservative roots at the calling point. > So, this "keepAlive" name is describing the purpose and effect of this function, no? Note that FTL has the same feature. https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp#L18347-L18355 1632731 10 ggaren 2020-03-22 20:52:02 -0700 Most confusing to me about the phrase "keepAlive" is when the function will stop keeping the value alive. If the goal is to create a root for conservative GC, maybe a better name is "gcRoot" or "conservativeRoot". 1632733 11 ysuzuki 2020-03-22 20:58:54 -0700 (In reply to Geoffrey Garen from comment #10) > Most confusing to me about the phrase "keepAlive" is when the function will > stop keeping the value alive. > > If the goal is to create a root for conservative GC, maybe a better name is > "gcRoot" or "conservativeRoot". I think `gcRoot` and `conservativeRoot` look ambiguous given that we already have `root` function in WebCore, and it is used for completely different purpose: returning opaque-root for the given pointer. Since "root" function is also used in GC's context, I think `gcRoot` / `conservativeRoot` look like the similar / same to `root` function in WebCore. But they are different. How about using `keepAliveAtThisPoint` `ensureReachableFromGCRootAtThisPoint` instead? They clarify that this keep-alive continue until this point. 1632734 12 ysuzuki 2020-03-22 21:01:16 -0700 (In reply to Yusuke Suzuki from comment #11) > (In reply to Geoffrey Garen from comment #10) > > Most confusing to me about the phrase "keepAlive" is when the function will > > stop keeping the value alive. > > > > If the goal is to create a root for conservative GC, maybe a better name is > > "gcRoot" or "conservativeRoot". > > I think `gcRoot` and `conservativeRoot` look ambiguous given that we already > have `root` function in WebCore, and it is used for completely different > purpose: returning opaque-root for the given pointer. Since "root" function > is also used in GC's context, I think `gcRoot` / `conservativeRoot` look > like the similar / same to `root` function in WebCore. But they are > different. > > How about using `keepAliveAtThisPoint` > `ensureReachableFromGCRootAtThisPoint` instead? They clarify that this > keep-alive continue until this point. Example of root function in WebCore. https://github.com/WebKit/webkit/blob/master/Source/WebCore/bindings/js/JSCSSRuleCustom.h#L33-L42 This is not used for keeping value alive in conservative root. It is a function returning opaque-root for the given pointer. 1632736 13 ysuzuki 2020-03-22 21:06:46 -0700 (In reply to Yusuke Suzuki from comment #12) > (In reply to Yusuke Suzuki from comment #11) > > (In reply to Geoffrey Garen from comment #10) > > > Most confusing to me about the phrase "keepAlive" is when the function will > > > stop keeping the value alive. > > > > > > If the goal is to create a root for conservative GC, maybe a better name is > > > "gcRoot" or "conservativeRoot". > > > > I think `gcRoot` and `conservativeRoot` look ambiguous given that we already > > have `root` function in WebCore, and it is used for completely different > > purpose: returning opaque-root for the given pointer. Since "root" function > > is also used in GC's context, I think `gcRoot` / `conservativeRoot` look > > like the similar / same to `root` function in WebCore. But they are > > different. > > > > How about using `keepAliveAtThisPoint` > > `ensureReachableFromGCRootAtThisPoint` instead? They clarify that this > > keep-alive continue until this point. > > Example of root function in WebCore. > https://github.com/WebKit/webkit/blob/master/Source/WebCore/bindings/js/ > JSCSSRuleCustom.h#L33-L42 This is not used for keeping value alive in > conservative root. It is a function returning opaque-root for the given > pointer. If user uses `root` function instead of `gcRoot` / `conservativeRoot` functions to keep value alive, this is wrong. And since both `root` and `gcRoot` will accept void*, it would be difficult to notice. So I think we should pick clear name here. Maybe, "ensureReachableFromGCRootAtThisPoint" would be the best. 1632737 14 mark.lam 2020-03-22 21:42:30 -0700 (In reply to Yusuke Suzuki from comment #13) > If user uses `root` function instead of `gcRoot` / `conservativeRoot` > functions to keep value alive, this is wrong. And since both `root` and > `gcRoot` will accept void*, it would be difficult to notice. So I think we > should pick clear name here. Maybe, "ensureReachableFromGCRootAtThisPoint" > would be the best. How about something shorter like "ensureStillAliveHere"? I like "ensure" better than "keep", and I think it is a better description of what this function is trying to do. 1632739 15 ysuzuki 2020-03-22 21:52:51 -0700 (In reply to Mark Lam from comment #14) > (In reply to Yusuke Suzuki from comment #13) > > If user uses `root` function instead of `gcRoot` / `conservativeRoot` > > functions to keep value alive, this is wrong. And since both `root` and > > `gcRoot` will accept void*, it would be difficult to notice. So I think we > > should pick clear name here. Maybe, "ensureReachableFromGCRootAtThisPoint" > > would be the best. > > How about something shorter like "ensureStillAliveHere"? I like "ensure" > better than "keep", and I think it is a better description of what this > function is trying to do. Sounds fine! I'll rename it to ensureStillAliveHere. 1632740 16 ysuzuki 2020-03-22 21:57:12 -0700 Committed r258825: <https://trac.webkit.org/changeset/258825> 1632905 17 darin 2020-03-23 10:13:58 -0700 Nice! Way better name! 1632908 18 394212 keith_miller 2020-03-23 10:17:21 -0700 Comment on attachment 394212 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394212&action=review >>>> Source/JavaScriptCore/runtime/JSCJSValue.h:641 >>>> +ALWAYS_INLINE void keepAlive(JSValue value) >>> >>> This function does not seem to keep 'value' alive, so "keep alive" is not a great name for it. >>> >>> It appears that you're trying to show the compiler that a given value is used at the point of the call. A better name for that might simply be "use" or "compilerUse". >> >> I think this "keepAlive" is keeping 'value' alive. >> While the implementation of this is `compilerUse` or `use`, the effect and goal of this function is keeping 'value' alive by keeping it in conservative roots at the calling point. >> So, this "keepAlive" name is describing the purpose and effect of this function, no? > > Note that FTL has the same feature. https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp#L18347-L18355 I think it might have been clearer if this was a C++ class that does this when being destructed. 394212 2020-03-22 04:40:31 -0700 2020-03-22 08:05:30 -0700 Patch bug-209398-20200322044031.patch text/plain 2807 ysuzuki U3VidmVyc2lvbiBSZXZpc2lvbjogMjU4ODE4CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCA2 YmFiM2JmZmM0NjY2ODk5MWJhOTk2NWYzZmZjMWFiNmU4ZjQ2NGRlLi4xODViNDY0YmUxYTA3OTBj MTUzYmFhZDgxOGE0YTc5MDg5ZjJlMWU0IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxOCBAQAorMjAyMC0wMy0yMiAgWXVzdWtlIFN1enVraSAgPHlzdXp1a2lAYXBwbGUuY29t PgorCisgICAgICAgIFtKU0NdIEFkZCBKU0M6OmtlZXBBbGl2ZShKU1ZhbHVlKQorICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjA5Mzk4CisKKyAgICAgICAg UmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQWRkIEpTQzo6a2VlcEFsaXZl KEpTVmFsdWUpLiBUaGlzIGlzIHVzZWZ1bCB0byBtYWtlIHNvbWUgSlNWYWx1ZSB2YXJpYWJsZSBh bGl2ZSBmcm9tIEdDLgorCisgICAgICAgICogaGVhcC9IZWFwQ2VsbC5jcHA6CisgICAgICAgICog cnVudGltZS9KU0NKU1ZhbHVlLmNwcDoKKyAgICAgICAgKEpTQzo6a2VlcEFsaXZlKToKKyAgICAg ICAgKiBydW50aW1lL0pTQ0pTVmFsdWUuaDoKKyAgICAgICAgKEpTQzo6a2VlcEFsaXZlKToKKwog MjAyMC0wMy0yMCAgUm9zcyBLaXJzbGluZyAgPHJvc3Mua2lyc2xpbmdAc29ueS5jb20+CiAKICAg ICAgICAgaGFzT2JzZXJ2YWJsZVNpZGVFZmZlY3RzRm9yUmVnRXhwU3BsaXQgZG9lc24ndCBjaGVj ayBmb3IgQEBtYXRjaCBvdmVycmlkZQpkaWZmIC0tZ2l0IGEvU291cmNlL0phdmFTY3JpcHRDb3Jl L2hlYXAvSGVhcENlbGwuY3BwIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL2hlYXAvSGVhcENlbGwu Y3BwCmluZGV4IDhkNzE0NjUwMDNjNTBiOWI3OGYxYWZjMjRhNzNjM2Q2NTE2OTU3OTIuLmI4MDZj OGU3NTVjNDlkMTM1YWUwZWUxYWYyNzIxODI3YjAwZWEyNmYgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9K YXZhU2NyaXB0Q29yZS9oZWFwL0hlYXBDZWxsLmNwcAorKysgYi9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvaGVhcC9IZWFwQ2VsbC5jcHAKQEAgLTQzLDYgKzQzLDcgQEAgYm9vbCBIZWFwQ2VsbDo6aXNM aXZlKCkKIH0KIAogI2lmICFDT01QSUxFUihHQ0NfQ09NUEFUSUJMRSkKKy8vIFRoaXMgbWFrZXMg dGhlIGFyZ3VtZW50IG9wYXF1ZSBmcm9tIHRoZSBjb21waWxlci4KIE5FVkVSX0lOTElORSB2b2lk IGtlZXBBbGl2ZShjb25zdCB2b2lkKikKIHsKIH0KZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2Ny aXB0Q29yZS9ydW50aW1lL0pTQ0pTVmFsdWUuY3BwIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL3J1 bnRpbWUvSlNDSlNWYWx1ZS5jcHAKaW5kZXggNTQ4MDEwZWIxYzA3MDE2MmZkMGM5NjIzMjZmZWUy OWIxMTAzZDMzZi4uZTU5MGMxYTI5MGMwZmE4YjVjZDBkNWRjN2Y4MWI4YmQ0N2NmYzA2MSAxMDA2 NDQKLS0tIGEvU291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvSlNDSlNWYWx1ZS5jcHAKKysr IGIvU291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvSlNDSlNWYWx1ZS5jcHAKQEAgLTQyNyw0 ICs0MjcsMTEgQEAgU3RyaW5nIEpTVmFsdWU6OnRvV1RGU3RyaW5nU2xvd0Nhc2UoSlNHbG9iYWxP YmplY3QqIGdsb2JhbE9iamVjdCkgY29uc3QKICAgICBSRUxFQVNFX0FORF9SRVRVUk4oc2NvcGUs IHN0cmluZy0+dmFsdWUoZ2xvYmFsT2JqZWN0KSk7CiB9CiAKKyNpZiAhQ09NUElMRVIoR0NDX0NP TVBBVElCTEUpCisvLyBUaGlzIG1ha2VzIHRoZSBhcmd1bWVudCBvcGFxdWUgZnJvbSB0aGUgY29t cGlsZXIuCitORVZFUl9JTkxJTkUgdm9pZCBrZWVwQWxpdmUoSlNWYWx1ZSkKK3sKK30KKyNlbmRp ZgorCiB9IC8vIG5hbWVzcGFjZSBKU0MKZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29y ZS9ydW50aW1lL0pTQ0pTVmFsdWUuaCBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL0pT Q0pTVmFsdWUuaAppbmRleCA1ODZhZjNmYWIzNzk1MmUzZmRlMWNlNDcwYzk5YWIxZWRjNGE3NDFl Li5mYjA2ZjFmMmIwNjkwMmE2NmE2YThjZjRjZjI0YjIxNTE2MmU0NTI4IDEwMDY0NAotLS0gYS9T b3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9KU0NKU1ZhbHVlLmgKKysrIGIvU291cmNlL0ph dmFTY3JpcHRDb3JlL3J1bnRpbWUvSlNDSlNWYWx1ZS5oCkBAIC02MzcsNCArNjM3LDE3IEBAIGJv b2wgaXNUaGlzVmFsdWVBbHRlcmVkKGNvbnN0IFB1dFByb3BlcnR5U2xvdCYsIEpTT2JqZWN0KiBi YXNlT2JqZWN0KTsKIC8vIFNlZSBzZWN0aW9uIDcuMi45OiBodHRwczovL3RjMzkuZ2l0aHViLmlv L2VjbWEyNjIvI3NlYy1zYW1ldmFsdWUKIGJvb2wgc2FtZVZhbHVlKEpTR2xvYmFsT2JqZWN0Kiwg SlNWYWx1ZSBhLCBKU1ZhbHVlIGIpOwogCisjaWYgQ09NUElMRVIoR0NDX0NPTVBBVElCTEUpCitB TFdBWVNfSU5MSU5FIHZvaWQga2VlcEFsaXZlKEpTVmFsdWUgdmFsdWUpCit7CisjaWYgVVNFKEpT VkFMVUU2NCkKKyAgICBhc20gdm9sYXRpbGUgKCIiIDogOiAiciIoYml0d2lzZV9jYXN0PHVpbnQ2 NF90Pih2YWx1ZSkpIDogIm1lbW9yeSIpOworI2Vsc2UKKyAgICBhc20gdm9sYXRpbGUgKCIiIDog OiAiciIodmFsdWUucGF5bG9hZCgpKSA6ICJtZW1vcnkiKTsKKyNlbmRpZgorfQorI2Vsc2UKK0pT X0VYUE9SVF9QUklWQVRFIHZvaWQga2VlcEFsaXZlKEpTVmFsdWUpOworI2VuZGlmCisKIH0gLy8g bmFtZXNwYWNlIEpTQwo=