Summary: | Array.prototype.push should always perform [[Set]] in strict mode | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexey Shvayka <ashvayka> | ||||
Component: | JavaScriptCore | Assignee: | Alexey Shvayka <ashvayka> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Trivial | CC: | darin, ews-watchlist, keith_miller, mark.lam, msaboff, ross.kirsling, saam, tzagallo, webkit-bug-importer | ||||
Priority: | P2 | Keywords: | InRadar, WebExposed | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=211205 | ||||||
Attachments: |
|
Description
Alexey Shvayka
2020-09-03 04:58:55 PDT
Created attachment 407877 [details]
Patch
(In reply to Alexey Shvayka from comment #1) > Created attachment 407877 [details] > Patch Warmed-up runs, --outer 50: r266507 patch new-array-push 6.5605+-0.4641 6.2634+-0.4522 might be 1.0474x faster array-push-0 350.3329+-2.1429 348.3786+-1.2194 array-push-1 733.3222+-13.1726 ? 739.4494+-13.0772 ? might be 1.0084x slower array-push-2 822.0986+-4.4731 ? 822.2864+-3.1685 ? array-push-3 836.1861+-24.9409 832.5322+-23.3867 might be 1.0044x faster --- I've vetted all puts via PutPropertySlot in JSC: other usages set strict mode correctly. Do you understand why the JSC tests are failing on EWS? (In reply to Darin Adler from comment #3) > Do you understand why the JSC tests are failing on EWS? Igalia's servers seem to be down. I don't think it is related. EWS is all-green now. Thank you for review! Committed r266581: <https://trac.webkit.org/changeset/266581> All reviewed patches have been landed. Closing bug and clearing flags on attachment 407877 [details]. Comment on attachment 407877 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407877&action=review > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:919 > + thisObj->putByIndexInline(globalObject, static_cast<uint64_t>(length + n), callFrame->uncheckedArgument(n), true); Sorry I didn’t notice this when reviewing. Can we remove this unnecessary static_cast? It caused me concern just now: I was worried that the addition was incorrectly performed in 32 bits with possible overflow before promoting the result to 64 bits. But it’s not, because length is already uint64_t, and for the same reason the static_cast is not needed or helpful. Committed r266641: <https://trac.webkit.org/changeset/266641> (In reply to Darin Adler from comment #8) > Can we remove this unnecessary static_cast? It caused me concern just now: I > was worried that the addition was incorrectly performed in 32 bits with > possible overflow before promoting the result to 64 bits. But it’s not, > because length is already uint64_t, and for the same reason the static_cast > is not needed or helpful. Thank you for the follow-up! I've also checked another casts in ArrayPrototype.cpp, all seem necessary. |