Open Bug 1364876 Opened 7 years ago Updated 2 years ago

Move heapsnapshot native codebase to /devtools/platform/

Categories

(DevTools :: General, enhancement, P3)

Product:
DevTools
Component:
General
Type:
enhancement

Tracking

(Not tracked)

People

(Reporter: ochameau, Unassigned)

References

Details

Attachments

(2 files)

Bug 1364876 - Move heapsnapshot native components to /devtools/platform/heapsnapshot.
(deleted), text/x-review-board-request
Details
Bug 1364876 - Move heapsnapshot JS code to Firefox package.
(deleted), text/x-review-board-request
Details 
As we are about to extract devtools sources out of mozilla-central in order to release them as an add-on, we have to pull out all native components, like heapsnapshot out of main devtools folders. (client, shared, server, which are all going to move to github)

/devtools/platform is going to stay on mozilla-central and contain all devtools native codebases.
Priority: -- → P3
Nick, Would you mind reviewing this patch, or forwarding to whoever maintain the heapsnapshot codebase?

My comprehension of this is that mochitest and gtest cover the c++ part,
while the unittest are focused on the JS helper modules that are still in /devtools/shared/heapsnapshot/.
Comment on attachment 8883617 [details]
Bug 1364876 - Move heapsnapshot native components to /devtools/platform/heapsnapshot.

https://reviewboard.mozilla.org/r/154556/#review159668

Most of the xpchsell tests need to stay with the heap snapshot code as well. They exist to exercise the heap snapshot platform code, so they should also be in devtools/platform with the heap snapshot code.
Attachment #8883617 - Flags: review?(nfitzgerald)
Attachment #8883617 - Flags: review?(nfitzgerald)
Sorry, I didn't meant to re-ask for review yet.

This is unfortunate that the c++ code is being codevered by test against the JS modules.
It complexify the story here as these modules depends on devtools.
It is fine moving/keeping JS in Firefox. I imagine it makes sense here to keep them next to the C++ API and expose stable interface via these HeapAnalyses modules.
But we are trying to remove all dependencies made to devtools codebase from firefox/platform.
The main reason is that devtools won't necessarely be installed on release channels.
Here that is not an issue as devtools are going to be installed when we want to use this code.
But this is not reasonable to depend on DevToolsWorker or DevToolsUtils as it may change
over time in the add-on and don't want to freeze them.

So, here is a second proposal, with this additional patch, I move all of heapsnapshot to /devtools/platform.
Everything will be kept in Firefox. But it would be better to followup in order to remove the dependencies to DevToolsWorker and DevToolsUtils. And it would be even better if we convert them to JSM in order to no longer depend on the module loader which is also a dependency made to the add-on.

Your thoughts?
Sorry for not leaving a more descriptive comment yesterday -- I realize that my review was hurried and unhelpful. Apologies!

Here is a quick survey of the xpcshell tests under devtools/shared/heapsnapshot

Tests that definitely need to stay with the HeapSnapshot platform code, because that is what they're exercising:

- test_HeapSnapshot_*
- test_ReadHeapSnapshot*
- test_saveHeapSnapshot*
- test_DominatorTree_*

Tests that can move to github with the rest of the devtools JS code, assuming there is a way to take a heap snapshot for tests since these depend on that ability, but exercise the JS post-processing of them:

- test_HeapAnalyses_*
- test_census_diff_*
- test_census_filtering_*
- test_census-tree-node-* (why mixing - and _?? sorry...)
- test_countToBucketBreakdown_*
- test_deduplicatePaths_01.js
- test_DominatorTreeNode_*
- test_getCensusIndividuals_*
- test_getReportLeaves_*

If there is no way to take heap snapshots in tests when the devtools code moves to github, then any test that exercises the analyses worker (test_HeapAnalyses_*) needs to stay in m-c as well. All of the CensusTreeNode and DominatorTreeNode tests can move still, as those are working with 100% mocked data.

(In reply to Alexandre Poirot [:ochameau] from comment #13)
> This is unfortunate that the c++ code is being codevered by test against the
> JS modules.

Some of it is doing that, some of it just depends on the C++ APIs to get data to work with, as mentioned above.

I agree that it would be better to have things like gtests (which we have some of) but sometimes its hard to gtest when you need so much of gecko/firefox loaded up. Also, some of it is testing things like, the webidl interface is properly exposed as we expect and behaves as we expect, which really needs to be tested by JS either way.

> So, here is a second proposal, with this additional patch, I move all of
> heapsnapshot to /devtools/platform.

I guess?

But where do we draw the line? The frontend depends heavily on the heap analyses worker, which depends heavily on the platform APIs. Should we keep the whole memory tool frontend in m-c too?

> Everything will be kept in Firefox. But it would be better to followup in
> order to remove the dependencies to DevToolsWorker and DevToolsUtils. And it
> would be even better if we convert them to JSM in order to no longer depend
> on the module loader which is also a dependency made to the add-on.

Would rather not convert to JSM... is that what every other module using the (devtools or addon-sdk) loader is doing? Maybe we can just go directly to ES6 modules?

I guess I'm OK making a copy of every DevToolsUtils function used and putting it in devtools/platform/heapsnapshot/utils.js or something...
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #15)
> Sorry for not leaving a more descriptive comment yesterday -- I realize that
> my review was hurried and unhelpful. Apologies!
> 
> Here is a quick survey of the xpcshell tests under
> devtools/shared/heapsnapshot
> 
> Tests that definitely need to stay with the HeapSnapshot platform code,
> because that is what they're exercising:
> 
> - test_HeapSnapshot_*
> - test_ReadHeapSnapshot*
> - test_saveHeapSnapshot*
> - test_DominatorTree_*

Ah! I didn't realized there was a couple of tests only relying on platform code.
That makes sense. So. If I get back to my original patch and move these tests
to /devtools/platform/heapsnapshot/tests/unit/, would that work for you?
This would be the simpliest. Or are you concerned about spliting that way?

> If there is no way to take heap snapshots in tests when the devtools code
> moves to github, then any test that exercises the analyses worker
> (test_HeapAnalyses_*) needs to stay in m-c as well. All of the
> CensusTreeNode and DominatorTreeNode tests can move still, as those are
> working with 100% mocked data.

For now, we would run the xpcshell as we do on m-c, so this would keep running even when using the add-on or being on github.

> > So, here is a second proposal, with this additional patch, I move all of
> > heapsnapshot to /devtools/platform.
> 
> I guess?
> 
> But where do we draw the line? The frontend depends heavily on the heap
> analyses worker, which depends heavily on the platform APIs. Should we keep
> the whole memory tool frontend in m-c too?

The default line is pretty simple, all code depending *on* devtools should move with devtools.
The dependencies on DevToolsWorker and DevToolsUtils justify looking into moving that to devtools.
Now, I think it also makes sense to keep things in Firefox/m-c.
This code, which I imagine is tighly coupled with the C++ implementation may be a good example.
This question has been also relevant regarding the whole server with actors.
But the dependencies on too many things from /devtools/shared would justify duplicating all these modules in m-c and the addon. Also we would like to push fixes made to the actors and not wait for firefox release cycle to hotfix them.

> Would rather not convert to JSM... is that what every other module using the
> (devtools or addon-sdk) loader is doing? Maybe we can just go directly to
> ES6 modules?

I wish we could use ES6 modules but haven't heard anyone using it in chrome yet.
It is unclear how you would import the first module from chrome scope.
And I agree I would prefer not doing any refactoring.
Product: Firefox → DevTools
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: