Bug 183546 – Put the DOM in IsoHeaps

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 335522 [details]
it begins!

[Filip Pizlo]

Created attachment 335526 [details]
a bit more

[Filip Pizlo]

Created attachment 335530 [details]
more

so much stuff

[Filip Pizlo]

Created attachment 335531 [details]
so much html

[Filip Pizlo]

Created attachment 335532 [details]
moving onto mathml

[Filip Pizlo]

Created attachment 335533 [details]
still have to do svg

[Filip Pizlo]

Created attachment 335538 [details]
started on svg

[Filip Pizlo]

Created attachment 335542 [details]
svg is so big

I'm somewhere through it

[Filip Pizlo]

Created attachment 335544 [details]
more

[Filip Pizlo]

Created attachment 335546 [details]
more

[Filip Pizlo]

Created attachment 335550 [details]
a bit more

[Filip Pizlo]

Created attachment 335561 [details]
more svg

it has so many classes

[Filip Pizlo]

Created attachment 335571 [details]
maybe done

[Filip Pizlo]

Created attachment 335582 [details]
seems to compile

[Filip Pizlo]

Created attachment 335606 [details]
the patch

still testing it, but it's ready for a rubber stamp ;-)

[EWS Watchlist]

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.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Filip Pizlo]

Created attachment 335619 [details]
the patch

Fixed HTMLMediaElement.

[EWS Watchlist]

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.

[Filip Pizlo]

Created attachment 335622 [details]
the patch

[EWS Watchlist]

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.

[Daniel Bates]

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.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Filip Pizlo]

(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.

[Filip Pizlo]

Created attachment 335634 [details]
patch for landing

Putting on EWS to see if I missed any other types.

[EWS Watchlist]

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.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Filip Pizlo]

Created attachment 335649 [details]
correct patch without changes to IsoHeap algorithm

Fixing more things.

[EWS Watchlist]

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.

[Filip Pizlo]

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...

[Filip Pizlo]

membuster shows 1-2% for each of the snapshots, with mediocre statistical confidence.

I'm going to investigate ways of reducing this overhead.

[Filip Pizlo]

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.

[Filip Pizlo]

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.

[Filip Pizlo]

Created attachment 335739 [details]
fully written line allocation optimization

I do not look forward to testing the things that I wrote.

[Filip Pizlo]

Created attachment 335742 [details]
line allocation with changelog

[Filip Pizlo]

Created attachment 335747 [details]
starting to compile line allocation

[Filip Pizlo]

Created attachment 335761 [details]
line allocation compiles

[Filip Pizlo]

Created attachment 335781 [details]
line allocation passes testbmalloc

[Filip Pizlo]

(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.

[Filip Pizlo]

Created attachment 335798 [details]
basic browsing seems to work with line allocation

[EWS Watchlist]

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.

[Filip Pizlo]

Created attachment 335824 [details]
line allocation is neutral on membuster

[Filip Pizlo]

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.

[EWS Watchlist]

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.

[Filip Pizlo]

Comment on attachment 335824 [details]
line allocation is neutral on membuster

Not ready for review. :-(

[Filip Pizlo]

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.

[Filip Pizlo]

(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.

[Filip Pizlo]

Created attachment 335969 [details]
correct patch without changes to IsoHeap algorithm, but with a runtime disable option (bmalloc_IsoHeap=0)

[Simon Fraser (smfr)]

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?

[Filip Pizlo]

(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.

[Filip Pizlo]

(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.

[Filip Pizlo]

Created attachment 335975 [details]
patch for landing (correct patch without changes to IsoHeap algorithm, but with a runtime disable option (bmalloc_IsoHeap=0))

[EWS Watchlist]

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.

[Filip Pizlo]

Created attachment 335988 [details]
patch for landing (correct patch without changes to IsoHeap algorithm, but with a runtime disable option (bmalloc_IsoHeap=0))

[EWS Watchlist]

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.

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/229694/webkit

[Radar WebKit Bug Importer]

<rdar://problem/38572226>