Created attachment 74957 [details] [review]
Replacements for EMemChunk in CamelObject

Created attachment 74958 [details] [review]
Replacements for EMemChunk in CamelFolderSummary

Created attachment 75014 [details] [review]
Replacements for EMemChunk in CamelException

Created attachment 75015 [details] [review]
Removed some now unneeded pointers from the CamelObject type

This one actually reduces memory usage a little bit.

Created attachment 75064 [details] [review]
This patch attempts to replace the many different ways of allocating a CamelFolderInfo with the gslice allocator.

This patch attempts to replace the many different ways of allocating a
CamelFolderInfo with the gslice allocator.

I haven't measured yet whether this change is significant in terms of
memory consumption. I can imagine it's going to be a little bit faster.
Most people don't have thousands of folders, so maybe it's not going to
make a huge difference in memory consumption.

However. My opinion is that people should be using "the same" way for
allocating a specific structure. I've seen at least three different
methods in the code (g_malloc0, g_new0 and a _new). Sounds like a
constructor function is needed here?

It's a diff against my own version, but in that version not much has
changed in the affected files. If you need assistance applying the patch
on a original Camel, ask. It's a unified diff, so I don't think you'll
have a lot troubles.

Note that the CamelPOP3FolderInfo's instance allocating isn't changed,
as that structure seems to be totally different. Some other providers
seemed to reuse the CamelFolderInfo infrastructure, so I've let them too
use the gslice allocs.

Created attachment 75067 [details] [review]
Moved it to a constructor function, fixed a alloc vs. alloc0 issue

g_slice_new0 in stead of g_slice_new (the existing code assumes this), and moved the allocs into a constructor function.

Created attachment 75255 [details] [review]
Proposed patch for evolution-data-server

I have taken the relevant parts of Philip's patches as well as my own work and rolled them into a new patch against CVS HEAD.  This patch obsoletes all previous patches here, but I lack permission to mark them as such.

A few comments:

- This patch adds modifications to ETrie, ESExp, and CamelFolderThread to
  eliminate EMemChunk usage.  EStrv and EPoolv also use EMemChunk, but I
  have already proposed their removal in bug #363965.  Therefore I am
  making this bug depend on it.

- Building tree structures with the slice allocator tricky.  You have to
  manually chain the allocated nodes together so they can be freed at once
  using g_slice_free_chain().  I had to modify several structures to allow
  for this.

- Some tree structure APIs have functions for freeing an individual node.
  Freeing individual nodes in an allocation chain is also tricky.  You either
  have to walk the allocation chain looking for the node to unlink, or make
  the chain bi-directional and deal with all the corner cases that presents.
  My approach is to not actually free the node, but just leave it on the
  allocation chain to be freed along with the parent structure.

- The allocation locks in some of the Camel structs are no longer necessary,
  since the slice allocator is thread-safe.

Sorry, my first comment was supposed to reference bug #363695.

Created attachment 75256 [details] [review]
Proposed patch for evolution

There's one case of EMemChunk usage in Evolution.  I'm not sure why this code isn't using ETrie, but the changes here are the same as for ETrie.

Hey Matthew, I found a few compilation glitches in the patch. Other than that, most looks good to me (but I'm not a patch reviewer). I have merged your memchunk replacements with my version of Camel too. Everything seems to work, great and thanks.

It would, however, be nice to get a small mention in the ChangeLog :). I once had to prove some contribution that I did for Camel, and I can tell you it's not fun to have to do that :(

Other than that, I don't care a lot about ChangeLogs and other blabla.

Anyway, thanks a lot for picking up the patches and bringing them to HEAD.

Philip, can you please post either a compilation log showing the glitches or a revised patch that fixes them?  I'm trying not to introduce any new compiler warnings or other no-no's; there's enough of them already.

Maybe those where caused by the merge or a manual glitch that sneaked in. I for example remember having a variable with a name like threan whereas the declaration of it was thread. It was something that doesn't compile.

So if you aren't seeing it, it's most likely a merge problem.

Created attachment 75433 [details] [review]
GSlice replacement for camel-mime-utils.c

This isn't a EMemChunk replacement, but a g_malloc(sizeof(*p)) one.

Philip: IIRC, Federico did a study on using GSlice for EMemChunk and found not-so-impressive results.  I hadn't been to GUADEC , so do not have much detail on it.  Adding Federico to the CC list. However, your GSlice replacement looks good.

http://primates.ximian.com/~federico/news-2006-06.html#evo-gslice

Just to clarify, Philip and I are proposing to deprecate EMemChunk in favor of GSlice, not make EMemChunk use GSlice internally.  Also, as I mentioned in comment #7, rather than using a GHashTable to keep track of allocated buffers, I added a field to the allocated structs themselves so that I could chain them together and free them at once using g_slice_free_chain().  This approach applies primarily to tree structures.  I imagine that's much faster than using a GHashTable, though I haven't taken any measurements myself.

Also, in bug #363695 I proposed replacing all of the EMemPools with GStringChunks, since they are almost always used to store strings.

The comment above was supposed to read:

...using a GHashTable to keep track of GROUPS OF allocated buffers...

I'd love to see some comparisons before/after these changes as far as:

1. memory usage
2. performance


I suspect your GSlice changes will be a win in both cases, but I'd love to see actual measurements :)

Comment #14 and #15: Indeed. Whereas Federico replaced the implementation of GMemChunk, we are replacing it in the code itself. I so far have only seen memory improvements (less heap-admin than the EMemChunk, but I might be measuring it wrong).

I haven't measured performance in-depth. I'm assuming GSlice is faster but I don't have any proof at this moment.

I agree with Jeffrey that we should measure it. EMemChunk had a nice feature that makes it possible remove all instances at once. As Matthew told you can mimic such a feature. That is indeed a performance-thingy/difference with EMemChunk. Especially, for example, for the CamelFolderSummary's message-info instances on larger folders.

On Evolution, however, folders are never freed (until shutdown of the application). So for Evolution, performance wouldn't differ. For tinymail, it does differ. The current GSlice implementation (after patched) of CamelFolderSummary uses the GPtrArray api to cleanup all instances. This is indeed slower than the old EMemChunk technique. Replacing that with the GLsice-foreach thingy or something faster, is on my todo list.

Did anyone ever do a comparison with numbers for these?

I haven't yet.  Philip?

ping: Any measurements to see the improvements? Lets see it before it becomes too late.

Setting patch status from comment #14

The big difference between GSlice and EMemChunk in terms of performance is in the fact that you can destroy an entire memchunk with one call, whereas you have to do this instance per instance with gslice.

GSlice, when destroying the entire bank (which happens mostly with and in CamelFolderSummary's CamelMessageInfo instances), takes significantly more time than EMemChunk.

On heap-admin GSlice is the winner. I saw significant improvements on memory usage itself. On allocation speed I haven't done a lot measuring, I'm thinking that GSlice will be faster.

Deallocation in Evolution happens far less than allocation (folders are kept open for the entire lifespan of the application). I think, therefore, that GSlice will be a winner for Evolution's case.

For Tinymail I'm in favor of GSlice because I care more about memory usage than raw performance right now. Although the performance hit on deallocation is, indeed, measurable. My opinion on performance is usually that micro optimization is the last thing to try, whereas with memory its my opinion that everything that is quantitative (like all the things that you want to use GSlice or EMemChunk for), needs to be as optimal as possible (hence my favor for GSlice).

(In reply to comment #22)
> The big difference between GSlice and EMemChunk in terms of performance is in
> the fact that you can destroy an entire memchunk with one call, whereas you
> have to do this instance per instance with gslice.

Unless you chain the instances together by embedding a "next" pointer into the allocated structure and use g_slice_free_chain().  I tried to do this with the ETrie patch.  See comment #7.

Also, destroying an entire memchunk with "one call" involves looping over the structure and calling g_free() a bunch of times anyway.  So I suspect there wouldn't be much of a performance difference between e_memchunk_destroy() and g_slice_free_chain().  Can't tell for sure without real data though, obviously.

EMemChunk should alloc and dealloc individual items faster than GSlice I think (since it is typically pointer arith or a simple list pop), but EMemChunk will not shrink its memory pool unless explicitly requested (or until the EMemChunk is destroyed)

stats on this change would be nice to have...

I'm marking the rest two patches as reviewed to get rid of this bug from the unreviewed patches filter. They are outdated for actual sources anyway.

GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. 
As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled).

If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow
  https://wiki.gnome.org/Community/GettingInTouch/BugReportingGuidelines
and create a new ticket at
  https://gitlab.gnome.org/GNOME/evolution-data-server/-/issues/

Thank you for your understanding and your help.