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.
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
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
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
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.
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 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.
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
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
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
(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.
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.
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
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.
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...
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.
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.
(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.
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.
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.
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.
(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 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?
(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.
(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.
Created attachment 335975[details]
patch for landing (correct patch without changes to IsoHeap algorithm, but with a runtime disable option (bmalloc_IsoHeap=0))
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.
Created attachment 335988[details]
patch for landing (correct patch without changes to IsoHeap algorithm, but with a runtime disable option (bmalloc_IsoHeap=0))
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.
2018-03-10 17:51 PST, Filip Pizlo
2018-03-10 18:17 PST, Filip Pizlo
2018-03-10 18:36 PST, Filip Pizlo
2018-03-10 20:18 PST, Filip Pizlo
2018-03-10 20:55 PST, Filip Pizlo
2018-03-10 22:02 PST, Filip Pizlo
2018-03-11 10:14 PDT, Filip Pizlo
2018-03-11 13:05 PDT, Filip Pizlo
2018-03-11 13:38 PDT, Filip Pizlo
2018-03-11 14:41 PDT, Filip Pizlo
2018-03-11 15:41 PDT, Filip Pizlo
2018-03-11 19:14 PDT, Filip Pizlo
2018-03-11 20:08 PDT, Filip Pizlo
2018-03-11 21:54 PDT, Filip Pizlo
2018-03-12 11:02 PDT, Filip Pizlo
2018-03-12 11:59 PDT, EWS Watchlist
2018-03-12 12:01 PDT, EWS Watchlist
2018-03-12 12:08 PDT, EWS Watchlist
2018-03-12 12:08 PDT, Filip Pizlo
2018-03-12 12:19 PDT, Filip Pizlo
ews-watchlist: commit-queue-
2018-03-12 13:08 PDT, EWS Watchlist
2018-03-12 13:16 PDT, EWS Watchlist
2018-03-12 13:26 PDT, EWS Watchlist
2018-03-12 13:52 PDT, Filip Pizlo
2018-03-12 14:59 PDT, EWS Watchlist
2018-03-12 15:09 PDT, Filip Pizlo
2018-03-13 13:56 PDT, Filip Pizlo
2018-03-13 15:46 PDT, Filip Pizlo
2018-03-13 16:12 PDT, Filip Pizlo
2018-03-13 17:06 PDT, Filip Pizlo
2018-03-13 22:59 PDT, Filip Pizlo
2018-03-14 10:42 PDT, Filip Pizlo
2018-03-14 14:12 PDT, Filip Pizlo
2018-03-14 19:57 PDT, Filip Pizlo
2018-03-16 14:33 PDT, Filip Pizlo
2018-03-16 15:14 PDT, Filip Pizlo
2018-03-16 18:02 PDT, Filip Pizlo