Ticket #3605 (closed enhancement: fixed)

Opened 7 years ago

Last modified 7 years ago

Donut doesn't accurately show memory usage

Reported by: AlexL Owned by: rwh
Priority: high Milestone: Update.1
Component: sugar Version:
Keywords: Update.1? review+ Cc: AlexL, walter, danw, marco
Action Needed: Verified: no
Deployments affected: Blocked By:
Blocking:

Description

Build 593 Q2C27

After resuming, closing and deleting many entries in the journal, the journal icon took up a large portion of the donut in the home view. I actually deleted everything from the journal, as we were checking to see if the files were being deleted from the datastore.

After typing "top" in the terminal, and finding the entry for the Journal, it said it was using 13.3% of the memory. However, it was taking up at least a third of the donut.

Attachments

3605-turn-off-donut.diff (0.5 kB) - added by danw 7 years ago.
patch to turn off the size calculation in the donut
fixed_activity_donut_size.diff (1.3 kB) - added by rwh 7 years ago.

Change History

Changed 7 years ago by AlexL

top values for Journal: RES 30m, SHR 10m, %CPU 0.0, %MEM 13.3

free values: Mem: total 237608, used 209680, free 108476, shared 0, buffers 0, cached 27928

Changed 7 years ago by jg

  • keywords relnote added
  • milestone changed from Untriaged to First Deployment, V1.0

Unless there is a trivial, one line sort of fix, we should defer this to after Trial-3.

Changed 7 years ago by AlexL

Also, the journal grew larger gradually as activity entries were deleted or resumed.

Changed 7 years ago by marco

  • milestone changed from First Deployment, V1.0 to Untriaged

Jim, I think we should disable the memory stuff in the donut for trial-3. danw says it will be easy...

I'm worried it will just confuse people given that it's buggy and doesn't reflect reality. (and people will think sugar is a lot more bloated than it really is).

Changed 7 years ago by jg

  • cc walter added
  • milestone changed from Untriaged to Trial-3

Marco, I agree it seems to be more of a bug than a feature right now, and will mislead people badly. Please disable for Trial-3.

Changed 7 years ago by danw

Doing "ps aux|grep Journal", deleting 10 or 20 journal entries, and then doing "ps aux|grep Journal" again shows that the Journal's memory usage according to ps *does* increase when you delete entries. Likewise, adding up just the anonymous (ie, heap memory) mappings from pmap for the journal also shows steadily increasing usage as you delete things. So the donut *is* telling the truth: the journal is steadily using more and more memory.

As for the exact percentages, ps/top's "%MEM" column measures the percentage of *total* RAM used, while the activity ring ignores the RAM used by system processes. So you shouldn't expect them to match.

That said, I can't explain why the journal would take up 1/3 of the ring with an RSS of 30M and 100M free memory. The actual measured memory usage should be less than the RSS, so it should take up less than 1/4 of the ring. So there seems to be some degree of measurement error here, and it might even be that the measurement error is getting worse as the journal uses more memory.

Changed 7 years ago by danw

patch to turn off the size calculation in the donut

Changed 7 years ago by marco

The leaks, while not being as critical as the donut would make you think, are still really bad. I started investigating it and they seem at least partially caused by pygobject, which sucks because we are using it extensively in all the code base. I'll keep investigating...

Changed 7 years ago by marco

I think this is the relevant pygobject bug: http://bugzilla.gnome.org/show_bug.cgi?id=320428 Luckily the issue seem less critical that I thought, the objects are going to be freed, if only by the cyclic gc.

Changed 7 years ago by marco

  • milestone changed from Trial-3 to First Deployment, V1.0

Checked in danw patch to disable it on the trial-3 branch.

(I'm still working on the memory leaks)

Changed 7 years ago by marco

  • type changed from defect to enhancement
  • milestone changed from First Deployment, V1.0 to Untriaged

This was disabled in Trial-3 because it was more confusing than useful. We need to figure if we want to spend resources trying to fixup the issues or we just punt the feature for FRS.

Changed 7 years ago by walter

While turning it off is probably prudent until it is more accurate, I would suggest that we increase the size of the sector each activity takes in the donut to discourage launching too many activities at once. Maybe 72 degrees per activity to suggest 5 max (including the journal)?

Changed 7 years ago by HoboPrimate

I would suggest 6. By my usage of sugar on a b4, if I stay within the 6 (including journal) limit, it allways goes fine, i.e. no activity crashes.

Changed 7 years ago by walter

6 is fine. I did notice that the second (and third...) instance of browser when launched from within the browse activity itself was no longer generating an icon in the donut--has that be sorted out since removing the memory-usage mapping? (It is important, because it is the only way to switch between windows.)

Changed 7 years ago by HoboPrimate

Oh, on second thought, perhaps 5 is best, to be conservative (and because i've just had a crash on starting Read as the 6th). Regarding the new Web window not appearing on the donut, I think there's a open ticket about that (more especifically, about how to handle pop-up windows).

Changed 7 years ago by marco

  • cc danw added

Honestly I don't have a clue about the possiblity to get this working well in 1.0. I'm not sure what it would take. Since both jg and danw seem to skeptic about it though, I think we should consider a different solution on the short time.

We agree with Walter idea and we suggest punting the real memory-usage based one to post 1.0.

Changed 7 years ago by Eben

I'm content to institute a fixed limit for now.

Changed 7 years ago by danw

http://wiki.laptop.org/go/Activity_ring has my notes on what makes this a hard problem and some ideas that might help in the future.

Changed 7 years ago by jg

  • milestone changed from Untriaged to V1.1

Changed 7 years ago by AlexL

The donut showing memory usage is enabled in Joyride (I'm on 234 right now), and I don't believe it is working properly yet.

I have browse open, and it is taking up two thirds of the donut. It's memory values from top are:

RES 47m, SHR 21m, %CPU 0.0, %MEM 20.3

If there are other values that would be good to report with these problems, please let me know.

Changed 7 years ago by marco

  • keywords Update.1? added; relnote removed
  • milestone changed from Future Release to Untriaged

There seem to be agreement on a fixed limit for Update.1.

Changed 7 years ago by jg

  • milestone changed from Untriaged to Never Assigned

Why did this get reenabled? I thought we had all agreed, and had implemented for Trial-3 and Update.1 a fixed size for each activity. Yet I see a variable size has been implemented...

Changed 7 years ago by marco

Jim, when we disabled this there was no agreement about FRS yet. It was thought as a short time workaround for Trial-3, so the patch to disable it went only in the Trial-3 branch.

Changed 7 years ago by jg

OK, we'll see how it goes this week, and if it works better than before, we'll leave it in; else we'll pull it. I don't think we should be spending time trying to fix it right now; either it now works well enough, or we should disable it again.

Changed 7 years ago by jg

  • milestone changed from Never Assigned to Update.1

Changed 7 years ago by marco

It doesn't seem to work right yet, and Alexl confirm it. I'd say to not waste time on it and just disable and go with walter plan.

Changed 7 years ago by marco

  • owner changed from marco to rwh

Changed 7 years ago by rwh

  • cc marco added
  • keywords review? added

Attached diff divides the donut in 6 parts. If there are more activities they split evenly.

Changed 7 years ago by marco

  • keywords review+ added; review? removed

Please add a comment in _update_activity_sizes that we are disabling real memory calculation because it's current unreliable (it would be good to also reference this ticket).

review+ with the comment

Changed 7 years ago by rwh

Changed 7 years ago by rwh

  • owner changed from rwh to ApprovalForUpdate

Comment added as Marco suggested

Changed 7 years ago by jg

Ok for now.

I'd like this to be the last attempt to fix this feature.

Changed 7 years ago by jg

  • owner changed from ApprovalForUpdate to cscott

Changed 7 years ago by marco

  • owner changed from cscott to rwh

rwh please push this in git, I'll build an rpm for it.

Changed 7 years ago by rwh

  • status changed from new to closed
  • resolution set to fixed
Note: See TracTickets for help on using tickets.