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

memory usage seems high #91

Closed
jbphet opened this issue May 17, 2017 · 14 comments
Closed

memory usage seems high #91

jbphet opened this issue May 17, 2017 · 14 comments
Assignees

Comments

@jbphet
Copy link
Contributor

jbphet commented May 17, 2017

I was working on checking for memory leaks, and was seeing memory usage under Chrome at about 60-70 MB. I just did a bit more testing and found the usage to now be around 85 MB. The odd thing is that I went and checked a previously released dev version and found its memory usage to be about the same, so I'm wondering if something recently changed in Chrome that causes it to use more memory.

At any rate, I'd like to get a handle on this in order to figure out if I need to reduce the memory usage and, if so, how. I'll start by collecting some data.

@jbphet
Copy link
Contributor Author

jbphet commented May 17, 2017

The latest dev release if v1.0.0-dev.23, available at http://www.colorado.edu/physics/phet/dev/html/expression-exchange/1.0.0-dev.23/expression-exchange_en.html. I'm first going to test the memory usage at startup. The procedure is:

  1. Open a new incognito window in Chrome so that we're starting from an unused tab
  2. Open the sim
  3. Go to the "Basics" screen
  4. Using the nav bar, go to each of the other screens
  5. Go to the home screen
  6. Take a heap snapshot

Chrome Version (from chrome://help/): Version 58.0.3029.110 (64-bit)

  • Trial 1: 80.3 MB
  • Trial 2: 80.4 MB
  • Trial 3: 82.5 MB
  • Trial 4: 82.1 MB

Two trials on the current master RequireJS version have the following values:

  • Trial 1: 87.3 MB
  • Trial 2: 87.8 MB

So, it's a bit higher in master+RequireJS, but less than 10% higher, so the high memory usage isn't due to some recent changes.

@jbphet
Copy link
Contributor Author

jbphet commented May 17, 2017

I turned off the game screen using the screens query parameter and the memory usage went to 46.8 MB. For a point of comparison, I run Unit Rates since it is a recently released three screen sim and found that it used 42.2 MB. These are in the ballpark of one another, so perhaps Expression Exchange's memory usage isn't all that out of line for this size of sim.

@jbphet
Copy link
Contributor Author

jbphet commented May 17, 2017

I just tried Function Builder, which is a recently released four screen sim and found that its memory usage is 78.6 MB (just one trial), so this also makes it seem like Expression Exchange is perhaps within reason in terms of its memory usage.

@jbphet
Copy link
Contributor Author

jbphet commented May 17, 2017

Marking for dev meeting. Do we have any guidelines or rules of thumb for memory usage?

@pixelzoom
Copy link
Contributor

Recent testing for Graphing Lines (phetsims/graphing-lines#78 (comment)) showed ~35MB for just the "Line Game" screen. So I don't think that these numbers are all that surprising for Expression Exchange or any of the other sims noted above.

@samreid
Copy link
Member

samreid commented May 18, 2017

Searching all of our repos for "MB" shows several hits that indicate prior memory test results:
https://github.com/issues?utf8=%E2%9C%93&q=is%3Aissue+MB+user%3Aphetsims+sort%3Acreated-asc

@jbphet
Copy link
Contributor Author

jbphet commented May 18, 2017

Discussed in the 5/18/2017 developer meeting and concluded that as long as it loads and runs on iPad 2, that should be a sufficient test.

@jbphet
Copy link
Contributor Author

jbphet commented May 18, 2017

I just did several tests on a 16 GB iPad 2 running iOS 9.3.5 (the PhET device named "Witten"), and the sim loaded most of the time, but there were two instances (out of roughly 8) where Safari crashed. This makes me feel as though we are on the edge for memory usage. @jonathanolson has been assigned to do the code review, and he also offered to look for ways to reduce the number of nodes somewhat. Assigning to him for now, I'll take it back when he's done what he can do.

@jbphet jbphet assigned jonathanolson and unassigned jbphet May 18, 2017
@samreid
Copy link
Member

samreid commented May 24, 2017

@jbphet can this be removed from "developer meeting" label?

@jbphet
Copy link
Contributor Author

jbphet commented May 24, 2017

Yes (done).

@jonathanolson
Copy link
Contributor

As noted in #88, I believe the creation of nodes (particularly images) for every coin term (if I'm reading things correctly) in CoinNodeFactory may be the culprit.

Using DAG patterns I believe should help a lot, as each Scenery Image node creates a DOM Image element, which depending on the browser may take up a sizable amount of memory.

RichText also creates a lot of nodes, which is also an issue for the Area Model simulations that I'm working on. A general improvement may be helpful.

Currently, by creating as many coins as I could on all screens, I was able to bump the number of nodes in the tree past 9000.

@jbphet, collaboration on this issue would probably be best.

@jonathanolson
Copy link
Contributor

Also will plan to check performance with @jbphet when it doesn't crash.

@phet-steele
Copy link
Contributor

@jbphet @jonathanolson, 1.1.0-dev.4 is not crashing on an iPad 2. Each screen was filled with max terms and the game was completed multiple time. That's not to say the sim didn't slow down, however (was still usable).

@jbphet
Copy link
Contributor Author

jbphet commented Jun 29, 2017

Since this now works on iPad 2 and doesn't appear to leak memory, I'm closing this issue.

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

No branches or pull requests

5 participants