|
Description
Filip Pizlo
2018-03-10 17:51:21 PST
Created attachment 335522 [details]
it begins!
Created attachment 335526 [details]
a bit more
Created attachment 335530 [details]
more
so much stuff
Created attachment 335531 [details]
so much html
Created attachment 335532 [details]
moving onto mathml
Created attachment 335533 [details]
still have to do svg
Created attachment 335538 [details]
started on svg
Created attachment 335542 [details]
svg is so big
I'm somewhere through it
Created attachment 335544 [details]
more
Created attachment 335546 [details]
more
Created attachment 335550 [details]
a bit more
Created attachment 335561 [details]
more svg
it has so many classes
Created attachment 335571 [details]
maybe done
Created attachment 335582 [details]
seems to compile
Created attachment 335606 [details]
the patch
still testing it, but it's ready for a rubber stamp ;-)
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 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. 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 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. 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 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. 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
Created attachment 335619 [details]
the patch
Fixed HTMLMediaElement.
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.
Created attachment 335622 [details]
the patch
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. 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. 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 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. 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 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. 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. Created attachment 335634 [details]
patch for landing
Putting on EWS to see if I missed any other types.
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 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. 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
Created attachment 335649 [details]
correct patch without changes to IsoHeap algorithm
Fixing more things.
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... membuster shows 1-2% for each of the snapshots, with mediocre statistical confidence. I'm going to investigate ways of reducing this overhead. 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.
Created attachment 335739 [details]
fully written line allocation optimization
I do not look forward to testing the things that I wrote.
Created attachment 335742 [details]
line allocation with changelog
Created attachment 335747 [details]
starting to compile line allocation
Created attachment 335761 [details]
line allocation compiles
Created attachment 335781 [details]
line allocation passes testbmalloc
(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. Created attachment 335798 [details]
basic browsing seems to work with line allocation
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.
Created attachment 335824 [details]
line allocation is neutral on membuster
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.
Comment on attachment 335824 [details]
line allocation is neutral on membuster
Not ready for review. :-(
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. (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. Created attachment 335969 [details]
correct patch without changes to IsoHeap algorithm, but with a runtime disable option (bmalloc_IsoHeap=0)
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.
|