228550 2021-07-28 06:10:35 -0700 LLInt get_by_id unset IC looks inadventently disabled 2021-08-04 06:11:17 -0700 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified NEW InRadar P2 Normal --- 1 angelos angelos ews-watchlist keith_miller mark.lam msaboff saam tzagallo webkit-bug-importer oldest_to_newest 1780033 0 angelos 2021-07-28 06:10:35 -0700 LLInt get_by_id IC looks inadventently disabled 1780034 1 434424 angelos 2021-07-28 06:11:05 -0700 Created attachment 434424 Patch 1780039 2 angelos 2021-07-28 06:16:13 -0700 This commit https://github.com/WebKit/WebKit/commit/22d3ec3ad0249d03d9a909c909fb1ccc246e1de5#diff-95ecbe05ccde2fa9490e1bac06bc5a159fabc13dc58aa4e7a33114fd95176d0aR775 appears to accidentally disable unsetMode for the IC in performLLIntGetByID. Specifically, we can only switch to unsetMode in setupGetByIdPrototypeCache, but the only call to setupGetByIdPrototypeCache is guarded by if (!LLINT_ALWAYS_ACCESS_SLOW && baseValue.isCell() && slot.isCacheable() && !slot.isUnset()) { in the body of performLLIntGetByID. 1780109 3 saam 2021-07-28 10:32:39 -0700 (In reply to Angelos Oikonomopoulos from comment #0) > LLInt get_by_id IC looks inadventently disabled The name of this bug seems off. I think you mean just proto loads. 1780357 4 angelos 2021-07-29 04:49:39 -0700 (In reply to Saam Barati from comment #3) > (In reply to Angelos Oikonomopoulos from comment #0) > > LLInt get_by_id IC looks inadventently disabled > > The name of this bug seems off. I think you mean just proto loads. I meant unsetMode. Updated the name. 1780460 5 434424 saam 2021-07-29 11:20:24 -0700 Comment on attachment 434424 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434424&action=review > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:775 > - && slot.isCacheable() > - && !slot.isUnset()) { > + && slot.isCacheable()) { I don't see how this is doing anything. In the below code, we only check for isValue before doing proto loads. What am I missing? 1780728 6 angelos 2021-07-30 02:16:11 -0700 (In reply to Saam Barati from comment #5) > Comment on attachment 434424 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=434424&action=review > > > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:775 > > - && slot.isCacheable() > > - && !slot.isUnset()) { > > + && slot.isCacheable()) { > > I don't see how this is doing anything. In the below code, we only check for > isValue before doing proto loads. What am I missing? Going by your use of 'only', what you're missing may be that } else if (UNLIKELY(metadata.hitCountForLLIntCaching && slot.isValue())) { is nested within if (!LLINT_ALWAYS_ACCESS_SLOW && baseValue.isCell() && slot.isCacheable() && !slot.isUnset()) { ? 1780792 7 saam 2021-07-30 08:25:04 -0700 (In reply to Angelos Oikonomopoulos from comment #6) > (In reply to Saam Barati from comment #5) > > Comment on attachment 434424 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=434424&action=review > > > > > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:775 > > > - && slot.isCacheable() > > > - && !slot.isUnset()) { > > > + && slot.isCacheable()) { > > > > I don't see how this is doing anything. In the below code, we only check for > > isValue before doing proto loads. What am I missing? > > Going by your use of 'only', what you're missing may be that > > } else if (UNLIKELY(metadata.hitCountForLLIntCaching && slot.isValue())) { > > is nested within > > if (!LLINT_ALWAYS_ACCESS_SLOW > && baseValue.isCell() > && slot.isCacheable() > && !slot.isUnset()) { > > ? slot.isValue() implies !slot.isUnset(). You can't be both a value and unset. So I don't see how removing !slot.isUnset() changes anything to be different in the proto load case, since we check slot.isValue() 1780800 8 angelos 2021-07-30 08:29:20 -0700 Fair enough, the patch is a no-op then. But AFAICT unsetMode remains effectively disabled, since the only call to setupGetByIdPrototypeCache is guarded by the slot.isValue() check. 1780906 9 saam 2021-07-30 12:28:15 -0700 (In reply to Angelos Oikonomopoulos from comment #8) > Fair enough, the patch is a no-op then. But AFAICT unsetMode remains > effectively disabled, since the only call to setupGetByIdPrototypeCache is > guarded by the slot.isValue() check. Right. Maybe that call to setupGetByIdPrototypeCache shouldn't be guarded by isValue()? 1780907 10 saam 2021-07-30 12:30:30 -0700 Clearing "r?" for now 1781872 11 webkit-bug-importer 2021-08-04 06:11:17 -0700 <rdar://problem/81511696> 434424 2021-07-28 06:11:05 -0700 2021-07-30 12:30:38 -0700 Patch bug-228550-20210728131103.patch text/plain 1422 angelos U3VidmVyc2lvbiBSZXZpc2lvbjogMjgwMzgwCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCAy ZGQ5ZDNmNTYxNTQ1ZjhjNWU4NzdjMjQwYjQ4NGFjNzAzYWIyZjlkLi4zZmFjOWI4Y2Y0MmIzMzk5 MmIyNDExZTRjNjU0MzEzNDc4YzAxOTQ4IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxMyBAQAorMjAyMS0wNy0yOCAgQW5nZWxvcyBPaWtvbm9tb3BvdWxvcyAgPGFuZ2Vsb3NA aWdhbGlhLmNvbT4KKworICAgICAgICBMTEludCBnZXRfYnlfaWQgSUMgbG9va3MgaW5hZHZlbnRl bnRseSBkaXNhYmxlZAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5j Z2k/aWQ9MjI4NTUwCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAg ICAgICAgKiBsbGludC9MTEludFNsb3dQYXRocy5jcHA6CisgICAgICAgIChKU0M6OkxMSW50Ojpw ZXJmb3JtTExJbnRHZXRCeUlEKToKKwogMjAyMS0wNy0yNyAgUGF0cmljayBBbmdsZSAgPHBhbmds ZUBhcHBsZS5jb20+CiAKICAgICAgICAgV2ViIEluc3BlY3RvcjogW0NvY29hXSAiUmVtb3RlSW5z cGVjdG9yIFhQQyBjb25uZWN0aW9uIHRvIHJlbGF5IGZhaWxlZC4iIG1lc3NhZ2VzIGFyZSBjb25m dXNpbmcgaW4gU3RkRXJyCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvbGxpbnQv TExJbnRTbG93UGF0aHMuY3BwIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL2xsaW50L0xMSW50U2xv d1BhdGhzLmNwcAppbmRleCA1NjE1NzQ0MjM2MTYyMjczZGNlZTBmNTA4MTQzMzIwMmZiNzc0OWIx Li4xOGVmYjBlNjA0Y2YzNWI2NTc4MDYxZjIxZTYxNWRjMDU4NTJjMmY1IDEwMDY0NAotLS0gYS9T b3VyY2UvSmF2YVNjcmlwdENvcmUvbGxpbnQvTExJbnRTbG93UGF0aHMuY3BwCisrKyBiL1NvdXJj ZS9KYXZhU2NyaXB0Q29yZS9sbGludC9MTEludFNsb3dQYXRocy5jcHAKQEAgLTc3Miw4ICs3NzIs NyBAQCBzdGF0aWMgSlNWYWx1ZSBwZXJmb3JtTExJbnRHZXRCeUlEKGNvbnN0IEluc3RydWN0aW9u KiBwYywgQ29kZUJsb2NrKiBjb2RlQmxvY2ssCiAKICAgICBpZiAoIUxMSU5UX0FMV0FZU19BQ0NF U1NfU0xPVwogICAgICAgICAmJiBiYXNlVmFsdWUuaXNDZWxsKCkKLSAgICAgICAgJiYgc2xvdC5p c0NhY2hlYWJsZSgpCi0gICAgICAgICYmICFzbG90LmlzVW5zZXQoKSkgeworICAgICAgICAmJiBz bG90LmlzQ2FjaGVhYmxlKCkpIHsKICAgICAgICAgewogICAgICAgICAgICAgU3RydWN0dXJlSUQg b2xkU3RydWN0dXJlSUQ7CiAgICAgICAgICAgICBzd2l0Y2ggKG1ldGFkYXRhLm1vZGUpIHsK