Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add goodAllocationSize to GCAllocator #3962

Merged
merged 3 commits into from
Feb 2, 2016

Conversation

schveiguy
Copy link
Member

GC always allocates from bins, and has very well-defined behavior here. So goodAllocSize can be implemented quite easily.

Rules:

  1. If allocation size < 16, allocate 16 bytes
  2. For allocations of 16 to 8192, use the power of 2 size that will hold it.
  3. For allocations greater than 8192, use the multiple of 4096 that will hold it.

I had to change some other unit tests because of the assumptions made for the default implementation of IAllocator.goodAllocSize

import core.bitop: bsr;

auto largestBit = bsr(n);
if (largestBit < 4) // less than 16
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should probably return before the bsr here.

@schveiguy
Copy link
Member Author

Updated for better bsr usage.

@JakobOvrum
Copy link
Member

This probably doesn't jive well with the fact that the GC is pluggable at link-time, but apart from that I guess it looks good to me.

@schveiguy
Copy link
Member Author

Well, it may be inaccurate. It wouldn't be catastrophic.
We could also provide a hook for this if there was a request for it.

@JakobOvrum
Copy link
Member

I think it's fine. Maybe when this is merged we can add a comment to gc.d informing the programmer of the assumptions made in std.allocator, so the issue can be dealt with when relevant.

@MartinNowak
Copy link
Member

The GC bins are wasting quite some space (so should the should be replaced) and it's fairly ugly to hardcode them here.

@schveiguy
Copy link
Member Author

@MartinNowak GCAllocator reports basically that you will waste no space by allocating any size (it just rounds up to the next multiple of 16). I don't see the point of having this be incorrect, as there is no way to query an allocator for the actual memory size you just created.

This is not a confirmation of the implementation of the GC, it's just trying to have GCAllocator accurately report the current situation. If you think the implementation should be done in a different way, then we can fix it when it makes sense. At this point, I don't see a lot of reason to add functions to the GC to satisfy something in std.experimental.

assert(GCAllocator.instance.goodAllocSize(s) == s);
assert(GCAllocator.instance.goodAllocSize(s - (s / 2) + 1) == s);

auto buffer = GCAllocator.instance.allocate(s);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't be better to request s - (s / 2) + 1 here instead of s? That way you do make sure that the allocator gets you the extra bytes up to s.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can probably do both :) Not that hard to add a couple lines.

One thing I was concerned about actually, is having the GC insert extra metadata (such as array used size) that would bump the size up to the next bin.

@andralex
Copy link
Member

andralex commented Feb 2, 2016

Yah, I agree with @MartinNowak's point but am not worried about changes. I've never heard of plugging the GC during link time, and the moment we replace the strategy in the GC, the Phobos unittests will fail prompting change.

@schveiguy
Copy link
Member Author

Added additional unit test

@andralex
Copy link
Member

andralex commented Feb 2, 2016

Nice, thanks! Let's hope it clears the unittest :o).

@andralex
Copy link
Member

andralex commented Feb 2, 2016

Auto-merge toggled on

andralex added a commit that referenced this pull request Feb 2, 2016
Add goodAllocationSize to GCAllocator
@andralex andralex merged commit 4be6bc1 into dlang:master Feb 2, 2016
@schveiguy schveiguy deleted the gc_goodAllocSize branch February 3, 2016 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants