Bug 183546 - Put the DOM in IsoHeaps
Summary: Put the DOM in IsoHeaps
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-03-10 17:51 PST by Filip Pizlo
Modified: 2018-03-16 23:12 PDT (History)
12 users (show)

See Also:


Attachments
it begins! (23.94 KB, patch)
2018-03-10 17:51 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
a bit more (54.10 KB, patch)
2018-03-10 18:17 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
more (63.90 KB, patch)
2018-03-10 18:36 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
so much html (90.15 KB, patch)
2018-03-10 20:18 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
moving onto mathml (111.75 KB, patch)
2018-03-10 20:55 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
still have to do svg (132.25 KB, patch)
2018-03-10 22:02 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
started on svg (151.89 KB, patch)
2018-03-11 10:14 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
svg is so big (173.71 KB, patch)
2018-03-11 13:05 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
more (196.26 KB, patch)
2018-03-11 13:38 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
more (212.97 KB, patch)
2018-03-11 14:41 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
a bit more (224.25 KB, patch)
2018-03-11 15:41 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
more svg (264.60 KB, patch)
2018-03-11 19:14 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
maybe done (283.67 KB, patch)
2018-03-11 20:08 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
seems to compile (285.18 KB, patch)
2018-03-11 21:54 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (307.93 KB, patch)
2018-03-12 11:02 PDT, Filip Pizlo
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-sierra (520.00 KB, application/zip)
2018-03-12 11:59 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews107 for mac-sierra-wk2 (562.59 KB, application/zip)
2018-03-12 12:01 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews112 for mac-sierra (533.35 KB, application/zip)
2018-03-12 12:08 PDT, EWS Watchlist
no flags Details
the patch (311.47 KB, patch)
2018-03-12 12:08 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (311.47 KB, patch)
2018-03-12 12:19 PDT, Filip Pizlo
dbates: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-sierra-wk2 (1.21 MB, application/zip)
2018-03-12 13:08 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews101 for mac-sierra (612.59 KB, application/zip)
2018-03-12 13:16 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews115 for mac-sierra (665.57 KB, application/zip)
2018-03-12 13:26 PDT, EWS Watchlist
no flags Details
patch for landing (312.50 KB, patch)
2018-03-12 13:52 PDT, Filip Pizlo
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.56 MB, application/zip)
2018-03-12 14:59 PDT, EWS Watchlist
no flags Details
correct patch without changes to IsoHeap algorithm (313.78 KB, patch)
2018-03-12 15:09 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
work in progress for IsoLine allocator (348.37 KB, patch)
2018-03-13 13:56 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
fully written line allocation optimization (359.65 KB, patch)
2018-03-13 15:46 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
line allocation with changelog (359.65 KB, patch)
2018-03-13 16:12 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
starting to compile line allocation (386.03 KB, patch)
2018-03-13 17:06 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
line allocation compiles (392.76 KB, patch)
2018-03-13 22:59 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
line allocation passes testbmalloc (393.94 KB, patch)
2018-03-14 10:42 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
basic browsing seems to work with line allocation (397.39 KB, patch)
2018-03-14 14:12 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
line allocation is neutral on membuster (401.18 KB, patch)
2018-03-14 19:57 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
correct patch without changes to IsoHeap algorithm, but with a runtime disable option (bmalloc_IsoHeap=0) (319.37 KB, patch)
2018-03-16 14:33 PDT, Filip Pizlo
simon.fraser: review+
Details | Formatted Diff | Diff
patch for landing (correct patch without changes to IsoHeap algorithm, but with a runtime disable option (bmalloc_IsoHeap=0)) (319.92 KB, patch)
2018-03-16 15:14 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
patch for landing (correct patch without changes to IsoHeap algorithm, but with a runtime disable option (bmalloc_IsoHeap=0)) (320.69 KB, patch)
2018-03-16 18:02 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-03-10 17:51:21 PST
Patch forthcoming.
Comment 1 Filip Pizlo 2018-03-10 17:51:50 PST
Created attachment 335522 [details]
it begins!
Comment 2 Filip Pizlo 2018-03-10 18:17:56 PST
Created attachment 335526 [details]
a bit more
Comment 3 Filip Pizlo 2018-03-10 18:36:10 PST
Created attachment 335530 [details]
more

so much stuff
Comment 4 Filip Pizlo 2018-03-10 20:18:00 PST
Created attachment 335531 [details]
so much html
Comment 5 Filip Pizlo 2018-03-10 20:55:54 PST
Created attachment 335532 [details]
moving onto mathml
Comment 6 Filip Pizlo 2018-03-10 22:02:55 PST
Created attachment 335533 [details]
still have to do svg
Comment 7 Filip Pizlo 2018-03-11 10:14:58 PDT
Created attachment 335538 [details]
started on svg
Comment 8 Filip Pizlo 2018-03-11 13:05:33 PDT
Created attachment 335542 [details]
svg is so big

I'm somewhere through it
Comment 9 Filip Pizlo 2018-03-11 13:38:54 PDT
Created attachment 335544 [details]
more
Comment 10 Filip Pizlo 2018-03-11 14:41:00 PDT
Created attachment 335546 [details]
more
Comment 11 Filip Pizlo 2018-03-11 15:41:59 PDT
Created attachment 335550 [details]
a bit more
Comment 12 Filip Pizlo 2018-03-11 19:14:44 PDT
Created attachment 335561 [details]
more svg

it has so many classes
Comment 13 Filip Pizlo 2018-03-11 20:08:23 PDT
Created attachment 335571 [details]
maybe done
Comment 14 Filip Pizlo 2018-03-11 21:54:09 PDT
Created attachment 335582 [details]
seems to compile
Comment 15 Filip Pizlo 2018-03-12 11:02:06 PDT
Created attachment 335606 [details]
the patch

still testing it, but it's ready for a rubber stamp ;-)
Comment 16 EWS Watchlist 2018-03-12 11:04:45 PDT
Attachment 335606 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:11:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: UXSS  [changelog/unwantedsecurityterms] [3]
Total errors found: 1 in 456 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 EWS Watchlist 2018-03-12 11:59:23 PDT
Comment on attachment 335606 [details]
the patch

Attachment 335606 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6917864

Number of test failures exceeded the failure limit.
Comment 18 EWS Watchlist 2018-03-12 11:59:25 PDT
Created attachment 335616 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 19 EWS Watchlist 2018-03-12 12:00:59 PDT
Comment on attachment 335606 [details]
the patch

Attachment 335606 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6917859

Number of test failures exceeded the failure limit.
Comment 20 EWS Watchlist 2018-03-12 12:01:02 PDT
Created attachment 335617 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 21 EWS Watchlist 2018-03-12 12:08:34 PDT
Comment on attachment 335606 [details]
the patch

Attachment 335606 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6917831

Number of test failures exceeded the failure limit.
Comment 22 EWS Watchlist 2018-03-12 12:08:35 PDT
Created attachment 335618 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 23 Filip Pizlo 2018-03-12 12:08:45 PDT
Created attachment 335619 [details]
the patch

Fixed HTMLMediaElement.
Comment 24 EWS Watchlist 2018-03-12 12:11:34 PDT
Attachment 335619 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:11:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: UXSS  [changelog/unwantedsecurityterms] [3]
Total errors found: 1 in 462 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 25 Filip Pizlo 2018-03-12 12:19:25 PDT
Created attachment 335622 [details]
the patch
Comment 26 EWS Watchlist 2018-03-12 12:22:40 PDT
Attachment 335622 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:11:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: UXSS  [changelog/unwantedsecurityterms] [3]
Total errors found: 1 in 462 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 27 Daniel Bates 2018-03-12 12:28:37 PDT
Comment on attachment 335622 [details]
the patch

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

Looks sane to me.

r=me

> Source/WebCore/dom/TemplateContentDocumentFragment.cpp:35
> +} // namespace WebCore

