Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a drawin crash, a drawin memory leak and a wallpaper error. #3880

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Elv13
Copy link
Member

@Elv13 Elv13 commented Dec 30, 2023

I doubt anybody ever noticed this, but it existed.

It is possible for that variable to not be set if `awful` was
never loaded.
Copy link

codecov bot commented Dec 30, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (cb72c0a) 91.01% compared to head (3fa9c66) 91.01%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3880   +/-   ##
=======================================
  Coverage   91.01%   91.01%           
=======================================
  Files         901      902    +1     
  Lines       57566    57622   +56     
=======================================
+ Hits        52392    52447   +55     
- Misses       5174     5175    +1     
Flag Coverage Δ
gcov 91.01% <95.31%> (+<0.01%) ⬆️
luacov 93.72% <94.91%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
lib/wibox/drawable.lua 98.88% <100.00%> (+<0.01%) ⬆️
objects/drawable.c 100.00% <100.00%> (ø)
objects/drawin.c 88.00% <100.00%> (+0.04%) ⬆️
tests/test-leaks-wibox.lua 100.00% <100.00%> (ø)
lib/wibox/init.lua 94.49% <72.72%> (-0.34%) ⬇️

... and 2 files with indirect coverage changes

There was no "real" unsolvable GC issues, but at least for Lua
5.2 and 5.3, it wasn't possible to GC a wibox anymore. This
commit unwind the circular references with 3 new weak refs. This
is enough to get a specific test to pass on Lua 5.3. It's voodoo,
but this was actually a pretty bad leak. There's other according
to some test I wrote, but that's one.

As for to_string, it appears to be accidental refactoring oversight.
When a drawable has a pending redraw in a delayed call and the
drawin is GCed (which was impossible because it was broken
until the last commit), it crashes.
When you run this outside of the test runner, it works, but not
with it. Anyway, it adds the regression tests for the previous
crash, so it's worth having.
Copy link
Member

@actionless actionless left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it was actually some reports about weird un- investigated/confirmed leaks

and myself sometimes notice some leakage on awesome proc but i'm not sure how to reproduce that - so neither can tell for sure if they're gone with this PR

but seeing the test gives a good feeling about this, even if it's not addressing aforementioned leaks

@sclu1034
Copy link
Contributor

sclu1034 commented Jan 4, 2024

and myself sometimes notice some leakage on awesome proc

Confirmed leakage, as in something like Valgrind confirmed that the address was dropped without a free()? Because merely observing a slow increase in memory usage over time via system monitoring is not a confirmation of a leak.

More likely, it would be the same issue as everything else reported as an alleged leak, including the 3-4 issues that are still open:

Lua doesn't have the facilities to notify its GC of how big a memory region in a pointer is. I.e. if you have a userdata that contains a pointer to a big memory chunk (such as a Cairo surface pointing to image data), Lua only knows about the size of the userdata struct, which would be 64 bit for the pointer. Lua doesn't know, and cannot be told about, the kilo- or megabytes of data that the pointer actually contains, because they were not allocated by Lua.
You'd have to do all memory allocation through lua_newuserdata, but that's not compatible with the internal memory management of GLib and other libraries.

Example:
With the default settings for the garbage collector, the allocated memory (as known to the GC) needs to double for the next cycle to start.
If the GC thinks that the current usage after the last cycle is 50 MB, then it would take ~800000 of those 64 bit userdata values to double the usage and trigger the next cycle. But if those values each point to 512 KB of memory, you'd also have about 400 MB that the GC isn't aware of, bringing the total to 500 MB.

This is easily verifiable by adding a GIdleSource, or a short timer, that forces full GC cycles (collectgarbage("collect")) constantly. If your memory usage stops growing with that, the issue is as described, and not a leak (leaked memory, by definition, would not be reachable by the GC).

@actionless
Copy link
Member

@sclu1034 if you wanna discuss each of them - i guess better place would be there (https://github.com/awesomeWM/awesome/issues?q=is%3Aissue+is%3Aopen+memory+leak+)

@actionless

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants