Bug 179202 - Only cage double butterfly accesses
Summary: Only cage double butterfly accesses
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-11-02 14:55 PDT by Saam Barati
Modified: 2017-11-15 12:20 PST (History)
13 users (show)

See Also:


Attachments
patch (30.09 KB, patch)
2017-11-07 12:43 PST, Saam Barati
mark.lam: review+
Details | Formatted Diff | Diff
patch for landing (31.54 KB, patch)
2017-11-07 20:47 PST, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2017-11-02 14:55:39 PDT
...
Comment 1 Saam Barati 2017-11-07 12:43:47 PST
Created attachment 326240 [details]
patch
Comment 2 Mark Lam 2017-11-07 16:50:44 PST
Comment on attachment 326240 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=326240&action=review

r=me with issues addressed.

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1238
>  macro storePropertyAtVariableOffset(propertyOffsetAsInt, objectAndStorage, value, scratch)

It doesn't hurt to leave it but scratch is now unused.

> Source/JavaScriptCore/runtime/JSObject.cpp:3250
> +    return Butterfly::createOrGrowPropertyStorage(butterfly(), vm, this, structure(vm), oldSize, newSize);

Why is this case not caged?  I see that Butterfly::createOrGrowPropertyStorage() does mem copying.  Either Butterfly::createOrGrowPropertyStorage() should always cage the butterfly if not null, or it we should do a null check + caging here.
Comment 3 Saam Barati 2017-11-07 20:47:38 PST
Created attachment 326298 [details]
patch for landing

Thanks for the review. I've removed the scratch register and I've made Butterfly:createOrGrowPropertyStorage cage when oldButterfly is non-null.
Comment 4 WebKit Commit Bot 2017-11-07 22:29:34 PST
Comment on attachment 326298 [details]
patch for landing

Clearing flags on attachment: 326298

Committed r224564: <https://trac.webkit.org/changeset/224564>
Comment 5 WebKit Commit Bot 2017-11-07 22:29:36 PST
All reviewed patches have been landed.  Closing bug.
Comment 6 Yusuke Suzuki 2017-11-08 05:04:12 PST
Oh, this improves ARES-6 by 4.4% at AWFY.
Comment 7 Radar WebKit Bug Importer 2017-11-15 12:20:32 PST
<rdar://problem/35567338>