This is OK as-is. We tend to omit this inline comment when the source code for the entire file fits easily on any modern desktop monitor. If you do decide to remove this comment then please remove it from all the other added files in this patch.
Comment 28 EWS Watchlist 2018-03-12 13:08:31 PDT
Comment on attachment 335622 [details]
the patch

Attachment 335622 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6919628

Number of test failures exceeded the failure limit.
Comment 29 EWS Watchlist 2018-03-12 13:08:33 PDT
Created attachment 335629 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 30 EWS Watchlist 2018-03-12 13:16:56 PDT
Comment on attachment 335622 [details]
the patch

Attachment 335622 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6919944

Number of test failures exceeded the failure limit.
Comment 31 EWS Watchlist 2018-03-12 13:16:58 PDT
Created attachment 335630 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 32 EWS Watchlist 2018-03-12 13:26:25 PDT
Comment on attachment 335622 [details]
the patch

Attachment 335622 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6919934

Number of test failures exceeded the failure limit.
Comment 33 EWS Watchlist 2018-03-12 13:26:26 PDT
Created attachment 335631 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 34 Filip Pizlo 2018-03-12 13:27:00 PDT
(In reply to Build Bot from comment #33)
> Created attachment 335631 [details]
> Archive of layout-test-results from ews115 for mac-sierra
> 
> The attached test failures were seen while running run-webkit-tests on the
> mac-debug-ews.
> Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6

Looks like I forgot Element this time.
Comment 35 Filip Pizlo 2018-03-12 13:52:35 PDT
Created attachment 335634 [details]
patch for landing

Putting on EWS to see if I missed any other types.
Comment 36 EWS Watchlist 2018-03-12 13:56:23 PDT
Attachment 335634 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:11:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: UXSS  [changelog/unwantedsecurityterms] [3]
Total errors found: 1 in 464 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 37 EWS Watchlist 2018-03-12 14:59:23 PDT
Comment on attachment 335634 [details]
patch for landing

Attachment 335634 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6921768

Number of test failures exceeded the failure limit.
Comment 38 EWS Watchlist 2018-03-12 14:59:24 PDT
Created attachment 335648 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 39 Filip Pizlo 2018-03-12 15:09:29 PDT
Created attachment 335649 [details]
correct patch without changes to IsoHeap algorithm

Fixing more things.
Comment 40 EWS Watchlist 2018-03-12 15:13:16 PDT
Attachment 335649 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:11:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: UXSS  [changelog/unwantedsecurityterms] [3]
Total errors found: 1 in 466 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 41 Filip Pizlo 2018-03-12 19:11:49 PDT
Looks like I have some builds to fix. But before I do that, I want to run more benchmarks.

So far, I know that Speedometer is neutral.

I'm running membuster next...
Comment 42 Filip Pizlo 2018-03-12 21:49:09 PDT
membuster shows 1-2% for each of the snapshots, with mediocre statistical confidence.

I'm going to investigate ways of reducing this overhead.
Comment 43 Filip Pizlo 2018-03-13 10:53:52 PDT
I'm going to make a gamble about what is going on:

The low, near-noise overhead implies that IsoHeaps aren't causing run-away fragmentation of any kind. I think we're only seeing the ~16KB-per-type overhead due to the fact that the DOM has so freaking many types.

There are two ways to attack this.

The first is to just coalesce so heaps. There are a bunch of parts of the DOM hierarchy where the subclass does not add any properties. In some cases, it makes strong type assumptions about some dynamic field in the parent. But that doesn't appear to always be the case. In those cases, the subclass could just inherit the superclass's heap. However, I'd rather not go in this direction, because this feels like it could be annoying to maintain. OTOH, even with other optimizations, this is likely to be profitable for perf and memory.

The second is to improve the IsoHeap design so that heaps containing just a small number of objects never allocate a whole page. For each size class, we could have a set of shared pages out of which we allocate memory for mini heaps. Within those shared pages, we can still preserve the invariant that once a virtual address is used for a type, it will never be used for any other type.
Comment 44 Filip Pizlo 2018-03-13 13:56:14 PDT
Created attachment 335724 [details]
work in progress for IsoLine allocator

The space overhead is probably because of types that we only allocate a few instances of.  Currently, that means having a whole page for those types.

This introduces a "baseline" IsoHeap allocator that uses a virtual IsoLine within an IsoLinePage.  I'm still working on it, but this will mean that:

- Heaps that get used sparingly will not allocate a full page.  They will use a first-fit bitvector allocator inside an IsoLine.  The whole thing has minimal space overhead.

- If you allocate more than N objects per millisecond, you tier-up to a full heap.

- If you run out of space in the IsoLine, you tier-up to a full heap.

- A tiered-up heap will still keep the IsoLine as the first place to allocate out of.  This preserves first-fit behavior.
Comment 45 Filip Pizlo 2018-03-13 15:46:03 PDT
Created attachment 335739 [details]
fully written line allocation optimization

I do not look forward to testing the things that I wrote.
Comment 46 Filip Pizlo 2018-03-13 16:12:46 PDT
Created attachment 335742 [details]
line allocation with changelog
Comment 47 Filip Pizlo 2018-03-13 17:06:17 PDT
Created attachment 335747 [details]
starting to compile line allocation
Comment 48 Filip Pizlo 2018-03-13 22:59:41 PDT
Created attachment 335761 [details]
line allocation compiles
Comment 49 Filip Pizlo 2018-03-14 10:42:45 PDT
Created attachment 335781 [details]
line allocation passes testbmalloc
Comment 50 Filip Pizlo 2018-03-14 10:42:58 PDT
(In reply to Filip Pizlo from comment #49)
> Created attachment 335781 [details]
> line allocation passes testbmalloc

Now I'm going to have to write some more tests.
Comment 51 Filip Pizlo 2018-03-14 14:12:46 PDT
Created attachment 335798 [details]
basic browsing seems to work with line allocation
Comment 52 EWS Watchlist 2018-03-14 14:17:03 PDT
Attachment 335798 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/DeferredDecommit.h:35:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/DeferredDecommit.h:35:  The parameter name "page" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoLinePageHandle.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/ChangeLog:11:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: UXSS  [changelog/unwantedsecurityterms] [3]
Total errors found: 4 in 503 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 53 Filip Pizlo 2018-03-14 19:57:39 PDT
Created attachment 335824 [details]
line allocation is neutral on membuster
Comment 54 Filip Pizlo 2018-03-14 19:59:32 PDT
Can someone review the bmalloc part of this change?

Dan reviewed the WebCore part, but since then, I made bmalloc changes to get membuster to be neutral.
Comment 55 EWS Watchlist 2018-03-14 20:02:08 PDT
Attachment 335824 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/DeferredDecommit.h:35:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/DeferredDecommit.h:35:  The parameter name "page" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoLinePageHandle.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoStats.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/ChangeLog:11:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: UXSS  [changelog/unwantedsecurityterms] [3]
Total errors found: 5 in 505 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 56 Filip Pizlo 2018-03-15 09:18:32 PDT
Comment on attachment 335824 [details]
line allocation is neutral on membuster

Not ready for review. :-(
Comment 57 Filip Pizlo 2018-03-15 09:19:33 PDT
It appears that the line allocation is a definite slow-down on Speedometer: 1.6% slower with p = 0.00002.

I'm going to have to investigate that.
Comment 58 Filip Pizlo 2018-03-16 14:31:31 PDT
(In reply to Filip Pizlo from comment #57)
> It appears that the line allocation is a definite slow-down on Speedometer:
> 1.6% slower with p = 0.00002.
> 
> I'm going to have to investigate that.

I did more experiments.  I'm not sure that the Speedometer numbers are right.  I did more testing and was unable to see a slow-down.  Also, more tests show that membuster isn't actually regressed.

Based on this, I think I should land the version that does not add line allocation.  We can keep that in our back pockets to try if the regression is real, but right now, I don't think it is.

I will make a version of the patch that adds runtime disabling of IsoHeaps.  I think we should land that.
Comment 59 Filip Pizlo 2018-03-16 14:33:44 PDT
Created attachment 335969 [details]
correct patch without changes to IsoHeap algorithm, but with a runtime disable option (bmalloc_IsoHeap=0)
Comment 60 Simon Fraser (smfr) 2018-03-16 14:37:18 PDT
Comment on attachment 335969 [details]
correct patch without changes to IsoHeap algorithm, but with a runtime disable option (bmalloc_IsoHeap=0)

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

> Source/bmalloc/bmalloc/IsoTLS.cpp:205
> +    const char* env = getenv("bmalloc_IsoHeap");
> +    if (env && (!strcasecmp(env, "false") || !strcasecmp(env, "no") || !strcmp(env, "0")))
> +        s_mallocFallbackState = MallocFallbackState::FallBackToMalloc;
> +    else
> +        s_mallocFallbackState = MallocFallbackState::DoNotFallBack;

Should this be using dispatch_once or whatever?
Comment 61 Filip Pizlo 2018-03-16 14:50:52 PDT
(In reply to Simon Fraser (smfr) from comment #60)
> Comment on attachment 335969 [details]
> correct patch without changes to IsoHeap algorithm, but with a runtime
> disable option (bmalloc_IsoHeap=0)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335969&action=review
> 
> > Source/bmalloc/bmalloc/IsoTLS.cpp:205
> > +    const char* env = getenv("bmalloc_IsoHeap");
> > +    if (env && (!strcasecmp(env, "false") || !strcasecmp(env, "no") || !strcmp(env, "0")))
> > +        s_mallocFallbackState = MallocFallbackState::FallBackToMalloc;
> > +    else
> > +        s_mallocFallbackState = MallocFallbackState::DoNotFallBack;
> 
> Should this be using dispatch_once or whatever?

I don't think it needs to be, since if multiple threads initialize this, they will all initialize it to the same thing.
Comment 62 Filip Pizlo 2018-03-16 14:54:08 PDT
(In reply to Filip Pizlo from comment #61)
> (In reply to Simon Fraser (smfr) from comment #60)
> > Comment on attachment 335969 [details]
> > correct patch without changes to IsoHeap algorithm, but with a runtime
> > disable option (bmalloc_IsoHeap=0)
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=335969&action=review
> > 
> > > Source/bmalloc/bmalloc/IsoTLS.cpp:205
> > > +    const char* env = getenv("bmalloc_IsoHeap");
> > > +    if (env && (!strcasecmp(env, "false") || !strcasecmp(env, "no") || !strcmp(env, "0")))
> > > +        s_mallocFallbackState = MallocFallbackState::FallBackToMalloc;
> > > +    else
> > > +        s_mallocFallbackState = MallocFallbackState::DoNotFallBack;
> > 
> > Should this be using dispatch_once or whatever?
> 
> I don't think it needs to be, since if multiple threads initialize this,
> they will all initialize it to the same thing.

OTOH - there is zero harm in putting it in a std::call_once.  I'll do that.
Comment 63 Filip Pizlo 2018-03-16 15:14:57 PDT
Created attachment 335975 [details]
patch for landing (correct patch without changes to IsoHeap algorithm, but with a runtime disable option (bmalloc_IsoHeap=0))
Comment 64 EWS Watchlist 2018-03-16 15:36:02 PDT
Attachment 335975 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:11:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: UXSS  [changelog/unwantedsecurityterms] [3]
Total errors found: 1 in 471 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 65 Filip Pizlo 2018-03-16 18:02:03 PDT
Created attachment 335988 [details]
patch for landing (correct patch without changes to IsoHeap algorithm, but with a runtime disable option (bmalloc_IsoHeap=0))
Comment 66 EWS Watchlist 2018-03-16 18:05:09 PDT
Attachment 335988 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:11:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: UXSS  [changelog/unwantedsecurityterms] [3]
Total errors found: 1 in 472 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 67 Filip Pizlo 2018-03-16 23:11:52 PDT
Landed in https://trac.webkit.org/changeset/229694/webkit
Comment 68 Radar WebKit Bug Importer 2018-03-16 23:12:29 PDT
<rdar://problem/38572226>