846909 - Insert our instrumentation framework before the app starts

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::PerformanceTest, defect, P1)

Description

[James Lal [:lightsofapollo]]

In the performance tests we have some very specific metrics we want to collect and they require us to make some changes to the apps themselves. 

As a result we include JS we don't actually need in production for testing only. 

Given the very small amount of code we are looking at today it is not a big issue but we should have conditional statements that allow us to include debug/test code only in un-optimized (or debug/test) builds so we don't have to worry about actually slowing the production application just for testing.

This could also be useful for things like desktop compatibility shims.

Comment 1

[James Lal [:lightsofapollo]]

Also- these statements should not break normal web content. Ideally we can use some very specific comments around some code/html that will get removed during build time.

The worst that can happen is we ship with the comments though ideally we remove them during the optimized build entirely.

Comment 2

[Anthony Ricaud (:rik)]

We're thinking about going in that direction. Right now the in-app metrics are varying a lot [1] and we sometimes don't receive events that are dispatched very early in the app lifetime (we never receive the full 5 first chunk event [2]).

So we probably want the performance helper to do more when included.

[1] https://datazilla.mozilla.org/b2g?branch=master&range=7&test=rendering_time_>_last_chunk&app=communications/contacts&app_list=browser,calendar,camera,clock,communications/contacts,contacts,email,fm_radio,ftu,gallery,marketplace,messages,music,phone,settings,usage&gaia_rev=c12c99b6862ef9da&gecko_rev=9306ad2157bb28da
[2] https://datazilla.mozilla.org/b2g?branch=master&range=7&test=rendering_time_>_first_chunk&app=communications/contacts&app_list=browser,calendar,camera,clock,communications/contacts,contacts,email,fm_radio,ftu,gallery,marketplace,messages,music,phone,settings,usage&gaia_rev=aafcb4fbcfb30371&gecko_rev=9306ad2157bb28da

Comment 3

[Anthony Ricaud (:rik)]

I talked with Vivien about our issues and he mentioned loadFrameScript, a method that can allow us to insert our code in the Gecko process before the app is launched. That sounds like a cleaner way to solve our issues.

Comment 4

[Julien Wajsberg [:julienw]]

From a discussion on IRC with jgriffin, we should switch to "chrome" context before executing our script that will use loadFrameScript. loadFrameScript itself needs an URL (either chrome:// or data:) so I suspect we'll need to convert our script as a data-url.

After that, we need to switch back to "content" context.

Comment 5

[James Lal [:lightsofapollo]]

I still plan to work on this ( I had a 25% working patch ) but I probably won't get to finish it this week :( once I wrap up my tef+ stuff I will work on this.

Comment 6

[James Lal [:lightsofapollo]]

I wrote some code that can do this recently: https://github.com/mozilla-b2g/marionette-content-script but it will need some changes to work OOP I believe.

Comment 7

[Hubert Figuiere [:hub]]

Attachment: Pull request

Proposed implementation on top of the branch for bug 915156.

Comment 8

[James Lal [:lightsofapollo]]

Comment on attachment 8340562 [details] [review]
Pull request

This does not look quite right to me... Usually more boilerplate is needed so each frame in the content process has the correct setup.

See: https://github.com/mozilla-b2g/gaia/blob/master/apps/email/test/marionette/lib/mocks/mock_navigator_mozalarms.js

I am no expert here... I stole this code from some other code in the old keyboard (gecko side) code base.

Comment 9

[Hubert Figuiere [:hub]]

On overall it does work, but there seems to be a discrepency in the output before and after the patch. It is not that obvious though, nor does it cause any error :-/

Comment 10

[Hubert Figuiere [:hub]]

Marionette Content Script seems to need this pull request to work.

https://github.com/mozilla-b2g/marionette-content-script/pull/4

Comment 11

[Hubert Figuiere [:hub]]

All things consider, I doubt of the benefit of doing so.

We already have the PerformanceHelper as a no-op if there is no listener.

Unlike in other places where this is used to "mock" other subsystems for testing purpose.

Deprioritizing.

Comment 12

[Anthony Ricaud (:rik)]

I don't see the relation between the no-op thingie and the purpose of that bug: having more reliable measurements.

Comment 13

[Hubert Figuiere [:hub]]

I don't see where it would be more reliable either.

Comment 14

[Hubert Figuiere [:hub]]

Right now I have some other ideas about this that would be worth it.

Comment 15

[Hubert Figuiere [:hub]]

Attachment: content script PR

James, I believe this fix is important.

Comment 16

[James Lal [:lightsofapollo]]

Looks like mike fixed it https://github.com/mozilla-b2g/marionette-content-script/blob/master/index.js#L50

Comment 17

[Hubert Figuiere [:hub]]

P1, see bug 980978

Comment 19

[Hubert Figuiere [:hub]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18697

Comment 20

[Julien Wajsberg [:julienw]]

Comment on attachment 8412837 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18697

Questions and suggesting in the PR.

I haven't tested it yet, but I am really happy to see this happening !

Comment 21

[Hubert Figuiere [:hub]]

Comment on attachment 8412837 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18697

I have updated the PR with some of the issues addressed and answers given.

Comment 22

[Hubert Figuiere [:hub]]

And for the record, it seems that this doesn't work for Tarako (1.3T) due to differences in the platform. I will need to figure out a proper workaround for that.

Comment 23

[Hubert Figuiere [:hub]]

(In reply to Hubert Figuiere [:hub] from comment #22)
> And for the record, it seems that this doesn't work for Tarako (1.3T) due to
> differences in the platform. I will need to figure out a proper workaround
> for that.

That was bug 1006064 - and then bug 1007401 is about switch automation over.

So we should be good to go.

Comment 24

[Julien Wajsberg [:julienw]]

Sorry for the delay, I was in the middle of 1.3t+ issues. Now it's over but it's the end of the week but I'll be sure to review this asap next week.

On a side note, it may be a good idea to shift progressively the reviews to your team.

Comment 25

[Julien Wajsberg [:julienw]]

Comment on attachment 8412837 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18697

r=me with the nits

Comment 26

[Julien Wajsberg [:julienw]]

Comment on attachment 8412837 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18697

oops, wrong bug

Comment 27

[Julien Wajsberg [:julienw]]

Comment on attachment 8412837 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18697

This looks good, but I still have a question about the pref idea.

I'm giving r+ because we badly need this patch I want it landed. But I think using a pref to control how it works could be a good idea.

Comment 28

[Hubert Figuiere [:hub]]

Merged
https://github.com/mozilla-b2g/gaia/commit/2dabcaf728e8fd74c0d2959ad28dfe8e5dc5e9b2

Comment 29

[Hubert Figuiere [:hub]]

Filed bug 1009650 to do it using preferences for the marionette client.