[Build][Raptor] Add a code transformer tool to support dynamically adding marks to help to test app performance

RESOLVED FIXED in FxOS-S3 (24Jul)

Status

Firefox OS
Gaia::Build
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: snowmantw, Assigned: snowmantw)

Tracking

({perf})

unspecified
FxOS-S3 (24Jul)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(tracking-b2g:+)

Details

(Whiteboard: [perf-wanted])

Attachments

(2 attachments)

Since Raptor tests require to add marks in code, if we do that without any dynamic patching way, Gaia codebase would be full with `Performance.mark` without any visible & meaningful relations among them. The better way is to collect all these marks into some patching files, and apply them only when we want to run the specific Raptor test. For example, if we want to test LockScreen performance from locking to unlocking automatically, we may need to patch two functions with such marks:

    LockScreen#lock = function() {
        Performance.mark('LockScreen#lock')
        // ....
    }
    
    LockScreen#unlock = function() {
        Performance.mark('LockScreen#unlock');
        // ....
    }

We can collect them in a file `lockscreen-lock-unlock.esp.js`:

    Espect.select('lockscreen.js LockScreen#lock')
          .before(function() {
              Performance.mark('LockScreen#lock')
          })
    
    Espect.select('lockscreen.js LockScreen#unlock')
          .before(function() {
              Performance.mark('LockScreen#unlock')
          })


(The Espect is a tool written by me for the purpose to patch Gaia during build time)

https://github.com/snowmantw/espect.js

And apply such `.esp.js` file only when we want to run such Raptor test. So the codebase keeps clean from such marks, and since we collect all related marks in the same file, we know they're for the same test. This is useful especially when we want to test multiple functions across different apps (ex: to test LockScreen unlocking to Homescreen shows up). The whole idea is here:

https://docs.google.com/presentation/d/1DemVx_963IX19LVWE0THKQKm9S40X24XGOAtwOhupA0/edit?usp=sharing

I now have a WIP for that. How to use it is:

1. Apply the patch to your Gaia or just checkout my branch
2. npm install
3. as Commit message: APP=<your app, like system> DEBUGUY=<where the debuguy is> TESTDIR=<where the esp files are> make

After that, you can check the file your `.esp.js` is for would be patched. There is one demo esp file under `/apps/system/lockscreen/raptor`, which should patch the `lockscreen.js`. The sample make command is:

APP=system DEBUGUY=gaia/node_modules/debuguy TESTDIR=gaia/apps/system/lockscreen/raptor make

I would update the patch to make it ready.

