Closed
Bug 1044297
Opened 10 years ago
Closed 10 years ago
[B2G][Performance] Update memory performance tests to use new moz-app-loaded event
Categories
(Firefox OS Graveyard :: Performance, defect, P1)
Tracking
(b2g-v2.1 fixed)
RESOLVED
FIXED
2.1 S2 (15aug)
Tracking | Status | |
---|---|---|
b2g-v2.1 | --- | fixed |
People
(Reporter: wcosta, Assigned: wcosta)
References
Details
(Keywords: perf, Whiteboard: [c=memory p= s= u=])
User Story
Update the existing Memory Performance Tests reporting to Datazilla [1] to listen for the moz-app-loaded event [2] before gathering memory usage data. [1] https://wiki.mozilla.org/FirefoxOS/Performance/Memory#Memory_Usage_in_Automation [2] https://bugzilla.mozilla.org/show_bug.cgi?id=996038#user_story_header
Attachments
(1 file, 1 obsolete file)
Create memory performance tests using the new app Launch Events [1] [1] [!] https://developer.mozilla.org/en-US/Apps/Build/Performance/Firefox_OS_app_responsiveness_guidelines#Implementation
Assignee | ||
Comment 1•10 years ago
|
||
See also Bug 1039150 for details.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → wcosta
Updated•10 years ago
|
Severity: normal → major
Status: NEW → ASSIGNED
User Story: (updated)
Keywords: perf
Priority: P2 → P1
Summary: [B2G][Mem] Create memory performance tests → [B2G][Performance] Update memory performance tests to use new moz-app-loaded event
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8465066 -
Attachment description: PR → Makes start_test.js performance test to execute on moz-app-loaded.
Assignee | ||
Updated•10 years ago
|
Attachment #8465066 -
Flags: review?(hub)
Attachment #8465066 -
Flags: review?(eperelman)
Comment 3•10 years ago
|
||
Comment on attachment 8465066 [details] [review] Makes start_test.js performance test to execute on moz-app-loaded. Can we rename this file to something like memory_test.js? It's not *technically* startup focused, and I think naming it something along the lines of memory makes sense. Also, I'm up in the air on the scope of the changes with the PR. The focus of this bug should be to use the new event for measuring memory, but we have a fair bit of refactoring here, and I think we should wait to handle this refactoring until the work towards make test-perf stabilizes. That's not to say this refactoring isn't needed, it's just a bit out of scope. r- Let's reduce the scope of changes here and rename the file to something more suitable.
Attachment #8465066 -
Flags: review?(eperelman) → review-
Comment 4•10 years ago
|
||
Comment on attachment 8465066 [details] [review] Makes start_test.js performance test to execute on moz-app-loaded. startup_test is startup_test. if you really want to collect the memory info on an event, either do it in the startup_event_test in the right place, or write an actual separate memory test - I don't know if you have discussed this part with Eli, but I'd be more inclined to add this to the startup_event_test. Right now you just change startup_test completely, and that is not good because it make the test different, and we lose the actual startup_test.
Attachment #8465066 -
Flags: review?(hub) → review-
Comment 5•10 years ago
|
||
And just to be clear. If Eli prefer that we actually don't do that in the startup_event_test - and I see a few reason myself to not want -, I'm completely fine too. We can discuss this.
Assignee | ||
Comment 6•10 years ago
|
||
Patch without refactoring.
Assignee | ||
Comment 7•10 years ago
|
||
Initially I broke the patch into two commits, the change to fix the bug and the code refactoring, but I was told to submit PR with one commit. Attachment 8465447 [details] [diff] contains the patch without refactoring. The downside is that we have some code duplication. The :hub suggestion would fix this: moving the two tests to the same file would allow as least code change as possible, loosing some code modularity.
Both approaches have pros and cons, not sure what to take :/
Flags: needinfo?(hub)
Flags: needinfo?(eperelman)
Comment 8•10 years ago
|
||
I have no problem with a PR with more than one commit.
Flags: needinfo?(hub)
Comment 9•10 years ago
|
||
This still change the startup_test.js. Have you run before and after? What are the difference in times? Just by looking at this code, I'm sure it is longer. It was not about the refactor and such, but really about the fact you completely change an existing test. Also, since we are on gaia, please submit pull requests - it is the standard procedure. You can update an existing one by just pushing to the same branch (even with a -f).
Assignee | ||
Comment 10•10 years ago
|
||
(In reply to Hubert Figuiere [:hub] from comment #9) > This still change the startup_test.js. Have you run before and after? What > are the difference in times? Just by looking at this code, I'm sure it is > longer. > It was not about the refactor and such, but really about the fact you > completely change an existing test. > Hmm, start_test is actually a memory test, I thought would be better to change it to make memory test after moz-app-loaded, as it would be useless maintaining the two test cases. I will create a separate module for it. > Also, since we are on gaia, please submit pull requests - it is the standard > procedure. You can update an existing one by just pushing to the same branch > (even with a -f). Sure.
Comment 11•10 years ago
|
||
startup_test is a performance test for "apploadtime" on which we added a memory test. See bug 917717
Assignee | ||
Updated•10 years ago
|
Attachment #8465447 -
Attachment is obsolete: true
Assignee | ||
Comment 12•10 years ago
|
||
(In reply to Hubert Figuiere [:hub] from comment #11) > startup_test is a performance test for "apploadtime" on which we added a > memory test. See bug 917717 Attachment 8465066 [details] I didn't realize that, sorry. Ok, take 2 on PR. I created a new memory_test.js module. Output: https://pastebin.mozilla.org/5733626
Comment 13•10 years ago
|
||
Wander & Hub, I commented on the PR, please take a look and reply.
Comment 15•10 years ago
|
||
Hey Wander, could you please refactor this now that the config changes from bug 1049031 have landed? Specifically this is what I am looking for: 1. All apps that use the new launch event of "moz-app-loaded" are already whitelisted to have their memory measured in that way. Change startup_events_test.js to also capture the memory information upon reaching this event. 2. Keep the memory measurements in startup_test.js, but put a check at the top to return if the app exists in the mozLaunch whitelist (see isWhitelisted method in startup_events_test.js). Basically we don't want to measure the memory of apps that have been whitelisted to use the newer test. Please let us know if you have any questions.
Comment 16•10 years ago
|
||
Comment on attachment 8465066 [details] [review] Makes start_test.js performance test to execute on moz-app-loaded. A couple nits, otherwise good to go.
Attachment #8465066 -
Flags: review- → review+
Updated•10 years ago
|
Attachment #8465066 -
Flags: review- → review?(hub)
Comment 18•10 years ago
|
||
Comment on attachment 8465066 [details] [review] Makes start_test.js performance test to execute on moz-app-loaded. r=me But you shall squash the commits to one before we marge. Thanks.
Attachment #8465066 -
Flags: review?(hub) → review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 19•10 years ago
|
||
Master: https://github.com/mozilla-b2g/gaia/commit/d6fae2c283d4f5f18c9cd1e6a3dc0351c29b6836
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-b2g-v2.1:
--- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S2 (15aug)
You need to log in
before you can comment on or make changes to this bug.
Description
•