Bug 178108 - bmalloc should support strictly type-segregated isolated heaps
Summary: bmalloc should support strictly type-segregated isolated heaps
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: 179278 180042
  Show dependency treegraph
 
Reported: 2017-10-09 18:56 PDT by Filip Pizlo
Modified: 2017-12-06 14:09 PST (History)
12 users (show)

See Also:


Attachments
it's a start (22.29 KB, patch)
2017-10-09 18:57 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
a bit more (39.37 KB, patch)
2017-10-12 12:25 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
I think I know what it will look like now (50.31 KB, patch)
2017-10-12 14:55 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
a bit more (61.07 KB, patch)
2017-10-12 16:30 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
more! (65.91 KB, patch)
2017-10-12 21:39 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
it's looking pretty serious (78.18 KB, patch)
2017-10-13 12:59 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
it's almost done (87.82 KB, patch)
2017-10-13 14:13 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
even more (99.37 KB, patch)
2017-10-13 19:02 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
a bit more (120.00 KB, patch)
2017-10-14 12:39 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
I think it's written (128.77 KB, patch)
2017-10-14 16:18 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
fixed up scavenger integration (135.00 KB, patch)
2017-10-15 14:30 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
wrote a test (154.64 KB, patch)
2017-10-16 11:28 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
starting to fix compile errors (181.40 KB, patch)
2017-10-16 13:01 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
closer to building (186.08 KB, patch)
2017-10-16 14:27 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
it compiles! (188.16 KB, patch)
2017-10-16 14:49 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
it passes a test! (190.68 KB, patch)
2017-10-16 15:53 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
scavenging seems to work! (193.80 KB, patch)
2017-10-16 20:44 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
passes easy tests, now expanding to renderobject (229.04 KB, patch)
2017-10-31 12:34 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
more (266.95 KB, patch)
2017-10-31 13:31 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
more! (289.83 KB, patch)
2017-10-31 13:56 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
it is written (365.58 KB, patch)
2017-10-31 19:48 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
it compiles! (370.02 KB, patch)
2017-11-02 12:34 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
fixed a bug (373.70 KB, patch)
2017-11-02 17:55 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
it sorta works (368.93 KB, patch)
2017-11-02 19:40 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
fix some builds (369.13 KB, patch)
2017-11-02 21:48 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
more build fixing (369.13 KB, patch)
2017-11-03 08:29 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
more fixes (369.85 KB, patch)
2017-11-03 14:01 PDT, Filip Pizlo
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews115 for mac-elcapitan (1.40 MB, application/zip)
2017-11-03 14:58 PDT, Build Bot
no flags Details
the patch (370.45 KB, patch)
2017-11-03 19:38 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (374.43 KB, patch)
2017-11-03 20:23 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (374.43 KB, patch)
2017-11-03 20:38 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (374.45 KB, patch)
2017-11-03 20:47 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (374.51 KB, patch)
2017-11-03 20:55 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (374.52 KB, patch)
2017-11-04 16:28 PDT, Filip Pizlo
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2017-10-09 18:56:04 PDT
Patch forthcoming.
Comment 1 Filip Pizlo 2017-10-09 18:57:05 PDT
Created attachment 323267 [details]
it's a start
Comment 2 Filip Pizlo 2017-10-12 12:25:49 PDT
Created attachment 323546 [details]
a bit more
Comment 3 Filip Pizlo 2017-10-12 14:55:48 PDT
Created attachment 323571 [details]
I think I know what it will look like now
Comment 4 Filip Pizlo 2017-10-12 16:30:22 PDT
Created attachment 323597 [details]
a bit more
Comment 5 Filip Pizlo 2017-10-12 21:39:22 PDT
Created attachment 323630 [details]
more!
Comment 6 Filip Pizlo 2017-10-13 12:59:43 PDT
Created attachment 323728 [details]
it's looking pretty serious
Comment 7 Filip Pizlo 2017-10-13 13:04:25 PDT
The basic architecture:

