Bug 175141 – The allocator used to allocate memory for MarkedBlocks and LargeAllocations should not be the Subspace itself

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 317136 [details]
it's a start

[Filip Pizlo]

Created attachment 317139 [details]
the patch

[Build Bot]

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.

[Mark Lam]

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?

[Mark Lam]

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.

[Mark Lam]

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).

[Filip Pizlo]

(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".

[Filip Pizlo]

(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!

[Filip Pizlo]

Created attachment 317145 [details]
patch for landing

[Build Bot]

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.

[Filip Pizlo]

Created attachment 317150 [details]
patch for landing

More build fixing

[Build Bot]

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.

[Filip Pizlo]

(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.

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/220291/webkit