Ticket #8372 (closed defect: fixed)

Opened 6 years ago

Last modified 4 years ago

Remove numpy usage from the shell

Reported by: marco Owned by: marco
Priority: normal Milestone: 8.2.0 (was Update.2)
Component: sugar Version: not specified
Keywords: r+ Cc:
Action Needed: test in release Verified: no
Deployments affected: Blocked By:
Blocking: #7579

Description

It's using a lot of memory and slowing down startup.

Attachments

numpy-sugar.patch (4.3 kB) - added by marco 6 years ago.
0001-Add-a-SugarGrid-object-to-replace-the-numpy-implemen.patch (9.5 kB) - added by marco 6 years ago.

Change History

Changed 6 years ago by marco

Changed 6 years ago by marco

  • keywords r? added
  • next_action changed from never set to review
  • milestone changed from Not Triaged to 8.2.0 (was Update.2)

Not well tested nor reviewed yet. Let's first decide on the approach.

Changed 6 years ago by bernie

In all loops:

56 for (i = rect->x; i < rect->x + rect->width; i++) { 57 for (k = rect->y; i < rect->y + rect->width; i++) {

The second line s/k/i/g and s/width/height/.

Also, swap the inner and outer loop. This hopefully lets the compiler simplify the calculations for the array subscriptions to a linear increment.

Changed 6 years ago by bernie

Please also update sugar.spec to remove the dependency on numpy

Changed 6 years ago by bernie

28 void 
29 sugar_grid_setup(SugarGrid *grid, gint width, gint height) 
30 { 
31    g_free(grid->weights); 
32    
33    grid->weights = g_new0(guchar, width * height);

Why the g_free() on (possibly) uninitialized memory?

Changed 6 years ago by marco

  • owner set to marco
  • component changed from not assigned to sugar

To handle multiple setup calls (we don't do it at the moment but...). g_free is NULL safe, do you see cases where it could be uninitialized? It's set to NULL in _init.

Changed 6 years ago by cscott

Changed 6 years ago by marco

<benzea> ok, looks all good to me

Changed 6 years ago by mstone

  • blocking 7579 added

Changed 6 years ago by marco

I tested it pretty heavily while hunting down the leaks and it seem to behave well. The only problem is a warning on setup(). It doesn't hurt I think, but we should fix it.

Changed 6 years ago by marco

  • keywords r+ added; r? removed

Ok, this had a least a couple of reviews, so I'm going to push it. The warning is caused by SpreadLayout passing a float, will fix that in sugar.

Changed 6 years ago by marco

  • next_action changed from review to package

Changed 6 years ago by marco

  • next_action changed from package to test in build

Changed 6 years ago by marco

  • next_action changed from test in build to approve for release

sugar-0.82.7-1.olpc3 and sugar-toolkit-0.82.8-2.olpc3

Changed 6 years ago by mstone

  • next_action changed from approve for release to add to release

Approved.

Changed 6 years ago by cscott

  • next_action changed from add to release to test in release

Please test build 761 resulting from build-repo commit b7c11be9.

Changed 6 years ago by mchua

Is there a test case we can use to see (from the user side) whether the patch has had the desired effect? I'm lost on how to check for this other than "open up the source on the XO, make sure the sugar-grid code's in there and the numpy code isn't".

Changed 6 years ago by marco

You can either use ps_mem or free to measure memory, and compare before/after the change.

http://sugarlabs.org/go/DevelopmentTeam/Profiling

and

http://lists.laptop.org/pipermail/devel/2008-September/019301.html

Changed 6 years ago by mchua

From IRC:

<marcopg> mchua_: I would apply this patch (reversed) 
http://dev.laptop.org/attachment/ticket/8372/numpy-sugar.patch 
and test both after and before on a clean 763

|Test case|

  1. clean-install 763 on an XO; boot it up.
  2. On that XO, measure memory usage. See http://wiki.laptop.org/go/Measuring_memory_usage.
  3. download the numpy patch: (In a terminal) wget http://dev.laptop.org/attachment/ticket/8372/numpy-sugar.patch
  4. reverse the patch, patch -p0 -R < ./numpy-sugar.patch. You will now have an XO running 763 but (hopefully) with this particular bug reintroduced.
  5. Measure memory usage again.

Test passes if the clean-installed XO (1st time measuring memory usage) has better memory stats than the patch-reversed XO (2nd time measuring memory usage).

Changed 4 years ago by bernie

  • status changed from new to closed
  • resolution set to fixed

This has been done long ago.

Note: See TracTickets for help on using tickets.