Attachment 332498 Details for Bug 182226 – the patch

[patch] the patch

blah.patch (text/plain), 6.55 KB, created by Filip Pizlo on 2018-01-28 16:10:57 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Filip Pizlo

Created: 2018-01-28 16:10:57 PST

Size: 6.55 KB

patch

obsolete

>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 227720)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,38 @@
>+2018-01-28  Filip Pizlo  <fpizlo@apple.com>
>+
>+        LargeAllocation should do the same distancing as MarkedBlock
>+        https://bugs.webkit.org/show_bug.cgi?id=182226
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        This makes LargeAllocation do the same exact distancing that MarkedBlock promises to do.
>+        
>+        To make that possible, this patch first makes MarkedBlock know exactly how much distancing it
>+        is doing:
>+        
>+        - I've rationalized the payloadSize calculation. In particular, I made MarkedSpace use the
>+          calculation done in MarkedBlock. MarkedSpace used to do the math a different way. This
>+          keeps the old way just for a static_assert.
>+        
>+        - The promised amount of distancing is now codified in HeapCell.h as
>+          minimumDistanceBetweenCellsFromDifferentOrigins. We assert that the footer size is at least
>+          as big as this. I didn't want to just use footer size for this constant because then, if
>+          you increased the size of the footer, you'd also add padding to every large allocation.
>+        
>+        Then this patch just adds minimumDistanceBetweenCellsFromDifferentOrigins to each large
>+        allocation. It also zeroes that slice of memory to prevent any information leaks that way.
>+        
>+        This is perf neutral. Large allocations start out at ~8000 bytes. The amount of padding is
>+        ~300 bytes. That's 3.75% space overhead for objects that are ~8000 bytes, zero overhead for
>+        smaller objects, and diminishing overhead for larger objects. We allocate very few large
>+        objects, so we shouldn't have any real space overhead from this.
>+
>+        * heap/HeapCell.h:
>+        * heap/LargeAllocation.cpp:
>+        (JSC::LargeAllocation::tryCreate):
>+        * heap/MarkedBlock.h:
>+        * heap/MarkedSpace.h:
>+
> 2018-01-27  Filip Pizlo  <fpizlo@apple.com>
> 
>         Make MarkedBlock::Footer bigger
>Index: Source/JavaScriptCore/heap/HeapCell.h
>===================================================================
>--- Source/JavaScriptCore/heap/HeapCell.h	(revision 227720)
>+++ Source/JavaScriptCore/heap/HeapCell.h	(working copy)
>@@ -1,5 +1,5 @@
> /*
>- * Copyright (C) 2016-2017 Apple Inc. All rights reserved.
>+ * Copyright (C) 2016-2018 Apple Inc. All rights reserved.
>  *
>  * Redistribution and use in source and binary forms, with or without
>  * modification, are permitted provided that the following conditions
>@@ -37,6 +37,8 @@ class Subspace;
> class VM;
> struct CellAttributes;
> 
>+static constexpr unsigned minimumDistanceBetweenCellsFromDifferentOrigins = sizeof(void*) == 8 ? 304 : 288;
>+
> class HeapCell {
> public:
>     enum Kind : int8_t {
>Index: Source/JavaScriptCore/heap/LargeAllocation.cpp
>===================================================================
>--- Source/JavaScriptCore/heap/LargeAllocation.cpp	(revision 227720)
>+++ Source/JavaScriptCore/heap/LargeAllocation.cpp	(working copy)
>@@ -1,5 +1,5 @@
> /*
>- * Copyright (C) 2016-2017 Apple Inc. All rights reserved.
>+ * Copyright (C) 2016-2018 Apple Inc. All rights reserved.
>  *
>  * Redistribution and use in source and binary forms, with or without
>  * modification, are permitted provided that the following conditions
>@@ -35,9 +35,18 @@ namespace JSC {
> 
> LargeAllocation* LargeAllocation::tryCreate(Heap& heap, size_t size, Subspace* subspace)
> {
>-    void* space = subspace->alignedMemoryAllocator()->tryAllocateAlignedMemory(alignment, headerSize() + size);
>+    // This includes padding at the end of the allocation to maintain the distancing constraint.
>+    constexpr size_t distancing = minimumDistanceBetweenCellsFromDifferentOrigins;
>+    size_t sizeBeforeDistancing = headerSize() + size;
>+    size_t sizeIncludingDistancing = sizeBeforeDistancing + distancing;
>+    
>+    void* space = subspace->alignedMemoryAllocator()->tryAllocateAlignedMemory(alignment, sizeIncludingDistancing);
>     if (!space)
>         return nullptr;
>+    
>+    // Make sure that the padding does not contain useful things.
>+    memset(static_cast<char*>(space) + sizeBeforeDistancing, 0, distancing);
>+    
>     if (scribbleFreeCells())
>         scribble(space, size);
>     return new (NotNull, space) LargeAllocation(heap, size, subspace);
>Index: Source/JavaScriptCore/heap/MarkedBlock.h
>===================================================================
>--- Source/JavaScriptCore/heap/MarkedBlock.h	(revision 227720)
>+++ Source/JavaScriptCore/heap/MarkedBlock.h	(working copy)
>@@ -285,14 +285,19 @@ public:
>         Bitmap<atomsPerBlock> m_marks;
>         Bitmap<atomsPerBlock> m_newlyAllocated;
>     };
>-        
>+    
> private:    
>     Footer& footer();
>     const Footer& footer() const;
> 
> public:
>     static constexpr size_t endAtom = (blockSize - sizeof(Footer)) / atomSize;
>+    static constexpr size_t payloadSize = endAtom * atomSize;
>+    static constexpr size_t footerSize = blockSize - payloadSize;
> 
>+    static_assert(payloadSize == ((blockSize - sizeof(MarkedBlock::Footer)) & ~(atomSize - 1)), "Payload size computed the alternate way should give the same result");
>+    static_assert(footerSize >= minimumDistanceBetweenCellsFromDifferentOrigins, "Footer is not big enough to create the necessary distance between objects from different origins");
>+    
>     static MarkedBlock::Handle* tryCreate(Heap&, AlignedMemoryAllocator*);
>         
>     Handle& handle();
>Index: Source/JavaScriptCore/heap/MarkedSpace.h
>===================================================================
>--- Source/JavaScriptCore/heap/MarkedSpace.h	(revision 227720)
>+++ Source/JavaScriptCore/heap/MarkedSpace.h	(working copy)
>@@ -55,9 +55,8 @@ public:
>     // Sizes up to this amount get a size class for each size step.
>     static constexpr size_t preciseCutoff = 80;
>     
>-    // The amount of available payload in a block is the block's size minus the footer. But the
>-    // header size might not be atom size aligned, so we round down the result accordingly.
>-    static constexpr size_t blockPayload = (MarkedBlock::blockSize - sizeof(MarkedBlock::Footer)) & ~(MarkedBlock::atomSize - 1);
>+    // The amount of available payload in a block is the block's size minus the footer.
>+    static constexpr size_t blockPayload = MarkedBlock::payloadSize;
>     
>     // The largest cell we're willing to allocate in a MarkedBlock the "normal way" (i.e. using size
>     // classes, rather than a large allocation) is half the size of the payload, rounded down. This

Flags:

saam: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 182226: 332498