Patch forthcoming.
Created attachment 317136 [details] it's a start
Created attachment 317139 [details] the patch
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 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 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 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).
(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".
(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!
Created attachment 317145 [details] patch for landing
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.
Created attachment 317150 [details] patch for landing More build fixing
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.
(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.
Landed in https://trac.webkit.org/changeset/220291/webkit