Am 24.10.2013 10:22, schrieb Lex Trotman:

On 24 October 2013 18:39, Thomas Martitz <thomas.martitz@student.htw-berlin.de mailto:thomas.martitz@student.htw-berlin.de> wrote:

Am 24.10.2013 05:39, schrieb Lex Trotman:



    PS On the recycling of doc structures and doc->is_valid, this
    does have the advantage (for a structure where miscellaneous
    pointers to the structure are going to exist in Geany and
    plugins) that doc pointers will always point to a
    geanydocument struct.  So the is_valid test is always right.
     If the memory was returned and re-cycled into some other
    struct, the old doc pointers could point to anything, and
    could just as easily appear a valid document.  So its safer
    than the alternative, but the requirement to check is_valid
    really does need more visibility since its an unusual idiom.


The recycling & is_valid thing is completely bogus. It is wastes
memory unnecessarily.

Agreed. I wasn't meaning to sound like I was defending it. I was just pointing out that one of its side effects.

Yes you sounded like that.

I vote for removing this idiom and transition the documents array
to a GList which gives other nice advantages.

Unfortunately there is an accessor document_index() in the plugin interface that uses the index into the array.

Switching to a list means that there needs to be a change everywhere in Geany and the plugins that the array index is used instead of a pointer. I don't see anywhere where the index is used inside Geany but I may have missed somewhere. I don't know how often its used by plugins. But again its use could be deprecated and a temporary map (eg g_tree) from an int to the pointer could be used until plugins are modified.

All the places where Geany and plugins iterate over the document list also needs to be replaced by a loop using g_list_first() and g_list_next(). I count 12 or 13 such places in Geany itself, but I don't know how many there are in plugins.

Its certainly not a one line change, but may not be huge, depending on the plugins use of the index. However I wouldn't call it a high priority change, even a thousand geanydocument structs isn't much memory compared to the amount Scintilla uses :)

Okay, but you still agree that doc->is_valid should be removed eventually? That's a step forward :)

Best regards.

Am 24.10.2013 11:24, schrieb Lex Trotman:

I currently do not have access to a dev environment where I can grab the plugins and search for places the index is used. But if its not used anywhere (or even if its not used much) then it should be deprecated immediately so nobody uses it in new code. Use G_DEPRECATED I guess, plus \deprecated in the doxygen comments, I don't know if we've deprecated anything before have we?

@Colomban, do we have a process for plugin API deprecation and removal?

Deprecation is difficult with our plugin mechanism, because all API function calls are macros that, after expansion, call via pointer. I don't think G_DEPRECATED (gcc's __attribute__((deprecated))) works via such indirect calls.

Best regards.

On 24/10/2013 10:24, Lex Trotman wrote:

...

Okay, but you still agree that doc->is_valid should be removed eventually?

...

That's a step forward:)

Of course I agree. So its not a terribly big step:)

I only skimmed the discussion, but how can we remove that?

I think freeing document memory has high potential to break things. There are a few places where Geany assumes document memory isn't freed over time.

If people don't like foreach_document (or Matt's improved C99 foreach_doc with document pointers), we could add document_get_all or something that returned a list of valid pointers. That can be done without breaking existing code. Personally I prefer Matt's suggested macro because you don't have to free the list.

On 26/10/2013 00:22, Lex Trotman wrote:

On 26 October 2013 00:01, Nick Treleaven nick.treleaven@btinternet.comwrote:

...

On 24/10/2013 10:24, Lex Trotman wrote:

...

Okay, but you still agree that doc->is_valid should be removed eventually?

...

...

That's a step forward:)

Of course I agree. So its not a terribly big step:)

I only skimmed the discussion, but how can we remove that?

Given your experience it would be good if you could read the discussion and give your perspective on what the previous posters have discussed regarding handling doc pointers properly.

I read the discussion.

In summary, I think there are 3 issues: * forgetting to check is_valid when iterating documents * freeing memory * code that uses is_valid to check if a document still exists, which is bad practice

I think freeing memory is a non-issue, I think you (Lex) agree with me on that. It's not a memory leak, it may be reused later.

The first point needs to be handled before the last one (assuming each are worth fixing). I think we can:

* remove GeanyDocument::index and fix any code that uses it * review all code indexing documents_array and fix any code retaining an index (I expect that is rarer than code using GeanyDocument::index). * change documents_array to only contain valid documents, i.e. on close we swap the last valid document pointer with the pointer being removed, and decrement the length by 1. * use a free list for invalid documents

The last point is important to ensure no memory corruption bugs are introduced. I think it is harder to find the code that retains document pointers (and there is some), so I recommend we don't free document memory. At least, we would need a better reason to do so IMO than just memory usage, which is insignificant.

Now, once we've done that it's no longer needed to check is_valid when iterating documents. Possibly we could remove is_valid, but I think that is less important to do. Note that if we do all the steps above, we don't need to break the API, only the ABI. I can't guarantee that bugs won't be introduced due to these changes, but it seems fairly reliable.

On 26/10/2013 13:17, Nick Treleaven wrote:

• remove GeanyDocument::index and fix any code that uses it
• review all code indexing documents_array and fix any code retaining an

index (I expect that is rarer than code using GeanyDocument::index).

• change documents_array to only contain valid documents, i.e. on close

we swap the last valid document pointer with the pointer being removed, and decrement the length by 1.

As Dimitar mentioned, this will affect any loops that close documents. Hopefully there aren't many of them, they would need to be fixed.

On 13-10-26 06:22 AM, Lex Trotman wrote:

On 26 October 2013 23:17, Nick Treleaven nick.treleaven@btinternet.comwrote:

...

[snip]

• use a free list for invalid documents

The last point is important to ensure no memory corruption bugs are introduced. I think it is harder to find the code that retains document pointers (and there is some), so I recommend we don't free document memory. At least, we would need a better reason to do so IMO than just memory usage, which is insignificant.

Yes, the memory is not an issue, but the real problem (not handling close) needs to be fixed wherever it occurs, and changing the implementation won't help. In fact, as I said in the PS that started this thread, the current implementation at least ensures that the pointer/index will always reference something that is a GeanyDocument object, if the document structs were de-allocated in the normal way the pointer could point to anything.

Although the chances of the wrong document being referenced because of a retained pointer are slim, any fault that can happen, will happen (and at the worst moment according to Murphy).

If the GeanyDocument were a GObject (or we added hand-rolled reference counting), the document would never get freed from under the code that uses them's backs. It's a different way of thinking of it, but it's like std::shared_ptr or any GObjects, the last person that stops needing the thing and drops its last reference, causes the GeanyDocument destructor to run which would free the memory, and you'd be 100% sure everyone that anyone referencing the document was no longer doing so by the time the actual memory gets freed. This way it ensures any weird dangling pointers or pointing to the wrong document can't happen (unless someone is holding a document pointer, without taking a reference, and without watching for its close or destroy signals, which would mean it's buggy code anyway).

It takes a different way of thinking about it, but basically "close" of a document and "free" of a document are two distinct things, although an opportune time for various document holders to drop their reference is when the document emits its "close" signal, but I could imagine some cases (like caching closed documents in a free or MRU list or something), where the reference count would never drop to zero, even though the document emitted its close signal.

This makes it a lot easier in the future to have the same document open inside multiple notebooks or windows, or even some crazy plugin thing where it's showing the document in some other type of view where it doesn't make sense for its document pointer to become invalidated even if Geany core itself has dropped its references and doesn't want to manage/show this document anymore.

This is basically the same as the ScintillaObject; parts of core or plugins are all free to retain references to it to prevent it from vanishing when the main tab showing it gets closed. I feel quite confident that this is the most sane way to manage memory/sharing of objects in C and it's endemic in G*-style code already (including all over Geany's code for other objects/widgets).

My two cents

Cheers, Matthew Brush

Am 27.10.2013 12:49, schrieb Nick Treleaven:

On 26/10/2013 20:29, Matthew Brush wrote:

...

