Skip to content
This repository has been archived by the owner on Jan 16, 2023. It is now read-only.

Fix a memory leak occuring whenever win_res() is called #372

Closed
wants to merge 1 commit into from

Conversation

stevenyvr987
Copy link

win_res() is called multiple times when sxiv initializes, whenever an Xresource value is needed. For each call, win_res() creates an Xresources database from a specified window display.

  1. First, it calls XrmInitialize() to initialize the Xresources manager
  2. then calls XResourceManagerString() to get a pointer to the Xrm
  3. then calls XrmGetStringDatabase() with the pointer to get a pointer to a new Xresources database
  4. and finally, calls XrmGetResource() with the database pointer to get the XrmValue.

The leak occurs because win_res() stores the database pointer as an automatic and does not call XrmDestroyDatabase() before the pointer goes out of scope. The obvious fix is to call XrmDestroyDatabase() before returning. But this is not performant; since win_res() is called multiple times, we should avoid creating and destroying a database for each call.

The proposed fix is to have win_res() store the Xresources manager pointer and the database pointer as local statics. win_res() calls XrmInitialize(), XResourceManagerString(), and XrmGetStringDatabase() once, if the Xresources manager pointer has never been set. Thereafter, win_res() calls XrmGetResource() with the static database pointer for whatever value is needed. The end result is the database is created once and all memory will be collected when sxiv quits.

If XResourceManagerString() returns a null pointer, win_res() will continue to try to create a database for each call, but this will not be worse than the current situation.

If XrmGetStringDatabase() returns a null pointer, win_res() will return the given default value, as before, but a ?: operator is used instead of an if-else statement.

win_res() is called multiple times when sxiv initializes, whenever an Xresource value is needed. For each call, win_res() creates an Xresources database from a specified window display.
. First, it calls XrmInitialize() to initialize the Xresources manager
. then calls XResourceManagerString() to get a pointer to the Xrm
. then calls XrmGetStringDatabase() with the pointer to get a pointer to a new Xresources database
. and finally, calls XrmGetResource() with the database pointer to get the XrmValue.

The leak occurs because win_res() stores the database pointer as an automatic and does not call XrmDestroyDatabase() before the pointer goes out of scope. The obvious fix is to call XrmDestroyDatabase() before returning. But this is not performant; since win_res() is called multiple times, we should avoid creating and destroying a database for each call.

The proposed fix is to have win_res() store the Xresources manager pointer and the database pointer as local statics. win_res() calls XrmInitialize(), XResourceManagerString(), and XrmGetStringDatabase() once, when the Xresources manager pointer has never been set. Thereafter, win_res() calls XrmGetResource() with the static database pointer for whatever value is needed. The end result is the database is created once and all memory will be collected when sxiv quits.

If XResourceManagerString() returns a null pointer, win_res() will continue to try to create a database for each call, but this will not be worse than the current situation.

If XrmGetStringDatabase() returns a null pointer, win_res() will return the given default value, as before, but a ?: operator is used instead of an if-else statement.
xyb3rt added a commit that referenced this pull request Jan 16, 2020
@xyb3rt
Copy link
Owner

xyb3rt commented Jan 16, 2020

Thanks for reporting and fixing this. I used a simpler solution that does not use static variables.

@xyb3rt xyb3rt closed this Jan 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants