Bug 175141 - The allocator used to allocate memory for MarkedBlocks and LargeAllocations should not be the Subspace itself
Summary: The allocator used to allocate memory for MarkedBlocks and LargeAllocations s...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks: 174919
  Show dependency treegraph
 
Reported: 2017-08-03 11:20 PDT by Filip Pizlo
Modified: 2017-08-04 13:51 PDT (History)
3 users (show)

See Also:


Attachments
it's a start (51.47 KB, patch)
2017-08-03 11:59 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (59.35 KB, patch)
2017-08-03 12:38 PDT, Filip Pizlo
mark.lam: review+
Details | Formatted Diff | Diff
patch for landing (59.48 KB, patch)
2017-08-03 13:30 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
patch for landing (59.52 KB, patch)
2017-08-03 14:10 PDT, Filip Pizlo
no flags 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-08-03 11:20:23 PDT
Patch forthcoming.
Comment 1 Filip Pizlo 2017-08-03 11:59:41 PDT
Created attachment 317136 [details]
it's a start
Comment 2 Filip Pizlo 2017-08-03 12:38:34 PDT
Created attachment 317139 [details]
the patch
Comment 3 Build Bot 2017-08-03 12:41:31 PDT
Attachment 317139 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/heap/AlignedMemoryAllocator.h:39:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/GigacageAlignedMemoryAllocator.h:41:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/ForwardingHeaders/heap/FastMallocAlignedMemoryAllocator.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/JavaScriptCore/heap/FastMallocAlignedMemoryAllocator.h:38:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Mark Lam 2017-08-03 12:58:32 PDT
Comment on attachment 317139 [details]
the patch

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

r=me with suggestions.

> Source/JavaScriptCore/heap/MarkedBlock.cpp:337
> +    RELEASE_ASSERT(m_allocator->subspace()->alignedMemoryAllocator() == m_alignedMemoryAllocator);

just use subspace() instead of m_allocator->subspace().

> Source/JavaScriptCore/heap/MarkedBlock.cpp:400
> +Subspace* MarkedBlock::Handle::subspace() const
> +{
> +    return allocator()->subspace();
> +}

Is there any reason this should not be an inline function?
Comment 5 Mark Lam 2017-08-03 12:59:58 PDT
Comment on attachment 317139 [details]
the patch

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

> Source/JavaScriptCore/heap/AlignedMemoryAllocator.h:39
> +    virtual void* tryAllocateAlignedMemory(size_t alignment, size_t size) = 0;

Please also fix the style-checker complaint and remove the unneeded size label.

> Source/JavaScriptCore/heap/FastMallocAlignedMemoryAllocator.h:38
> +    void* tryAllocateAlignedMemory(size_t alignment, size_t size) override;

Please also fix the style-checker complaint and remove the unneeded size label.

> Source/JavaScriptCore/heap/GigacageAlignedMemoryAllocator.h:41
> +    void* tryAllocateAlignedMemory(size_t alignment, size_t size) override;

Please also fix the style-checker complaint and remove the unneeded size label.
Comment 6 Mark Lam 2017-08-03 13:26:47 PDT
Comment on attachment 317139 [details]
the patch

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

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:902
> +		0FEC3C571F33A45300F59B6C /* FastMallocAlignedMemoryAllocator.h in Headers */ = {isa = PBXBuildFile; fileRef = 0FEC3C551F33A45300F59B6C /* FastMallocAlignedMemoryAllocator.h */; };

You need to give this the Private attribute in Xcode (or do what the entry for VM.h does).
Comment 7 Filip Pizlo 2017-08-03 13:28:44 PDT
(In reply to Mark Lam from comment #5)
> Comment on attachment 317139 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=317139&action=review
> 
> > Source/JavaScriptCore/heap/AlignedMemoryAllocator.h:39
> > +    virtual void* tryAllocateAlignedMemory(size_t alignment, size_t size) = 0;
> 
> Please also fix the style-checker complaint and remove the unneeded size
> label.
> 
> > Source/JavaScriptCore/heap/FastMallocAlignedMemoryAllocator.h:38
> > +    void* tryAllocateAlignedMemory(size_t alignment, size_t size) override;
> 
> Please also fix the style-checker complaint and remove the unneeded size
> label.
> 
> > Source/JavaScriptCore/heap/GigacageAlignedMemoryAllocator.h:41
> > +    void* tryAllocateAlignedMemory(size_t alignment, size_t size) override;
> 
> Please also fix the style-checker complaint and remove the unneeded size
> label.

I think that the style checker is wrong here.  It's true that the word "size" appears in the type "size_t", but since this is a memory allocation and one of the size_t's is named alignment, it's a lot clearer to name the other one "size".
Comment 8 Filip Pizlo 2017-08-03 13:29:28 PDT
(In reply to Mark Lam from comment #6)
> Comment on attachment 317139 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=317139&action=review
> 
> > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:902
> > +		0FEC3C571F33A45300F59B6C /* FastMallocAlignedMemoryAllocator.h in Headers */ = {isa = PBXBuildFile; fileRef = 0FEC3C551F33A45300F59B6C /* FastMallocAlignedMemoryAllocator.h */; };
> 
> You need to give this the Private attribute in Xcode (or do what the entry
> for VM.h does).

Fixed!
Comment 9 Filip Pizlo 2017-08-03 13:30:04 PDT
Created attachment 317145 [details]
patch for landing
Comment 10 Build Bot 2017-08-03 13:32:30 PDT
Attachment 317145 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/heap/AlignedMemoryAllocator.h:39:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/GigacageAlignedMemoryAllocator.h:41:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/ForwardingHeaders/heap/FastMallocAlignedMemoryAllocator.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/JavaScriptCore/heap/FastMallocAlignedMemoryAllocator.h:38:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Filip Pizlo 2017-08-03 14:10:25 PDT
Created attachment 317150 [details]
patch for landing

More build fixing
Comment 12 Build Bot 2017-08-03 14:11:51 PDT
Attachment 317150 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/heap/AlignedMemoryAllocator.h:39:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/GigacageAlignedMemoryAllocator.h:41:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/ForwardingHeaders/heap/FastMallocAlignedMemoryAllocator.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/JavaScriptCore/heap/FastMallocAlignedMemoryAllocator.h:38:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Filip Pizlo 2017-08-03 14:15:59 PDT
(In reply to Mark Lam from comment #4)
> Comment on attachment 317139 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=317139&action=review
> 
> r=me with suggestions.
> 
> > Source/JavaScriptCore/heap/MarkedBlock.cpp:337
> > +    RELEASE_ASSERT(m_allocator->subspace()->alignedMemoryAllocator() == m_alignedMemoryAllocator);
> 
> just use subspace() instead of m_allocator->subspace().

I fixed it to say allocator->subspace(), and I moved it to before where we set m_allocator.  It feels nicer to validate before making changes.

> 
> > Source/JavaScriptCore/heap/MarkedBlock.cpp:400
> > +Subspace* MarkedBlock::Handle::subspace() const
> > +{
> > +    return allocator()->subspace();
> > +}
> 
> Is there any reason this should not be an inline function?

It totally can be!  This is actually a revert.  This is how it worked before I did the block trading patch in the last week or so.

In general, these per-block functions can usually be out-of-line because very few of them are on hot paths, or they are only on hot paths that are in the MarkedBlock.cpp module anyway.  I have a slight out-of-line bias because when debugging it's nice to have out-of-line functions to instrument since it means not having to rebuild the world.
Comment 14 Filip Pizlo 2017-08-04 13:51:11 PDT
Landed in https://trac.webkit.org/changeset/220291/webkit