Bug 185218 - Strings should not be allocated in a gigacage
Summary: Strings should not be allocated in a gigacage
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: bmalloc (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords: InRadar
Depends on:
Blocks: 174917
  Show dependency treegraph
 
Reported: 2018-05-02 14:48 PDT by Filip Pizlo
Modified: 2018-05-03 18:51 PDT (History)
18 users (show)

See Also:


Attachments
possible patch (85.74 KB, patch)
2018-05-02 14:49 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (97.26 KB, patch)
2018-05-03 12:08 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (97.89 KB, patch)
2018-05-03 14:23 PDT, Filip Pizlo
sbarati: review+
Details | Formatted Diff | Diff
patch for landing (97.93 KB, patch)
2018-05-03 14:50 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 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.
Comment 1 Filip Pizlo 2018-05-02 14:49:12 PDT
Created attachment 339347 [details]
possible patch
Comment 2 Filip Pizlo 2018-05-03 12:08:34 PDT
Created attachment 339437 [details]
the patch
Comment 3 Filip Pizlo 2018-05-03 14:23:10 PDT
Created attachment 339465 [details]
the patch
Comment 4 Saam Barati 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
Comment 5 Filip Pizlo 2018-05-03 14:50:01 PDT
Created attachment 339470 [details]
patch for landing
Comment 6 Filip Pizlo 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.
Comment 7 Filip Pizlo 2018-05-03 17:40:41 PDT
Landed in https://trac.webkit.org/changeset/231337/webkit
Comment 8 Yusuke Suzuki 2018-05-03 18:50:52 PDT
Committed r231344: <https://trac.webkit.org/changeset/231344>
Comment 9 Radar WebKit Bug Importer 2018-05-03 18:51:23 PDT
<rdar://problem/39963637>