Opened 6 years ago

Closed 4 years ago

#8372 closed defect (fixed)

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:
Blocked By: Blocking: #7579
Deployments affected: Action Needed: test in release
Verified: no

Description

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

Attachments (2)

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.

Download all attachments as: .zip

Change History (22)

Changed 6 years ago by marco

comment:1 Changed 6 years ago by marco

  • Action Needed changed from never set to review
  • Keywords r? added
  • 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.

comment:2 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.

comment:3 Changed 6 years ago by bernie

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

comment:4 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?

comment:5 Changed 6 years ago by marco

  • Component changed from not assigned to sugar
  • Owner set to marco

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.

comment:7 Changed 6 years ago by marco

<benzea> ok, looks all good to me

comment:8 Changed 6 years ago by mstone

  • Blocking 7579 added

comment:9 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.

comment:10 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.

comment:11 Changed 6 years ago by marco

  • Action Needed changed from review to package

comment:12 Changed 6 years ago by marco

  • Action Needed changed from package to test in build

comment:13 Changed 6 years ago by marco

  • Action Needed changed from test in build to approve for release

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

comment:14 Changed 6 years ago by mstone

  • Action Needed changed from approve for release to add to release

Approved.

comment:16 Changed 6 years ago by cscott

  • Action Needed changed from add to release to test in release

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

comment:17 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".

comment:18 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

comment:19 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).

comment:20 Changed 4 years ago by bernie

  • Resolution set to fixed
  • Status changed from new to closed

This has been done long ago.

Note: See TracTickets for help on using tickets.