Bug 185218

Summary: Strings should not be allocated in a gigacage
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: bmallocAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, beidson, benjamin, cdumez, cmarcelo, dbates, esprehn+autocc, ews-watchlist, ggaren, gyuyoung.kim, jsbell, keith_miller, mark.lam, msaboff, saam, simon.fraser, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=174924
Bug Depends on:    
Bug Blocks: 174917    
Attachments:
Description Flags
possible patch
none
the patch
none
the patch
saam: review+
patch for landing none

Filip Pizlo
Reported 2018-05-02 14:48:03 PDT
Caging strings only protects us from string-based read gadgets. It's also a slow-down on some benchmarks and a memory use regression. The benefit/cost isn't big enough to keep this around so we should go back to allocating strings the normal way.
Attachments
possible patch (85.74 KB, patch)
2018-05-02 14:49 PDT, Filip Pizlo 		no flags
DetailsFormatted DiffDiff
the patch (97.26 KB, patch)
2018-05-03 12:08 PDT, Filip Pizlo 		no flags
DetailsFormatted DiffDiff
the patch (97.89 KB, patch)
2018-05-03 14:23 PDT, Filip Pizlo 		saam: review+
DetailsFormatted DiffDiff
patch for landing (97.93 KB, patch)
2018-05-03 14:50 PDT, Filip Pizlo 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Filip Pizlo
Comment 1 2018-05-02 14:49:12 PDT
Created attachment 339347 [details] possible patch
Filip Pizlo
Comment 2 2018-05-03 12:08:34 PDT
Created attachment 339437 [details] the patch
Filip Pizlo
Comment 3 2018-05-03 14:23:10 PDT
Created attachment 339465 [details] the patch
Saam Barati
Comment 4 2018-05-03 14:44:45 PDT
Comment on attachment 339465 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=339465&action=review > Source/bmalloc/bmalloc/Gigacage.h:38 > #define PRIMITIVE_GIGACAGE_SIZE 0x80000000llu > #define JSVALUE_GIGACAGE_SIZE 0x40000000llu Might be worth expanding one (or both) of these
Filip Pizlo
Comment 5 2018-05-03 14:50:01 PDT
Created attachment 339470 [details] patch for landing
Filip Pizlo
Comment 6 2018-05-03 17:38:52 PDT
(In reply to Saam Barati from comment #4) > Comment on attachment 339465 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=339465&action=review > > > Source/bmalloc/bmalloc/Gigacage.h:38 > > #define PRIMITIVE_GIGACAGE_SIZE 0x80000000llu > > #define JSVALUE_GIGACAGE_SIZE 0x40000000llu > > Might be worth expanding one (or both) of these Yeah. Or give them separate runways.
Filip Pizlo
Comment 7 2018-05-03 17:40:41 PDT
Landed in https://trac.webkit.org/changeset/231337/webkit
Yusuke Suzuki
Comment 8 2018-05-03 18:50:52 PDT
Committed r231344: <https://trac.webkit.org/changeset/231344>
Radar WebKit Bug Importer
Comment 9 2018-05-03 18:51:23 PDT
<rdar://problem/39963637>
Note You need to log in before you can comment on or make changes to this bug.