This is an archive of the discontinued LLVM Phabricator instance.

[Support] Reverted r357058 and r351916, MemoryBlock class now exposes its allocated size instead of requested size.
ClosedPublic

Authored by mvhooren on May 6 2019, 7:25 AM.

Details

Summary

Renamed member 'Size' to 'AllocatedSize' in MemoryBlock and OwningMemoryBlock in order to provide a hint that the allocated size may be different than the requested size. Comments were added to clarify this point.
Updated the InMemoryBuffer in FileOutputBuffer.cpp to track the requested buffer size.

Recently, MemoryBlock was changed so that its 'Size' member holds the size that was requested with allocateMappedMemory instead of the size that was actually allocated by allocateMappedMemory. The allocated size is usually (much) larger than the requested size.
This caused a large waste of memory, as in 100s of MBs in my use case, because on Windows allocateMappedMemory returns memory with a granularity of 64kb (16 pages) and SectionMemoryManager previously assumed that the blocks were multiples of the page size (4kb) as it would be on Unix/Linux systems, in the worst case causing 60kb of wasted memory for each allocation on Windows.

Also see bug: https://bugs.llvm.org/show_bug.cgi?id=41690

Diff Detail

Repository
rL LLVM

Event Timeline

mvhooren created this revision.May 6 2019, 7:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 6 2019, 7:25 AM
mvhooren added a comment.EditedMay 6 2019, 7:25 AM

I do not have commit access by the way. This is also my first time contributing to llvm :-).

mvhooren retitled this revision from MemoryBlock objects now expose their allocated size in addition to the requested size. to [Support] MemoryBlock objects now expose their allocated size in addition to the requested size..May 6 2019, 7:34 AM
lhames requested changes to this revision.May 7 2019, 11:11 AM

I don't think this is the right solution. MemoryBlock's purpose is to track the allocated memory. What the client does with that memory (including subdividing it) is their business. The client already knows the number of bytes they're requesting, so the fix for fragmentation in SectionMemoryManager should be limited to fixes in SectionMemoryManager.

This revision now requires changes to proceed.May 7 2019, 11:11 AM

I don't think this is the right solution. MemoryBlock's purpose is to track the allocated memory. What the client does with that memory (including subdividing it) is their business. The client already knows the number of bytes they're requesting, so the fix for fragmentation in SectionMemoryManager should be limited to fixes in SectionMemoryManager.

Yes I agree it would be better if the Size member of MemoryBlock just held the allocated size. This would mean that r357058 and r351916 (and perhaps others) should be reverted.

lhames added a comment.May 7 2019, 1:01 PM

Yes I agree it would be better if the Size member of MemoryBlock just held the allocated size. This would mean that r357058 and r351916 (and perhaps others) should be reverted.

I agree. To (hopefully) catch all call sites I think we will want to revert, but use your suggested "allocatedSize" name for the method. That should allow us to find and fix any call sites that are making bogus assumptions.

Yes I agree it would be better if the Size member of MemoryBlock just held the allocated size. This would mean that r357058 and r351916 (and perhaps others) should be reverted.

I agree. To (hopefully) catch all call sites I think we will want to revert, but use your suggested "allocatedSize" name for the method. That should allow us to find and fix any call sites that are making bogus assumptions.

Alright I will update and resubmit the patch tomorrow.

lhames added a comment.May 7 2019, 2:49 PM

Great! Thanks so much Machiel!

I don't think this is the right solution. MemoryBlock's purpose is to track the allocated memory. What the client does with that memory (including subdividing it) is their business. The client already knows the number of bytes they're requesting, so the fix for fragmentation in SectionMemoryManager should be limited to fixes in SectionMemoryManager.

Yes I agree it would be better if the Size member of MemoryBlock just held the allocated size. This would mean that r357058 and r351916 (and perhaps others) should be reverted.

By making the Size member be the allocated rather than the requested size effectively reverts those two revisions. To address the issue that r357058 and r351916 fixed, you will need to change the class InMemoryBuffer in FileOutputBuffer.cpp to itself track the requested size rather than use the allocated size from MemoryBlock.

This does seem to be the better solution, given the interface and other usages of MemoryBlock. When I made the change in r357058, I just mirrored the change in r351916, assuming that was the appropriate approach.

Cheers,
Andrew

mvhooren updated this revision to Diff 198615.May 8 2019, 3:43 AM
mvhooren retitled this revision from [Support] MemoryBlock objects now expose their allocated size in addition to the requested size. to [Support] Reverted r357058 and r351916, MemoryBlock class now exposes its allocated size instead of requested size..
mvhooren edited the summary of this revision. (Show Details)

Reverted changes from revisions r357058 and r351916 as discussed. Updated the InMemoryBuffer in FileOutputBuffer.cpp to track the requested buffer size.
The Size member of MemoryBlock and OwningMemoryBlock was renamed to AllocatedSize in order to provide a hint that the allocated size may be different than the requested size. I also added some comments to further clarify this point.
I have checked all the call sites of MemoryBlock::size() and as far as I can tell, FileOutputBuffer is the only location where wrong assumptions were made about the size.

Looking at FileOutputBuffer, I wonder why it is using MemoryBlock/allocateMappedMemory at all. It probably should be using something like a WritableMemoryBuffer instead.
I have not changed this because it is a little beyond the scope of this patch, but it might be worth investigating. Perhaps it does have a requirement somewhere that its memory should be page-aligned, but if so it is not obvious from looking at it.

mvhooren updated this revision to Diff 198827.May 9 2019, 8:01 AM

Patch was missing context in the diff, updated the patch with full context.

Some of the changes in "lib/Support/Unix/Memory.inc" don't appear to have been clang formatted.

lib/Support/FileOutputBuffer.cpp
78–82

Minor nit, perhaps bufferSize -> BufSize to match current naming convention?

mvhooren updated this revision to Diff 198880.May 9 2019, 12:00 PM

Applied Clang Format to the changed files. Renamed bufferSize -> BufSize to match naming convention..

Applied Clang Format to the changed files. Renamed bufferSize -> BufSize to match naming convention..

I think you may have applied clang-format to the whole file which has introduced quite a few formatting-only changes to your patch. The guidelines are to only clang-format the changed parts of the code, so that it is easier to see the "real" changes. See http://llvm.org/docs/GettingStarted.html#sending-patches for how to achieve this.

Otherwise, it looks fine to me, but probably best to get one of the other reviewers to give the OK too.

mvhooren updated this revision to Diff 199242.May 13 2019, 4:04 AM

Removed changes done by clang-format that are not related to this patch.

Looks good to me. Thanks so much for all your work on this Machiel!

Do you have commit access? If not I can commit on your behalf.

lhames accepted this revision.May 14 2019, 1:04 PM

Formally accepting so Phab does not gripe at me when this gets committed. :)

This revision is now accepted and ready to land.May 14 2019, 1:04 PM

Looks good to me. Thanks so much for all your work on this Machiel!

Do you have commit access? If not I can commit on your behalf.

Thanks for reviewing, and thanks to andrewng as well, sorry about the newbie mistakes.

No, I don't have commit access so if you could commit, that would be great.

lhames closed this revision.May 20 2019, 1:57 PM

Committed in r361195.