git patch file (master)
0001-Ticket-47677-Size-returned-by-slapi_entry_size-is-no.patch

I notice that slapi_dn_size does not include the sizeof(Slapi_DN) but slapi_sdn_get_size() does. Is this intentional? It seems to me that including sizeof(Slapi_DN) is correct but I don't know if slapi_entry_size had some reason for not including it.

Also, I'm not sure if sizeof(Slapi_RWLock) is the total amount of memory allocated by slapi_rwlock_new(). I think it is, but if it isn't, we don't have an easy way to calculate it.

I also note that slapi_entry_size does not take into consideration the following fields:
void e_extension;
Slapi_Attr e_aux_attrs;

But I think we should investigate this in another ticket.

Ticket has been cloned to Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1057805

Description: slapi_entry_size calculating the entry size had issues.
. To calculate the Slapi_DN size, local function slapi_dn_size was used.
slapi_dn_size internally calls slapi_sdn_get_dn and slapi_sdn_get_ndn.
The calls generates normalized dn and case lowered normalized dn from
raw dn udn if the normalized dn are not stored in Slapi_DN yet. I.e.,
the get size function allocates extra memory for the normalized dn.
Local slapi_dn_size also failed to count the raw dn length. This patch
replaces slapi_dn_size with (slapi_sdn_get_size - sizeof(Slapi_DN)).
. slapi_entry_size counted Slapi_RDN twice.
. slapi_entry_size did not count the size of e_virtual_lock, e_aux_attrs
and e_extension.

git patch file (master; take 2) -- added the size of e_extension
0001-Ticket-47677-Size-returned-by-slapi_entry_size-is-no.2.patch

Replying to [comment:7 nhosoi]:

Description: slapi_entry_size calculating the entry size had issues.
. To calculate the Slapi_DN size, local function slapi_dn_size was used.
slapi_dn_size internally calls slapi_sdn_get_dn and slapi_sdn_get_ndn.
The calls generates normalized dn and case lowered normalized dn from
raw dn udn if the normalized dn are not stored in Slapi_DN yet. I.e.,
the get size function allocates extra memory for the normalized dn.

If we want an accurate size in memory, then I think we need to account for that. Let's say someone creates the entry in the cache with the "raw" DN. The size is calculated at the time it is added to the cache, without the normalized DN. Sometime later, someone calls slapi_sdn_get_ndn() on that entry which changes the entry. Now, the size of the cache is not correct because the cache size does not take into account the size of the normalized dn.

Local slapi_dn_size also failed to count the raw dn length. This patch
replaces slapi_dn_size with (slapi_sdn_get_size - sizeof(Slapi_DN)).
. slapi_entry_size counted Slapi_RDN twice.
. slapi_entry_size did not count the size of e_virtual_lock, e_aux_attrs
and e_extension.

Replying to [comment:8 rmeggins]:

Replying to [comment:7 nhosoi]:

Description: slapi_entry_size calculating the entry size had issues.
. To calculate the Slapi_DN size, local function slapi_dn_size was used.
slapi_dn_size internally calls slapi_sdn_get_dn and slapi_sdn_get_ndn.
The calls generates normalized dn and case lowered normalized dn from
raw dn udn if the normalized dn are not stored in Slapi_DN yet. I.e.,
the get size function allocates extra memory for the normalized dn.

If we want an accurate size in memory, then I think we need to account for that. Let's say someone creates the entry in the cache with the "raw" DN. The size is calculated at the time it is added to the cache, without the normalized DN. Sometime later, someone calls slapi_sdn_get_ndn() on that entry which changes the entry. Now, the size of the cache is not correct because the cache size does not take into account the size of the normalized dn.

It makes sense. On the other hand, we want to avoid calling malloc as much as possible... :) I could add a function slapi_sdn_get_maxsize, which returns 3 * existing DN (regardless of normalized or raw). Do we prefer that way?

Replying to [comment:9 nhosoi]:

Replying to [comment:8 rmeggins]:

Replying to [comment:7 nhosoi]:

Description: slapi_entry_size calculating the entry size had issues.
. To calculate the Slapi_DN size, local function slapi_dn_size was used.
slapi_dn_size internally calls slapi_sdn_get_dn and slapi_sdn_get_ndn.
The calls generates normalized dn and case lowered normalized dn from
raw dn udn if the normalized dn are not stored in Slapi_DN yet. I.e.,
the get size function allocates extra memory for the normalized dn.

If we want an accurate size in memory, then I think we need to account for that. Let's say someone creates the entry in the cache with the "raw" DN. The size is calculated at the time it is added to the cache, without the normalized DN. Sometime later, someone calls slapi_sdn_get_ndn() on that entry which changes the entry. Now, the size of the cache is not correct because the cache size does not take into account the size of the normalized dn.

It makes sense. On the other hand, we want to avoid calling malloc as much as possible... :) I could add a function slapi_sdn_get_maxsize, which returns 3 * existing DN (regardless of normalized or raw). Do we prefer that way?

I don't know. Worst case, you have 10 million entries that you overestimate by 100 bytes each. That's a lot of memory that cannot be used.

In most cases, we are going to have to use the normalized DN, right? So, better to take the malloc hit when adding to the cache rather than later.

Replying to [comment:10 rmeggins]:

I don't know. Worst case, you have 10 million entries that you overestimate by 100 bytes each. That's a lot of memory that cannot be used.

In most cases, we are going to have to use the normalized DN, right? So, better to take the malloc hit when adding to the cache rather than later.

Right. And it looks I did not have to emphasize the loose ndn generation... Before adding an entry to the entry cache, it anyway generates the ndn (line 1261), then calculates the size (line 1261). So, it's guaranteed ndn exists before calling slapi_entry_size via cache_entry_size.
{{{
1256 entrycache_add_int(struct cache cache, struct backentry e, int state,
1257 struct backentry *alt)
1261 const char ndn = slapi_sdn_get_ndn(backentry_get_sdn(e));
1277 entry_size = cache_entry_size(e);
}}}

Reviewed by Rich (Thank you!!)

Pushed to master:
22a8572..b1e4cdf master -> master
commit b1e4cdf

Pushed to 389-ds-base-1.3.2:
476a193..7b08429 389-ds-base-1.3.2 -> 389-ds-base-1.3.2
commit 7b08429

Pushed to 389-ds-base-1.3.1:
ae2e7f4..baa3df4 389-ds-base-1.3.1 -> 389-ds-base-1.3.1
commit baa3df4

Pushed to 389-ds-base-1.2.11:
57199b3..346d366 389-ds-base-1.2.11 -> 389-ds-base-1.2.11
commit 346d366

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/1014

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.