[Geany-Devel] On document pointer recycling

[Lex Trotman]

On 24 October 2013 18:39, Thomas Martitz <
thomas.martitz at 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.


>
> The is_valid thing is giving false security. Because a doc becomes valid
> again after it was recycled you cannot be sure that the document is still
> the same. Yet DOC_VALID() gives a false positive here if code missed the
> document-swap behind the scenes. So basically code that cares about
> GeanyDocument cannot rely on DOC_VALID() anyway.
>

Indeed, it only ensures that a doc is valid, not that its the *same*
document.


> The _only_ way to make sure things are alright is to cleanup per-document
> stuff in the "document-close" signal handler. If you don't connect to this
> signal your per-document code is broken, regardless of doc->is_valid. Even
> worse, this brokenness is hidden by valid doc pointers, whereas it would
> otherwise crash and suggest debugging the problem properly. The right way
> is to NULL'ify any cached document pointer in the signal-handler which
> makes recycling pointers unnecessary.
>

Yes, just don't use pointers to closed documents.


>
> Also, recycling the pointers is only possible because the document array
> never shrinks. So if you had opened a large number of documents and close
> many of them you waste a lot of memory.
>

Yes, I don't think the designers had people opening hundreds of documents
in mind. :)  Although in the end the waste isn't all that great, but it
doesn't need to happen.


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

Cheers
Lex


> Best regards.
> ______________________________**_________________
> Devel mailing list
> Devel at lists.geany.org
> https://lists.geany.org/cgi-**bin/mailman/listinfo/devel<https://lists.geany.org/cgi-bin/mailman/listinfo/devel>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.geany.org/pipermail/devel/attachments/20131024/b06b5216/attachment.html>