(BTW, Debuguy is another tool developed by our device team. It's used here to perform multiple dynamic patching at once).

https://github.com/seiyugi/debuguy
(Assignee)

Updated

2 years ago
Assignee: nobody → gweng
(Assignee)

Comment 1

2 years ago
Created attachment 8630376 [details] [review]
WIP patch

Ricky would be the reviewer, so set feedback to him early.
Attachment #8630376 - Flags: feedback?(rchien)
(Assignee)

Comment 2

2 years ago
And NI bobby to see who should be CCed as well.
Flags: needinfo?(bchien)
(Assignee)

Comment 3

2 years ago
NI Eli to see if there are more advanced opinions.
Flags: needinfo?(eperelman)

Updated

2 years ago
tracking-b2g: --- → +
Flags: needinfo?(bchien)
Keywords: perf
Whiteboard: [perf-wanted]
(Assignee)

Comment 4

2 years ago
Oh and I forgot that Debuguy would be an old version if you install them via NPM. I've asked owner to update it. Before that, installing it via cloning on GitHub makes the demo run.
(Assignee)

Comment 5

2 years ago
I've update the patch. Now the command is:

    APP=<app> TESTDIR=<where the test dir is> RAPTOR_TRANSFORM=1 make

Can still specify the debuguy path with an additional option:

    RAPTOR_TRANSFORMER_PATH=<path to debuguy>

But it's optional.
(Assignee)

Comment 6

2 years ago
And I now added one build test: if someone change the code to make espect couldn't match new code, like the file got moved away or removed, the test would fail and we can catch such regression. It's now in 'system/test/build/integration/raptor_transform.js'.
(Assignee)

Comment 7

2 years ago
Oh...but I still don't know how to add a new NPM package dependency to CI server. NI Ricky for that.
Flags: needinfo?(rchien)
Forward to Gareth who is the best person to answer your question!
Flags: needinfo?(rchien)
Flags: needinfo?(gaye)

Comment 9

2 years ago
I don't have any reservations about dynamic injection of marks; the approach being used here seems legit.
Flags: needinfo?(eperelman)
So the best thing to do as far as npm dependencies go is to make a new, local package like gaia-marionette (https://github.com/mozilla-b2g/gaia/tree/master/bin/gaia-marionette) if you're planning on this being a utility that's available to gaia apps. If it's something we're thinking of baking into raptor, then it could go here instead https://github.com/mozilla-b2g/gaia/tree/master/tools/raptor. (I haven't looked at the patch)
Flags: needinfo?(gaye)
(Assignee)

Comment 11

2 years ago
I could put the files in Gaia repo, but what confuse me is it may still depend on other 3rd packages that may also have their own deep dependencies. So if I really need to check it in Gaia, this means I also need to check-in the whole node_modules after I install it locally. However, I don't see this happens in the current Gaia repo, so I don't know if it's legit.

And in my recollection there is a taskcluster-npm-mirroring or cache server to serve such requests during npm install. I've re-read the thread on mailing list again, but I still don't know if it works on b2g-inbound or mozilla-central. And Ricky and I just don't get if it works like:

1. register our packages in package.json as usual
2. when package.json in Gaia updated and when it's trying on Taskcluster, the cache server would mirror all necessary packages when it's first time installing those modules
3. after the cache built, the dependencies on CI server are done, so there is no need to fetch packages from external network anymore

Since Bug 1141792 it describes to remove node module tarball, but what we should do to make this work is a bit confused. And I don't know if check-in the whole module repo into Gaia is not conflicting with such solution.
Flags: needinfo?(gaye)
Comment on attachment 8630376 [details] [review]
WIP patch

Didn't see any potential risks for that WIP patch. I've leaved nits and question on Github!
Attachment #8630376 - Flags: feedback?(rchien) → feedback+
(Assignee)

Comment 13

2 years ago
NI Aus because I still don't know which approach is correct: let Taskcluster NPM cache to handle the package Gaia depends on? Or manually parsing all dependencies of such module and land them all in Gaia repo?
Flags: needinfo?(aus)

Comment 14

2 years ago
I can't see where you're adding node module dependencies in your patch. Are you sure it's up to date?

NPM cache works on *ALL* trees. That's why it's not specified in the email where it works. You should let it take care of your dependencies. Just make sure the dependencies are present in the appropriate package.json.
Flags: needinfo?(aus)
(Assignee)

Comment 15

2 years ago
Since I'm not sure what to deal with the dependencies, I leave no new package at the package.json. I would submit the change according to your opinion, thanks!
Flags: needinfo?(gaye)

Comment 16

2 years ago
Created attachment 8634587 [details] [review]
[gaia] snowmantw:bug1181069 > mozilla-b2g:master
(Assignee)

Comment 17

2 years ago
Okay great. It looks the patch would do the test on CI server:

https://treeherder.mozilla.org/logviewer.html#?job_id=1664056&repo=gaia

And almost all the tests are green. So I would set the review? flag soon.
(Assignee)

Comment 18

2 years ago
Comment on attachment 8634587 [details] [review]
[gaia] snowmantw:bug1181069 > mozilla-b2g:master

Ricky: I've updated the patch and it passed the CI tests. So I set review. Thanks!
Attachment #8634587 - Flags: review?(rchien)
Comment on attachment 8634587 [details] [review]
[gaia] snowmantw:bug1181069 > mozilla-b2g:master

I don't have any other concerns about this patch. There are some nits in Github please fix it before landing.
Attachment #8634587 - Flags: review?(rchien) → review+
(Assignee)

Comment 20

2 years ago
Comment on attachment 8634587 [details] [review]
[gaia] snowmantw:bug1181069 > mozilla-b2g:master

Sorry I forgot: since Tim and me may be the first few user of this feature, I add feedback to him. I would also fix what Tim says before landing.

For now it could be tested with:

APP=<app> TESTDIR=<where the test dir is> RAPTOR_TRANSFORM=1 make

For example:

APP=system TESTDIR=gaia/apps/system/lockscreen/raptor RAPTOR_TRANSFORM=1 make

And the profile would be transformed with the code of 'performance.mark'. (I forgot it's lowercase and would update in the patch. Anyway the architecture and the flow works).
Attachment #8634587 - Flags: feedback?(timdream)
Comment on attachment 8634587 [details] [review]
[gaia] snowmantw:bug1181069 > mozilla-b2g:master

Small question on GitHub, other than that it's nothing to see here since majority of the logic is in the node modules right?
Attachment #8634587 - Flags: feedback?(timdream) → feedback+
(Assignee)

Comment 22

2 years ago
I've updated the patch and now is waiting CI to land the code.
(Assignee)

Comment 23

2 years ago
OK I now land it!
(Assignee)

Comment 24

2 years ago
Oh I forgot to land it with autolander. But I remember manual landing is stil OK. So here it is!

https://github.com/mozilla-b2g/gaia/commit/e01915e8c66e169802d7c5a4b6b089ab3c7f5e12
(Assignee)

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED

Updated

2 years ago
Target Milestone: --- → FxOS-S3 (24Jul)
You need to log in before you can comment on or make changes to this bug.