Closed Bug 1044297 Opened 7 years ago Closed 7 years ago

[B2G][Performance] Update memory performance tests to use new moz-app-loaded event

Categories

(Firefox OS Graveyard :: Performance, defect, P1)

All
Linux

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
See also Bug 1039150 for details.
Priority: -- → P2
See Also: → 1039150
Whiteboard: [c=memory p= s= u=]
Assignee: nobody → wcosta
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
Attachment #8465066 - Attachment description: PR → Makes start_test.js performance test to execute on moz-app-loaded.
Attachment #8465066 - Flags: review?(hub)
Attachment #8465066 - Flags: review?(eperelman)
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 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-
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.
Attached patch bug-1044297.patch (obsolete) — Splinter Review
Patch without refactoring.
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)
I have no problem with a PR with more than one commit.
Flags: needinfo?(hub)
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).
(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.
startup_test is a performance test for "apploadtime" on which we added a memory test. See bug 917717
Attachment #8465447 - Attachment is obsolete: true
(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
See Also: → 917717
Wander & Hub, I commented on the PR, please take a look and reply.
Blocking on the breakage for now.
Depends on: 1048024
Depends on: 1045717
Depends on: 1049031
Flags: needinfo?(eperelman)
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 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+
Attachment #8465066 - Flags: review- → review?(hub)
Duplicate of this bug: 1036638
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+
Keywords: checkin-needed
Master: https://github.com/mozilla-b2g/gaia/commit/d6fae2c283d4f5f18c9cd1e6a3dc0351c29b6836
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
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.