Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

Fix issue 2504 - AA.reserve #1929

Closed
wants to merge 1 commit into from
Closed

Fix issue 2504 - AA.reserve #1929

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/object.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1931,6 +1931,7 @@ extern (C)
inout(void)[] _aaKeys(inout void* p, in size_t keysize, const TypeInfo tiKeyArray) pure nothrow;
void* _aaRehash(void** pp, in TypeInfo keyti) pure nothrow;
void _aaClear(void* p) pure nothrow;
void _aaReserve(void** p, const TypeInfo_AssociativeArray ti, size_t ndim) pure nothrow;

// alias _dg_t = extern(D) int delegate(void*);
// int _aaApply(void* aa, size_t keysize, _dg_t dg);
Expand Down Expand Up @@ -1974,6 +1975,16 @@ void clear(T : Value[Key], Value, Key)(T* aa)
_aaClear(*cast(void **) aa);
}

void reserve(T : Value[Key], Value, Key)(ref T aa, size_t ndim)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add an overload for T* as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

Choose a reason for hiding this comment

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

Ouch do we really need this T* for new overloads? After all aren't we trying to move away from the ugly C-like pointer syntax?
Also while you are at it, note that most AA methods still aren't @safe :/

Copy link
Member

Choose a reason for hiding this comment

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

@wilzbach yes, it needs to be there. Otherwise it operates inconsistently with other normal objects that can call "members" based on a pointer.

This is a special case because AA's are builtins. If they were fully library types, this wouldn't be needed.

Copy link
Member

Choose a reason for hiding this comment

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

sux but ok

{
_aaReserve(cast(void **)&aa, typeid(Value[Key]), ndim);
Copy link
Member

Choose a reason for hiding this comment

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

You can use typeid(T) here since T == Value[Key].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, however I used Value[Key] to be consistent with the rest of the file.

Copy link
Member

Choose a reason for hiding this comment

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

There are a few kinks we could do better in this area, si @Darredevil let's go with the cleaner way here and we'll update the rest in good time.

}

void reserve(T : Value[Key], Value, Key)(T* aa, size_t ndim)
{
reserve(*aa, ndim);
}

T rehash(T : Value[Key], Value, Key)(T aa)
{
_aaRehash(cast(void**)&aa, typeid(Value[Key]));
Expand Down
51 changes: 51 additions & 0 deletions src/rt/aaA.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,6 +474,23 @@ extern (C) void _aaClear(AA aa) pure nothrow
}
}

/// Reserve AA
extern (C) void _aaReserve(AA* aa, const TypeInfo_AssociativeArray ti, size_t ndim)
{
ndim = nextpow2(ndim);
Copy link

Choose a reason for hiding this comment

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

The buckets size used should be large enough that if ndim elements are added, the bucket array will not be grown further (ndim function arg value). However, the choices here don't achieve that. Specifically, the bucket array will be grown if it becomes 80% full (GROW_NUM/GROW_DEN). nextpow2(ndim) will not always need this requirement. It will sometimes be necessary to move one power of two further to achieve this. e.g. If the arg is 500, the bucket size selected should be 1024, not 512. I suggest testing for this after the ndim = nextpow2(ndim) line, if it doesn't meet it, find the next power of two. That should handle the first two cases (aa.impl is null, ndim <= aa.dim). For the third case tested (aa.used * GROW_DEN > ndim * GROW_NUM), it is less clear to me when that would get triggered.

Copy link
Member

Choose a reason for hiding this comment

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

I think applying ndim = (ndim * GROW_DEN + GROW_NUM - 1) / GROW_NUM; before finding the next power of 2 should be good enough.
I'm not 100% sure, but you might also have to take the deleted entries into account when comparing with aa.dim or aa.used.

Copy link
Member

Choose a reason for hiding this comment

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

One thing to be cautious of is that reserve should never remove buckets. Due to the logic below, it won't, but if the logic is moved around, it could easily happen.

Copy link

@jondegenhardt jondegenhardt Oct 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Regarding the power-of-two allocation sizes: Best I can tell all the existing bucket allocation code paths use nextpow2 to set the bucket size when allocating. Not clear if that is important. If it is, the

ndim = (aa.used * GROW_DEN + GROW_NUM - 1) / GROW_NUM;

line is not following it. However, it the power-of-two component is not important, there may be an opportunity with reserve. Assuming that a call to reserve is a hint about expected element count, it might be better to choose the size based on fill ratio rather than power-of-two. For example, reserving 7 million entries will pick a power-of-two of 16777216, a 42% fill ratio. A 70% fill ratio of 10 million would save space and improve locality of reference.

Update: findSlotLookup and findSlotInsert identify the slot based on a mask created by buckets.length - 1, so it looks like power-of-two is expected. The code line in reserve should probably be changed to conform.

// lazily alloc implementation
if (aa.impl is null)
aa.impl = new Impl(ti, ndim);
else
{
if (ndim <= aa.dim) return;
if (aa.used * GROW_DEN > ndim * GROW_NUM)
ndim = (aa.used * GROW_DEN + GROW_NUM - 1) / GROW_NUM;
assert(aa.used * GROW_DEN <= ndim * GROW_NUM);
aa.resize(ndim);
}
}

/// Rehash AA
extern (C) void* _aaRehash(AA* paa, in TypeInfo keyti) pure nothrow
{
Expand Down Expand Up @@ -1013,3 +1030,37 @@ unittest
assert(typeid(a).getHash(&a) == typeid(a).getHash(&a));
assert(typeid(a).getHash(&a) == typeid(a).getHash(&a2));
}

// test AA.reserve
unittest
{
int[int] aa;
assert(aa is null);

aa.reserve(2000);
assert(aa !is null);

auto paa = &aa;
paa.reserve(3000);

foreach(i;0..2000) aa[i] = i;
assert(aa[1337] == 1337);

// stress test
aa.clear();
aa.reserve(3_000_000);

auto impl = (cast(AA*)&aa);
auto buckets = impl.buckets;

assert(buckets.length >= 2_000_000);

foreach(i;0..2_000_000) aa[i] = i;
assert(aa[1_999_133] == 1_999_133);

// pointer should be the same if no rehashing occurs
assert((cast(AA*)&aa).buckets == buckets);

// try to reserve less than current length
aa.reserve(10);
}