Size returned by slapi_entry_size is not accurate

[389-ds-bot]

Cloned from Pagure issue: https://pagure.io/389-ds-base/issue/47677

• Created at 2014-01-18 06:35:25 by nhosoi (@nhosoi)
• Closed as Fixed
• Assigned to nhosoi (@nhosoi)
• Associated bugzillas
  • https://bugzilla.redhat.com/show_bug.cgi?id=1064006
  • https://bugzilla.redhat.com/show_bug.cgi?id=1057805

---

1. slapi_dn_size implemented as a static function in entry.c is obsolete.
   It can be replaced with a slapi api slapi_sdn_get_size.
2. size of e_virtual_lock could be added to the size.

[389-ds-bot]

Comment from nhosoi (@nhosoi) at 2014-01-18 06:47:13

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

[389-ds-bot]

Comment from rmeggins (@richm) at 2014-01-20 22:57:42

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.

[389-ds-bot]

Comment from nhosoi (@nhosoi) at 2014-01-25 05:40:38

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

[389-ds-bot]

Comment from nhosoi (@nhosoi) at 2014-02-11 04:07:52

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.

[389-ds-bot]

Comment from nhosoi (@nhosoi) at 2014-02-11 04:08:59

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

[389-ds-bot]

Comment from rmeggins (@richm) at 2014-02-11 04:12:29

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.

[389-ds-bot]

Comment from nhosoi (@nhosoi) at 2014-02-11 04:19:33

Replying to [comment:8 richm]:

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?

[389-ds-bot]

Comment from rmeggins (@richm) at 2014-02-11 05:50:34

Replying to [comment:9 nhosoi]:

Replying to [comment:8 richm]:

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.

[389-ds-bot]

Comment from nhosoi (@nhosoi) at 2014-02-11 06:12:16

Replying to [comment:10 richm]:

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);

[389-ds-bot]

Comment from nhosoi (@nhosoi) at 2014-02-11 08:03:53

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-bot]

Comment from nhosoi (@nhosoi) at 2017-02-11 23:10:22

Metadata Update from @nhosoi:

• Issue assigned to nhosoi
• Issue set to the milestone: 1.2.11.26