If the GeanyDocument were a GObject (or we added hand-rolled reference counting), the document would never get freed from under the code that uses them's backs.

My concern is that we might not find all the code that assumes documents aren't freed. Some code might not be updated and would access the wrong memory. I'm not certain, but I think there may be some code that retains pointers without checking is_valid. Maybe I'm being over-cautious.

If that would be the case it is buggy now and will be buggy afterwards.

Personally I think reference counting would be OTT for this (particularly manually incremented/decremented ref counting), there doesn't seem to be a sensible reason to free document memory IMO.

I agree with Matthew, reference counting seems to be the right tool for this job.

Best regards.

On Thu, 24 Oct 2013 19:22:34 +1100 Lex Trotman elextr@gmail.com wrote:

Unfortunately there is an accessor document_index() in the plugin interface that uses the index into the array.

Switching to a list means that there needs to be a change everywhere in Geany and the plugins that the array index is used instead of a pointer. I don't see anywhere where the index is used inside Geany but I may have missed somewhere. I don't know how often its used by plugins. But again its use could be deprecated and a temporary map (eg g_tree) from an int to the pointer could be used until plugins are modified.

From what I see, document_index() is used in the plugins:

- inside foreach_document(i), which makes zero sense - in a similar loop in codenav, where the number of documents is copied in a local variable, without an obvious reason, and there is no is_valid check - in geanypy, to implement a Python document_index() function.

That being said, just how is a list better than a GPtrArray?

On 13-10-24 02:51 PM, Lex Trotman wrote:

On 25 October 2013 04:04, Dimitar Zhekov dimitar.zhekov@gmail.com wrote:

...

On Thu, 24 Oct 2013 19:22:34 +1100 Lex Trotman elextr@gmail.com wrote:

...

Unfortunately there is an accessor document_index() in the plugin

interface

...

that uses the index into the array.

Switching to a list means that there needs to be a change everywhere in Geany and the plugins that the array index is used instead of a pointer.

I

...

don't see anywhere where the index is used inside Geany but I may have missed somewhere. I don't know how often its used by plugins. But again its use could be deprecated and a temporary map (eg g_tree) from an int

to

...

the pointer could be used until plugins are modified.

From what I see, document_index() is used in the plugins:

• inside foreach_document(i), which makes zero sense
• in a similar loop in codenav, where the number of documents is copied in a local variable, without an obvious reason, and there is no is_valid check
• in geanypy, to implement a Python document_index() function.

Thanks.

...

That being said, just how is a list better than a GPtrArray?

True, its the fact that the index is held constant because it might be stored and re-used that stops the array from being shrunk. Getting rid of the use of indexes is the necessary pre-condition for shrinking the array, or using a list or whatever.

We could use GHashTable as a Set to contain the documents internally afterwards. It doesn't make much sense to have a particular document instantiated more than once anyway and the only order we care about for documents here is maybe the order/time they were opened, which is better solved with a "open timestamp" or something, IMO.

My two cents

Cheers, Matthew Brush

On 13-10-24 08:34 PM, Lex Trotman wrote:

[...]

We could use GHashTable as a Set to contain the documents internally

...

afterwards. It doesn't make much sense to have a particular document instantiated more than once anyway and the only order we care about for documents here is maybe the order/time they were opened, which is better solved with a "open timestamp" or something, IMO.

The current index does not depend on opening order since it recycles the document structs, so there can be no dependency on the current order that makes sense.

I don't follow. I didn't mean to imply the current way cares about the index order, I'm just saying since we don't care, a good data structure for that is a "set", which in GLib is done with GHashTable.

What would you use as the key to the hash/set, and why? A simple list or array is all thats needed to hold pointers to open documents and iterate them.

I'd use it, as mentioned, as a "set" data structure (unique unordered collection, like std::unordered_set in C++ or set() in Python) where the key is the document and value is the document (GHashTable has special case for use as a "set" like this to avoid extra memory allocation IIRC). It's definitively not complicated and the GHashTable API is quite nice, IMO.

Cheers, Matthew Brush