Bug 12174 - DBusMessageLoader is wasteful
Summary: DBusMessageLoader is wasteful
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Havoc Pennington
QA Contact: John (J5) Palmieri
Depends on:
Reported: 2007-08-26 17:13 UTC by Allison Lortie (desrt)
Modified: 2007-09-19 21:15 UTC (History)
0 users

See Also:
i915 platform:
i915 features:

implement _dbus_string_compact() and use it (6.00 KB, patch)
2007-08-27 10:40 UTC, Allison Lortie (desrt)
Details | Splinter Review

Description Allison Lortie (desrt) 2007-08-26 17:13:32 UTC
i was looking into the memory use of dconf-server today and i discovered that dbus keeps a chunk of memory allocated that is approximately equal to the largest message that it has received.

this is because the 'data' DBusString that is stored as part of the DBusMessageLoader has _dbus_string_delete() called on it after the DBusMessage has been extracted.  _dbus_string_delete() makes no attempt to reclaim memory from the shrinking string.

seeing as it is possible to receive a 64MB message, this should be fixed.
Comment 1 Havoc Pennington 2007-08-26 19:50:25 UTC
something like:

 if (_dbus_string_get_length(data) > 2048)

would avoid malloc churn for normal-size messages, though it probably doesn't matter.
Comment 2 Allison Lortie (desrt) 2007-08-26 20:40:35 UTC
in DBusString or in DBusMessageLoader?
Comment 3 Havoc Pennington 2007-08-27 06:42:02 UTC
In the loader. Having the string "self compact" might be wrong in some cases, and there's various code that assumes the string never spontaneously reallocs smaller (and thus that using previously-allocated space can't fail due to OOM)

There's some kind of function on dbus string that reallocs smaller to fit the current content already, I don't remember its exact semantics.

Comment 4 Allison Lortie (desrt) 2007-08-27 10:40:12 UTC
Created attachment 11290 [details] [review]
implement _dbus_string_compact() and use it
Comment 5 Havoc Pennington 2007-08-27 10:58:18 UTC
Looks good, thanks. I would take a couple minutes to add a test case for two code paths ("there was more than max_waste wasted" and "there wasn't"), in dbus-string-util.c:_dbus_string_test()

This is fine to commit otherwise. (don't remember if you have access, if you don't then it would be good to ask a fd.org admin to add you to the dbus group)
Comment 6 John (J5) Palmieri 2007-08-27 11:07:32 UTC
I think it is time for another development release.  Get this into head and I will roll one.  I'm guessing this should go into stable too?
Comment 7 Havoc Pennington 2007-08-27 13:35:54 UTC
this is fine for stable and devel, though I don't think it's critical for stable. hopefully we'll have a 1.2 relatively soon (not sure we should block on getting around to reviewing all those portability patches, though it sucks not to)

If you're rolling a release anyway you can probably just commit this for ryan if he has no account.

Comment 8 Allison Lortie (desrt) 2007-08-27 15:01:59 UTC
i'm waiting to be added to the group, but no point in waiting.

you can go ahead and do this without me.

Comment 9 Allison Lortie (desrt) 2007-09-19 21:15:06 UTC
committed to git

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.