- Each heap is type-specialized for a size and instantiated on a per-type basis.
- Use type-specialization to fold as much size-oriented math as possible.
- Super dedicated first-fit discipline, so that we can maximize the amount of pages scavenged as well as their contiguity.
- Bump'n'pop allocator in each page. Bump'n'pop increases our ability to do first-fit, which increases our ability to free pages.
- Riptide-style bitvector tracking of pages.
- Bmalloc-style logging of deallocated objects. This amortizes lock acquisition.
- Single lock around each isoheap, which is good enough, since we acquire locks only on slow paths.
- Small isoheaps will only need to allocate one page (meta-data will be 100s of bytes stored in a bmalloc'd chunk).
- No limit on isoheap size. We do a unode-style expansion of the heap to be a linked list of "page directories" that allow for arbitrary sized heaps. The directories are bmalloced.
Comment 8 Filip Pizlo 2017-10-13 14:13:16 PDT
Created attachment 323745 [details]
it's almost done
Comment 9 Filip Pizlo 2017-10-13 19:02:15 PDT
Created attachment 323789 [details]
even more

I figured out an opportunity to make things a lot better in IsoTLS.  I'll do that, and then I think I'll be done.
Comment 10 Filip Pizlo 2017-10-14 12:39:54 PDT
Created attachment 323817 [details]
a bit more
Comment 11 Filip Pizlo 2017-10-14 16:18:08 PDT
Created attachment 323829 [details]
I think it's written
Comment 12 Filip Pizlo 2017-10-15 14:30:41 PDT
Created attachment 323849 [details]
fixed up scavenger integration
Comment 13 Filip Pizlo 2017-10-16 11:28:29 PDT
Created attachment 323913 [details]
wrote a test

Still haven't compiled it yet though.  I figured it would be pointless unless something included the *Inlines.h files.  So, I starting working on a test suite.
Comment 14 Filip Pizlo 2017-10-16 13:01:48 PDT
Created attachment 323931 [details]
starting to fix compile errors
Comment 15 Filip Pizlo 2017-10-16 14:27:06 PDT
Created attachment 323940 [details]
closer to building
Comment 16 Filip Pizlo 2017-10-16 14:49:50 PDT
Created attachment 323945 [details]
it compiles!
Comment 17 Filip Pizlo 2017-10-16 15:53:44 PDT
Created attachment 323950 [details]
it passes a test!
Comment 18 Filip Pizlo 2017-10-16 20:44:41 PDT
Created attachment 323983 [details]
scavenging seems to work!
Comment 19 Filip Pizlo 2017-10-31 12:34:58 PDT
Created attachment 325470 [details]
passes easy tests, now expanding to renderobject
Comment 20 Filip Pizlo 2017-10-31 13:31:02 PDT
Created attachment 325481 [details]
more
Comment 21 Filip Pizlo 2017-10-31 13:56:07 PDT
Created attachment 325486 [details]
more!
Comment 22 Filip Pizlo 2017-10-31 19:48:17 PDT
Created attachment 325540 [details]
it is written
Comment 23 Filip Pizlo 2017-10-31 19:59:33 PDT
A really easy way of making this more efficient is to support the idea of an IsoHeap allocating objects in an IsoPage of some other IsoHeap. When this happens, we would somehow make freeing that object only free it back to the IsoHeap that allocated it. This would mean that an IsoPage would have to be able to enter a multi-heap state, where it separately manages bits for each heap that owns it, and knows its separate state in each heap. We can only free it when it's free in all heaps.

Seems doable but super hard.
Comment 24 Filip Pizlo 2017-10-31 20:02:14 PDT
(In reply to Filip Pizlo from comment #23)
> A really easy way of making this more efficient is to support the idea of an
> IsoHeap allocating objects in an IsoPage of some other IsoHeap. When this
> happens, we would somehow make freeing that object only free it back to the
> IsoHeap that allocated it. This would mean that an IsoPage would have to be
> able to enter a multi-heap state, where it separately manages bits for each
> heap that owns it, and knows its separate state in each heap. We can only
> free it when it's free in all heaps.
> 
> Seems doable but super hard.

Probably, at the time of allocation, we would flag the allocated object as being from another heap.  Then the IsoPage would continue to "belong" to one heap but would allocate some rare data for maintaining separate states for other heaps.

We'd have to appropriately throttle use of this feature.  For example, it's probably only practical for heaps that don't yet have their own pages.  Maybe it's also only practical to steal in this way from IsoHeaps that only have one page.

Or, maybe IsoHeaps would use special shared pages until those shared pages fill up.

This seems hard in two ways - the actual implementation and the heuristics it requires.
Comment 25 Filip Pizlo 2017-11-02 10:38:23 PDT
CC'd some people who know more about rendering.

IsoHeaps are a security feature that cause each ISO_ALLOCATED type to have its own isolated heap at the virtual memory level (IsoHeaps can trade physical pages, but once a virtual memory address is associated with an object of a type, only other objects of exactly that type will appear at that address in the future).  In order to mitigate the perf overhead of managing multiple heaps, IsoHeap is type-specialized so that all math having to do with the size and alignment of the types it allocates is constant-folded.  Also, this ensures that we can always rapidly find the right IsoHeap allocator for any type.

Please take a look at the WebCore part of this change.  The flipside of those optimizations is that each ISO_ALLOCATED type needs two annotations:

- Inside the class:
    WTF_MAKE_ISO_ALLOCATED(ClassType);

- In the .cpp file for that class:
    WTF_MAKE_ISO_ALLOCATED_IMPL(ClassType);

Alternatively, if you're willing to #include <wtf/IsoMallocInlines.h>, then you can just do this inside the class:
    WTF_MAKE_ISO_ALLOCATED_INLINE(ClassType)

I recommend the latter for classes declared in .cpp files.  I don't recommend #including <wtf/IsoMallocInlines.h> in .h files.

You have to do this dance for each type in a type hierarchy, because there is no way (that I thought of) to create new overrides in the base class that automagically instantiate a separate IsoHeap per type.  I hope that's only annoying for me - the person refactoring code to use IsoHeap - and not everyone after.  Hopefully, it's not hard to remember to add those ISO_ALLOCATED annotations to new classes.  If you don't do it, then you will RELEASE_ASSERT on first allocation, unless the subclass happens to have the same size, in which case it'll just work.
Comment 26 Simon Fraser (smfr) 2017-11-02 11:04:11 PDT
(In reply to Filip Pizlo from comment #25)

> IsoHeaps are a security feature that cause each ISO_ALLOCATED type to have
> its own isolated heap at the virtual memory level (IsoHeaps can trade
> physical pages, but once a virtual memory address is associated with an
> object of a type, only other objects of exactly that type will appear at
> that address in the future).

This sounds like it will make it more likely that objects of the same type will tend to re-use addresses more often than before, which could lead to more predictable use-after-free behavior. Is that a concern?
Comment 27 Filip Pizlo 2017-11-02 11:14:20 PDT
(In reply to Simon Fraser (smfr) from comment #26)
> (In reply to Filip Pizlo from comment #25)
> 
> > IsoHeaps are a security feature that cause each ISO_ALLOCATED type to have
> > its own isolated heap at the virtual memory level (IsoHeaps can trade
> > physical pages, but once a virtual memory address is associated with an
> > object of a type, only other objects of exactly that type will appear at
> > that address in the future).
> 
> This sounds like it will make it more likely that objects of the same type
> will tend to re-use addresses more often than before, which could lead to
> more predictable use-after-free behavior. Is that a concern?

No for two reasons.

First, the worst use-after-free's are the ones that lead to type confusion.  If we can only alias objects of the same type with UAF then it doesn't give you a path to RCE.  It could still be used for UXSS.

Second, attackers are good at making bmalloc or any other malloc allocate objects predictably.  IsoHeaps don't make that fundamentally easier (it was feasible before and is still feasible now).
Comment 28 Simon Fraser (smfr) 2017-11-02 11:49:41 PDT
(In reply to Filip Pizlo from comment #27)

> No for two reasons.
> 
> First, the worst use-after-free's are the ones that lead to type confusion. 
> If we can only alias objects of the same type with UAF then it doesn't give
> you a path to RCE.  It could still be used for UXSS.
> 
> Second, attackers are good at making bmalloc or any other malloc allocate
> objects predictably.  IsoHeaps don't make that fundamentally easier (it was
> feasible before and is still feasible now).

Cool.

Your macros sound fine. Shame we can't detect lack of the macros on a subclass at compile time, though.
Comment 29 Filip Pizlo 2017-11-02 12:34:55 PDT
Created attachment 325747 [details]
it compiles!
Comment 30 Filip Pizlo 2017-11-02 12:54:35 PDT
(In reply to Filip Pizlo from comment #29)
> Created attachment 325747 [details]
> it compiles!

And it seems to work.  Safari started just fine.

Now to test it more...
Comment 31 Geoffrey Garen 2017-11-02 13:52:05 PDT
> - Super dedicated first-fit discipline, so that we can maximize the amount
> of pages scavenged as well as their contiguity.
> - Bump'n'pop allocator in each page. Bump'n'pop increases our ability to do
> first-fit, which increases our ability to free pages.

Tbis is a bummer, since it undoes the optimization provided by bmalloc. I'd really like to avoid doing this unless we meet a high burden of proof that it's required.
Comment 32 Filip Pizlo 2017-11-02 14:01:18 PDT
(In reply to Geoffrey Garen from comment #31)
> > - Super dedicated first-fit discipline, so that we can maximize the amount
> > of pages scavenged as well as their contiguity.
> > - Bump'n'pop allocator in each page. Bump'n'pop increases our ability to do
> > first-fit, which increases our ability to free pages.
> 
> Tbis is a bummer, since it undoes the optimization provided by bmalloc. I'd
> really like to avoid doing this unless we meet a high burden of proof that
> it's required.

It wouldn't matter for the code.  It's not possible to literally reuse SmallPage etc because of other constraints.  So, if I used immix-style allocation, then I'd have to implement it a second time.  Since I'm more familiar with the riptide-style, I just went with that.

I think that the immix-style is probably better for perf.  I was going to test that in a follow-up.  It's not in this patch only because I wanted to get something to work quickly.
Comment 33 Filip Pizlo 2017-11-02 17:55:56 PDT
Created attachment 325807 [details]
fixed a bug

I can now browse twitter.
Comment 34 Filip Pizlo 2017-11-02 19:40:15 PDT
Created attachment 325823 [details]
it sorta works
Comment 35 Build Bot 2017-11-02 19:42:49 PDT
Attachment 325823 [details] did not pass style-queue:


ERROR: Source/bmalloc/test/testbmalloc.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/test/testbmalloc.cpp:102:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:131:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:146:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/bmalloc/IsoAllocatorInlines.h:78:  More than one command on the same line  [whitespace/newline] [4]
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/FreeList.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/AllIsoHeaps.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/IsoTLS.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/IsoTLS.cpp:56:  Use nullptr instead of NULL.  [readability/null] [5]
ERROR: Source/WebCore/rendering/svg/RenderSVGResourcePattern.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:47:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:50:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:64:  The parameter name "freeList" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/Bits.h:230:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/Bits.h:283:  Missing spaces around |  [whitespace/operators] [3]
ERROR: Source/WebCore/rendering/RenderTableSection.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntryInlines.h:59:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/IsoDirectoryInlines.h:49:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapInlines.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapImpl.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/IsoTLSEntry.h:38:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLS.h:77:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLSLayout.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/IsoTLSEntry.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/rendering/RenderImageResource.h:39:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free  [changelog/unwantedsecurityterms] [3]
Total errors found: 28 in 284 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 36 Filip Pizlo 2017-11-02 21:48:26 PDT
Created attachment 325844 [details]
fix some builds
Comment 37 Build Bot 2017-11-02 21:52:19 PDT
Attachment 325844 [details] did not pass style-queue:


ERROR: Source/bmalloc/test/testbmalloc.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/test/testbmalloc.cpp:104:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:133:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:148:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/bmalloc/IsoAllocatorInlines.h:78:  More than one command on the same line  [whitespace/newline] [4]
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/FreeList.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/AllIsoHeaps.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/IsoTLS.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/IsoTLS.cpp:56:  Use nullptr instead of NULL.  [readability/null] [5]
ERROR: Source/WebCore/rendering/svg/RenderSVGResourcePattern.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:47:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:50:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:64:  The parameter name "freeList" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/Bits.h:230:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/Bits.h:283:  Missing spaces around |  [whitespace/operators] [3]
ERROR: Source/WebCore/rendering/RenderTableSection.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntryInlines.h:59:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/IsoDirectoryInlines.h:49:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapInlines.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapImpl.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/IsoTLSEntry.h:38:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLS.h:77:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLSLayout.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/IsoTLSEntry.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/rendering/RenderImageResource.h:39:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free  [changelog/unwantedsecurityterms] [3]
Total errors found: 28 in 284 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 38 Filip Pizlo 2017-11-03 08:29:18 PDT
Created attachment 325897 [details]
more build fixing
Comment 39 Build Bot 2017-11-03 08:32:39 PDT
Attachment 325897 [details] did not pass style-queue:


ERROR: Source/bmalloc/test/testbmalloc.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/test/testbmalloc.cpp:104:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:133:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:148:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/bmalloc/IsoAllocatorInlines.h:78:  More than one command on the same line  [whitespace/newline] [4]
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/FreeList.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/AllIsoHeaps.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/IsoTLS.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/IsoTLS.cpp:56:  Use nullptr instead of NULL.  [readability/null] [5]
ERROR: Source/WebCore/rendering/svg/RenderSVGResourcePattern.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:47:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:50:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:64:  The parameter name "freeList" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/Bits.h:230:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/Bits.h:283:  Missing spaces around |  [whitespace/operators] [3]
ERROR: Source/WebCore/rendering/RenderTableSection.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntryInlines.h:59:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/IsoDirectoryInlines.h:49:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapInlines.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapImpl.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/IsoTLSEntry.h:38:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLS.h:77:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLSLayout.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/IsoTLSEntry.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/rendering/RenderImageResource.h:39:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free  [changelog/unwantedsecurityterms] [3]
Total errors found: 28 in 284 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 40 Filip Pizlo 2017-11-03 14:01:40 PDT
Created attachment 325960 [details]
more fixes

This seems to make a lot of tests pass.
Comment 41 Build Bot 2017-11-03 14:05:36 PDT
Attachment 325960 [details] did not pass style-queue:


ERROR: Source/bmalloc/test/testbmalloc.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/test/testbmalloc.cpp:104:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:133:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:148:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/bmalloc/IsoAllocatorInlines.h:78:  More than one command on the same line  [whitespace/newline] [4]
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/FreeList.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/AllIsoHeaps.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/IsoTLS.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/IsoTLS.cpp:60:  Use nullptr instead of NULL.  [readability/null] [5]
ERROR: Source/WebCore/rendering/svg/RenderSVGResourcePattern.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:47:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:50:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:64:  The parameter name "freeList" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/Bits.h:230:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/Bits.h:283:  Missing spaces around |  [whitespace/operators] [3]
ERROR: Source/WebCore/rendering/RenderTableSection.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntryInlines.h:59:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/IsoDirectoryInlines.h:49:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapInlines.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapImpl.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/IsoTLSEntry.h:38:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLS.h:77:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLSLayout.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/IsoTLSEntry.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/rendering/RenderImageResource.h:39:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free  [changelog/unwantedsecurityterms] [3]
Total errors found: 28 in 284 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 42 Build Bot 2017-11-03 14:58:16 PDT
Comment on attachment 325960 [details]
more fixes

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

Number of test failures exceeded the failure limit.
Comment 43 Build Bot 2017-11-03 14:58:17 PDT
Created attachment 325968 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 44 Filip Pizlo 2017-11-03 19:38:29 PDT
Created attachment 326003 [details]
the patch
Comment 45 Build Bot 2017-11-03 19:41:45 PDT
Attachment 326003 [details] did not pass style-queue:


ERROR: Source/bmalloc/test/testbmalloc.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/test/testbmalloc.cpp:104:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:133:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:148:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/bmalloc/IsoAllocatorInlines.h:78:  More than one command on the same line  [whitespace/newline] [4]
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/FreeList.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/AllIsoHeaps.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/IsoTLS.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/IsoTLS.cpp:60:  Use nullptr instead of NULL.  [readability/null] [5]
ERROR: Source/WebCore/rendering/svg/RenderSVGResourcePattern.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:47:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:50:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:64:  The parameter name "freeList" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/Bits.h:230:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/Bits.h:283:  Missing spaces around |  [whitespace/operators] [3]
ERROR: Source/WebCore/rendering/RenderTableSection.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntryInlines.h:59:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/IsoDirectoryInlines.h:49:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapInlines.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapImpl.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/IsoTLSEntry.h:38:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLS.h:77:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLSLayout.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/IsoTLSEntry.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/rendering/RenderImageResource.h:39:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free  [changelog/unwantedsecurityterms] [3]
Total errors found: 28 in 285 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 46 Filip Pizlo 2017-11-03 20:23:30 PDT
Created attachment 326007 [details]
the patch
Comment 47 Build Bot 2017-11-03 20:26:15 PDT
Attachment 326007 [details] did not pass style-queue:


ERROR: Source/bmalloc/test/testbmalloc.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/test/testbmalloc.cpp:104:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:133:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:148:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/bmalloc/IsoAllocatorInlines.h:78:  More than one command on the same line  [whitespace/newline] [4]
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/FreeList.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/bmalloc.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/AllIsoHeaps.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/IsoTLS.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/IsoTLS.cpp:61:  Use nullptr instead of NULL.  [readability/null] [5]
ERROR: Source/WebCore/rendering/svg/RenderSVGResourcePattern.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:47:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:50:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:64:  The parameter name "freeList" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/Bits.h:230:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/Bits.h:283:  Missing spaces around |  [whitespace/operators] [3]
ERROR: Source/WebCore/rendering/RenderTableSection.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntryInlines.h:59:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:53:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:53:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:87:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:87:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoDirectoryInlines.h:49:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapInlines.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapImpl.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/IsoTLSEntry.h:38:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLS.h:77:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLSLayout.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/IsoTLSEntry.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/rendering/RenderImageResource.h:39:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free  [changelog/unwantedsecurityterms] [3]
Total errors found: 33 in 286 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 48 Filip Pizlo 2017-11-03 20:38:53 PDT
Created attachment 326008 [details]
the patch
Comment 49 Build Bot 2017-11-03 20:42:50 PDT
Attachment 326008 [details] did not pass style-queue:


ERROR: Source/bmalloc/test/testbmalloc.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/test/testbmalloc.cpp:104:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:133:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:148:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/bmalloc/IsoAllocatorInlines.h:78:  More than one command on the same line  [whitespace/newline] [4]
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/FreeList.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/bmalloc.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/AllIsoHeaps.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/IsoTLS.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/IsoTLS.cpp:61:  Use nullptr instead of NULL.  [readability/null] [5]
ERROR: Source/WebCore/rendering/svg/RenderSVGResourcePattern.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:47:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:50:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:64:  The parameter name "freeList" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/Bits.h:230:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/Bits.h:283:  Missing spaces around |  [whitespace/operators] [3]
ERROR: Source/WebCore/rendering/RenderTableSection.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntryInlines.h:59:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:53:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:53:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:87:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:87:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoDirectoryInlines.h:49:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapInlines.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapImpl.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/IsoTLSEntry.h:38:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLS.h:77:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLSLayout.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/IsoTLSEntry.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/rendering/RenderImageResource.h:39:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free  [changelog/unwantedsecurityterms] [3]
Total errors found: 33 in 286 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 50 Filip Pizlo 2017-11-03 20:47:10 PDT
Created attachment 326010 [details]
the patch
Comment 51 Build Bot 2017-11-03 20:50:01 PDT
Attachment 326010 [details] did not pass style-queue:


ERROR: Source/bmalloc/test/testbmalloc.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/test/testbmalloc.cpp:104:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:133:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:148:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/bmalloc/IsoAllocatorInlines.h:78:  More than one command on the same line  [whitespace/newline] [4]
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/FreeList.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/bmalloc.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/AllIsoHeaps.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/IsoTLS.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/IsoTLS.cpp:61:  Use nullptr instead of NULL.  [readability/null] [5]
ERROR: Source/WebCore/rendering/svg/RenderSVGResourcePattern.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:48:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:51:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:65:  The parameter name "freeList" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/Bits.h:230:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/Bits.h:283:  Missing spaces around |  [whitespace/operators] [3]
ERROR: Source/WebCore/rendering/RenderTableSection.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntryInlines.h:59:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:53:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:53:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:87:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:87:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoDirectoryInlines.h:49:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapInlines.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapImpl.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/IsoTLSEntry.h:38:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLS.h:77:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLSLayout.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/IsoTLSEntry.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/rendering/RenderImageResource.h:39:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free  [changelog/unwantedsecurityterms] [3]
Total errors found: 33 in 286 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 52 Filip Pizlo 2017-11-03 20:55:49 PDT
Created attachment 326012 [details]
the patch
Comment 53 Build Bot 2017-11-03 20:59:13 PDT
Attachment 326012 [details] did not pass style-queue:


ERROR: Source/bmalloc/test/testbmalloc.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/test/testbmalloc.cpp:104:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:133:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:148:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/bmalloc/IsoAllocatorInlines.h:78:  More than one command on the same line  [whitespace/newline] [4]
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/FreeList.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/bmalloc.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/AllIsoHeaps.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/IsoTLS.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/IsoTLS.cpp:61:  Use nullptr instead of NULL.  [readability/null] [5]
ERROR: Source/WebCore/rendering/svg/RenderSVGResourcePattern.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:48:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:51:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:65:  The parameter name "freeList" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/Bits.h:231:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/Bits.h:284:  Missing spaces around |  [whitespace/operators] [3]
ERROR: Source/WebCore/rendering/RenderTableSection.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntryInlines.h:59:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:53:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:53:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:87:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:87:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoDirectoryInlines.h:49:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapInlines.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapImpl.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/IsoTLSEntry.h:38:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLS.h:77:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLSLayout.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/IsoTLSEntry.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/rendering/RenderImageResource.h:39:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free  [changelog/unwantedsecurityterms] [3]
Total errors found: 33 in 286 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 54 Ryosuke Niwa 2017-11-03 21:06:07 PDT
WebCore changes look fine to me. I just wish we didn't have to add _IMPL macro to cpp files but I guess including IsoMallocInlines.h would regress the build time?
Comment 55 Filip Pizlo 2017-11-04 16:26:56 PDT
(In reply to Ryosuke Niwa from comment #54)
> WebCore changes look fine to me. I just wish we didn't have to add _IMPL
> macro to cpp files but I guess including IsoMallocInlines.h would regress
> the build time?

For now the #include situation also makes it very easy to hack on the isoheap implementation.  It's very fast to recompile webcore since only a small number of unified sources end up including IsoMallocInlines.
Comment 56 Filip Pizlo 2017-11-04 16:27:08 PDT
PLT, motionmark, and membuster are all neutral.
Comment 57 Filip Pizlo 2017-11-04 16:28:11 PDT
Created attachment 326049 [details]
the patch

Just updating changelogs to include new perf results.

It's neutral on everything I tried (PLT, membuster, motionmark).
Comment 58 Build Bot 2017-11-04 16:30:57 PDT
Attachment 326049 [details] did not pass style-queue:


ERROR: Source/bmalloc/test/testbmalloc.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/test/testbmalloc.cpp:104:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:133:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:148:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/bmalloc/IsoAllocatorInlines.h:78:  More than one command on the same line  [whitespace/newline] [4]
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/FreeList.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/bmalloc.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/AllIsoHeaps.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/IsoTLS.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/IsoTLS.cpp:61:  Use nullptr instead of NULL.  [readability/null] [5]
ERROR: Source/WebCore/rendering/svg/RenderSVGResourcePattern.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:48:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:51:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:65:  The parameter name "freeList" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/Bits.h:231:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/Bits.h:284:  Missing spaces around |  [whitespace/operators] [3]
ERROR: Source/WebCore/rendering/RenderTableSection.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntryInlines.h:59:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:53:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:53:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:87:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:87:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoDirectoryInlines.h:49:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapInlines.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapImpl.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/IsoTLSEntry.h:38:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLS.h:77:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLSLayout.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/IsoTLSEntry.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/rendering/RenderImageResource.h:39:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free  [changelog/unwantedsecurityterms] [3]
Total errors found: 33 in 286 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 59 Saam Barati 2017-11-06 17:02:24 PST
Comment on attachment 326049 [details]
the patch

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

r=me
I have a few comments and a couple questions.

> Source/bmalloc/ChangeLog:18
> +        pages. Pages are corrected into directories. Directories track pages using bitvectors, so

corrected => collected

> Source/bmalloc/bmalloc/Bits.h:1
> +/*

It's a bit unfortunate that this is probably almost identical to WTF's implementation.

> Source/bmalloc/bmalloc/Bits.h:496
> +        return BitReference(&this->m_words.word(index >> 5), 1 << (index & 31));

WHy 5?

> Source/bmalloc/bmalloc/DeferredDecommit.h:45
> +
> +
> +    

unneeded whitespace

> Source/bmalloc/bmalloc/IsoDirectory.h:37
> +class IsoDirectoryBaseBase {

BaseBase :(

> Source/bmalloc/bmalloc/IsoDirectory.h:42
> +    virtual void didDecommit(unsigned index) = 0;

Do we really need this to be virtual? There is only one implementation.

> Source/bmalloc/bmalloc/IsoDirectory.h:69
> +    void didBecome(IsoPage<Config>*, IsoPageTrigger) override;

This only has one implementation. Do we need it to be virtual?

> Source/bmalloc/bmalloc/IsoDirectoryInlines.h:49
> +    unsigned pageIndex = (m_eligible | ~m_committed).findBit(m_firstEligible, true);;

style: extra semicolon at the end

> Source/bmalloc/bmalloc/IsoHeapImpl.h:67
> +    void didBecomeEligible(IsoDirectory<Config, numPagesInInlineDirectory>*); // This one probably needs to set a bit or something.
> +    void didBecomeEligible(IsoDirectory<Config, IsoDirectoryPage<Config>::numPages>*); // This one needs to do things.

these comments are not helpful

> Source/bmalloc/bmalloc/IsoHeapImpl.h:96
> +    // The basic operations that we need to support are:

Part of this text seems to have been written before you implemented things. Maybe you can clean some of it up now that you've made implementation decisions?

> Source/bmalloc/bmalloc/IsoHeapImpl.h:121
> +    // - Deallocation should probably use a deallocation log, just like the rest of bmalloc.
> +    //   -> This avoids having to use any kinds of atomics on the freeing fast path. The slow path can
> +    //      just grab a lock around the whole isoheap or something.

This makes things seem up in the air, but are they?

> Source/bmalloc/bmalloc/IsoHeapImpl.h:122
> +    // - When we create an isoheap, we want to make sure that TLCs find out.

TLC?

> Source/bmalloc/bmalloc/IsoHeapImplInlines.h:46
> +        if (result.kind == EligibilityKind::Full)

Do we want to assert it's not OOM kind?

> Source/bmalloc/bmalloc/IsoHeapImplInlines.h:62
> +        if (result.kind != EligibilityKind::Full)

ditto w.r.t OOM?

> Source/bmalloc/bmalloc/IsoPage.h:40
> +    static constexpr size_t pageSize = 16384;

Do we want this to be 4096 on iOS? Doesn't bmalloc already have a notion of this somewhere?

> Source/bmalloc/bmalloc/IsoPage.h:46
> +    static constexpr unsigned numObjects = pageSize / Config::objectSize;

Do we want to add static_assert(numObjects > 0)?

> Source/bmalloc/bmalloc/IsoPage.h:55
> +    // This must have a trivial destructor.

Nit: Maybe it's worth putting this comment by the field declarations?

> Source/bmalloc/bmalloc/IsoPageInlines.h:104
> +        unsigned beginWord = begin >> 5;
> +        unsigned endWord = end >> 5;

Maybe we can give this 5 a name? And also static cast that (sizeof(unsigned) * 8 == 1<<5)?

> Source/bmalloc/bmalloc/IsoPageInlines.h:124
> +            unsigned endBeginSlop = (begin + 31) & ~31;
> +            unsigned beginEndSlop = end & ~31;

seems like this may be related to the 5? as in 2^5 - 1?
Also, bmalloc already has roundUpToMultipleOf, can we use that here w/ 32 instead of it handwritten?

This math confuses me :(

> Source/bmalloc/bmalloc/IsoPageInlines.h:180
> +        unsigned wordIndex = index >> 5;

why not / 32 here? llvm should do the right thing and it's easier to read

> Source/bmalloc/bmalloc/IsoPageInlines.h:184
> +        unsigned bitMask = 1 << (index & 31);
> +        if (word & bitMask)
> +            continue;

This confuses me. What is the purpose of this?

> Source/bmalloc/bmalloc/IsoPageInlines.h:186
> +        if (!word)
> +            m_numNonEmptyWords++;

Shouldn't this condition be flipped?

> Source/bmalloc/bmalloc/IsoPageInlines.h:231
> +        for (unsigned bitIndex = 0; bitIndex < 32; ++bitIndex) {

nit: maybe it's worth writing this as while(word) { ... }? Or is it worth having this be unrolled perhaps?

> Source/bmalloc/bmalloc/IsoTLSInlines.h:109
> +    if (!handle.isInitialized()) {
> +        std::lock_guard<StaticMutex> locker(handle.m_initializationLock);

Can't two threads race to enter here at the same time? Shouldn't we check isInitialized again?
Comment 60 Saam Barati 2017-11-06 17:06:48 PST
Comment on attachment 326049 [details]
the patch

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

> Source/bmalloc/bmalloc/IsoTLSInlines.h:108
> +    if (!handle.isInitialized()) {

Maybe UNLIKELY?
Comment 61 Saam Barati 2017-11-06 17:08:26 PST
Comment on attachment 326049 [details]
the patch

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

> Source/bmalloc/bmalloc/IsoPage.h:91
> +    // 000 - Deallocated. It has no objects and its memory is not paged in.
> +    // 111 - Empty.
> +    // 101 - Eligible for allocation, meaning that there is at least one free object in the page.
> +    // 001 - Full.
> +    // 001 - Currently being used for allocation.

Which 3 bits are these digits in reference to?
Comment 62 Filip Pizlo 2017-11-07 10:29:02 PST
(In reply to Saam Barati from comment #59)
> Comment on attachment 326049 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326049&action=review
> 
> r=me
> I have a few comments and a couple questions.
> 
> > Source/bmalloc/ChangeLog:18
> > +        pages. Pages are corrected into directories. Directories track pages using bitvectors, so
> 
> corrected => collected

Fixed.

> 
> > Source/bmalloc/bmalloc/Bits.h:1
> > +/*
> 
> It's a bit unfortunate that this is probably almost identical to WTF's
> implementation.

Yeah. :-(

It's subtly different in that it allocates the bits inline.  But in a world where WTF and bmalloc were one, this would just be a different specialization of the various FastBitVector templates/base classes.

> 
> > Source/bmalloc/bmalloc/Bits.h:496
> > +        return BitReference(&this->m_words.word(index >> 5), 1 << (index & 31));
> 
> WHy 5?

Shift by 5 === multiply/divide by 32.

> 
> > Source/bmalloc/bmalloc/DeferredDecommit.h:45
> > +
> > +
> > +    
> 
> unneeded whitespace

Fixed.

> 
> > Source/bmalloc/bmalloc/IsoDirectory.h:37
> > +class IsoDirectoryBaseBase {
> 
> BaseBase :(
> 
> > Source/bmalloc/bmalloc/IsoDirectory.h:42
> > +    virtual void didDecommit(unsigned index) = 0;
> 
> Do we really need this to be virtual? There is only one implementation.

Yes.  There are infinity implementations, since the implementation is in a template class.

I use this idiom in a bunch of places in this code.  Say you have Foo<T> for many T's.  And say that Foo<T> has a method that does not depend on T.  And say that you want to call this method for a raw Foo*, without having to know T.  You can do this by creating a FooBase that has a pure version method and Foo<T> overrides it, and then the callers use FooBase* as the T-less pointer to Foo<T>.

> 
> > Source/bmalloc/bmalloc/IsoDirectory.h:69
> > +    void didBecome(IsoPage<Config>*, IsoPageTrigger) override;
> 
> This only has one implementation. Do we need it to be virtual?

See above.

> 
> > Source/bmalloc/bmalloc/IsoDirectoryInlines.h:49
> > +    unsigned pageIndex = (m_eligible | ~m_committed).findBit(m_firstEligible, true);;
> 
> style: extra semicolon at the end

Fixed.

> 
> > Source/bmalloc/bmalloc/IsoHeapImpl.h:67
> > +    void didBecomeEligible(IsoDirectory<Config, numPagesInInlineDirectory>*); // This one probably needs to set a bit or something.
> > +    void didBecomeEligible(IsoDirectory<Config, IsoDirectoryPage<Config>::numPages>*); // This one needs to do things.
> 
> these comments are not helpful

Removed.

> 
> > Source/bmalloc/bmalloc/IsoHeapImpl.h:96
> > +    // The basic operations that we need to support are:
> 
> Part of this text seems to have been written before you implemented things.
> Maybe you can clean some of it up now that you've made implementation
> decisions?

Removed.

> 
> > Source/bmalloc/bmalloc/IsoHeapImpl.h:121
> > +    // - Deallocation should probably use a deallocation log, just like the rest of bmalloc.
> > +    //   -> This avoids having to use any kinds of atomics on the freeing fast path. The slow path can
> > +    //      just grab a lock around the whole isoheap or something.
> 
> This makes things seem up in the air, but are they?

Removed.

> 
> > Source/bmalloc/bmalloc/IsoHeapImpl.h:122
> > +    // - When we create an isoheap, we want to make sure that TLCs find out.
> 
> TLC?

TLC = thread local cache.  Also sometimes known as a TLAB = thread local allocation buffer.

TLC and TLAB are common terms in GC literature.  I have a tendency to use them interchangeably.

> 
> > Source/bmalloc/bmalloc/IsoHeapImplInlines.h:46
> > +        if (result.kind == EligibilityKind::Full)
> 
> Do we want to assert it's not OOM kind?

No.  It's OK to have OOM.  This code will return the OOM in this case.

> 
> > Source/bmalloc/bmalloc/IsoHeapImplInlines.h:62
> > +        if (result.kind != EligibilityKind::Full)
> 
> ditto w.r.t OOM?

If we ran out of memory, we should tell the user about it.  This code is right.

> 
> > Source/bmalloc/bmalloc/IsoPage.h:40
> > +    static constexpr size_t pageSize = 16384;
> 
> Do we want this to be 4096 on iOS? Doesn't bmalloc already have a notion of
> this somewhere?

Other way around.  iOS wants 16384.  I don't think it's beneficial to have the size be 4096 on macOS.

> 
> > Source/bmalloc/bmalloc/IsoPage.h:46
> > +    static constexpr unsigned numObjects = pageSize / Config::objectSize;
> 
> Do we want to add static_assert(numObjects > 0)?

Added.

> 
> > Source/bmalloc/bmalloc/IsoPage.h:55
> > +    // This must have a trivial destructor.
> 
> Nit: Maybe it's worth putting this comment by the field declarations?

Fixed.

> 
> > Source/bmalloc/bmalloc/IsoPageInlines.h:104
> > +        unsigned beginWord = begin >> 5;
> > +        unsigned endWord = end >> 5;
> 
> Maybe we can give this 5 a name? And also static cast that (sizeof(unsigned)
> * 8 == 1<<5)?

I think that assert would be super tautological.  The other bitvector codes in WK don't do that.

> 
> > Source/bmalloc/bmalloc/IsoPageInlines.h:124
> > +            unsigned endBeginSlop = (begin + 31) & ~31;
> > +            unsigned beginEndSlop = end & ~31;
> 
> seems like this may be related to the 5? as in 2^5 - 1?
> Also, bmalloc already has roundUpToMultipleOf, can we use that here w/ 32
> instead of it handwritten?
> 
> This math confuses me :(

Sorry, I know the raw math better than any helpers.

> 
> > Source/bmalloc/bmalloc/IsoPageInlines.h:180
> > +        unsigned wordIndex = index >> 5;
> 
> why not / 32 here? llvm should do the right thing and it's easier to read

Yeah.  I've just trained myself to think in terms of shifts.

> 
> > Source/bmalloc/bmalloc/IsoPageInlines.h:184
> > +        unsigned bitMask = 1 << (index & 31);
> > +        if (word & bitMask)
> > +            continue;
> 
> This confuses me. What is the purpose of this?

If the object is in use, don't put it on the free list.

> 
> > Source/bmalloc/bmalloc/IsoPageInlines.h:186
> > +        if (!word)
> > +            m_numNonEmptyWords++;
> 
> Shouldn't this condition be flipped?

No.  We're setting bits.  Some bits area already set.  We want to detect if the word before we set the bit was zero.  If it was, then us setting the bit increases the number of words that are not zero.  If the word was already not zero, then we aren't changing the number of non-zero words by setting a bit.

> 
> > Source/bmalloc/bmalloc/IsoPageInlines.h:231
> > +        for (unsigned bitIndex = 0; bitIndex < 32; ++bitIndex) {
> 
> nit: maybe it's worth writing this as while(word) { ... }? Or is it worth
> having this be unrolled perhaps?

Oh yeah!  I would fix it, except I already tested everything, so I'm paranoid.

Also, forEachLiveObject and the other iterators are purely for white-box testing and debugging.  They don't have to be efficient.  They just have to be right.

> 
> > Source/bmalloc/bmalloc/IsoTLSInlines.h:109
> > +    if (!handle.isInitialized()) {
> > +        std::lock_guard<StaticMutex> locker(handle.m_initializationLock);
> 
> Can't two threads race to enter here at the same time? Shouldn't we check
> isInitialized again?

You're totally right.  Fixed.
Comment 63 Filip Pizlo 2017-11-07 11:22:59 PST
Landed in http://trac.webkit.org/changeset/224537/webkit
Comment 64 Saam Barati 2017-11-07 11:47:25 PST
Comment on attachment 326049 [details]
the patch

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

> Source/bmalloc/bmalloc/IsoHeap.h:41
> +// It's not valid to create an IsoHeap except in static storage.

I just realized at least partly why this is. You use the below scheme for 2s complement wraparound for zero-initialized data to allow TLS to notice when it doesn't have that up to that offset initialized.
1. Do we care we're using somewhat UB in C++?
2. Maybe it's worth a comment we rely on this wraparound.
Comment 65 Filip Pizlo 2017-11-07 13:09:43 PST
(In reply to Saam Barati from comment #64)
> Comment on attachment 326049 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326049&action=review
> 
> > Source/bmalloc/bmalloc/IsoHeap.h:41
> > +// It's not valid to create an IsoHeap except in static storage.
> 
> I just realized at least partly why this is. You use the below scheme for 2s
> complement wraparound for zero-initialized data to allow TLS to notice when
> it doesn't have that up to that offset initialized.
> 1. Do we care we're using somewhat UB in C++?

Wraparound for unsigned is well defined.  It's only signed wraparound that is undef.

> 2. Maybe it's worth a comment we rely on this wraparound.
Comment 66 Geoffrey Garen 2017-11-07 20:37:13 PST
> I think that the immix-style is probably better for perf.  I was going to
> test that in a follow-up.

Sounds good.

In addition to finding the best solution for performance, I think it's important to avoid the bad old days where we had N allocators, each with a different set of strategies and tradeoffs for throughput, high-water-mark memory use, long-term memory use, and free list integrity, reflecting the N personalities that implemented them.

Ideally, the only tradeoff you should have to consider when adopting an isolated heap is the fact that you won't share memory with other objects anymore.
Comment 67 Radar WebKit Bug Importer 2017-11-15 13:05:12 PST
<rdar://problem/35568759>
Comment 68 Michael Catanzaro 2017-12-06 14:09:17 PST
Is there a guideline for when to use WTF_MAKE_ISO_ALLOCATED instead of WTF_MAKE_FAST_ALLOCATED or the system allocator?