Ted Campbell [:tcampbell] Reporter
	Description • 6 years ago

With Bug 1186409 landed and enabled, we should explore removing the features of Bug 1013997.

The current global sharing implementation of devtools loader is using non-global objects as a target for loadSubscript calls. While this used to be an acceptable pattern, the perf of doing so is quite bad. We wind up using WithEnvironments and @@unscopables checks which bounces back and forth between JS and C++. With the current JSM global-sharing mechanism (using NSVOs), we get the CCW performance gains, and memory reduction we want, but without requiring WithEnvironments.

	Ted Campbell [:tcampbell] Reporter
	Comment 1 • 6 years ago

Bug 1428489 also aims to fix blockers that prevent IonMonkey compilation in either of these loader cases.

	Ted Campbell [:tcampbell] Reporter
	Comment 2 • 6 years ago

These are the devtools loader prefs to disable:

https://searchfox.org/mozilla-central/rev/137f1b2f434346a0c3756ebfcbdbee4069e15dc8/devtools/client/shared/browser-loader.js#115
https://searchfox.org/mozilla-central/rev/137f1b2f434346a0c3756ebfcbdbee4069e15dc8/devtools/shared/Loader.jsm#65

	Ted Campbell [:tcampbell] Reporter
	Comment 3 • 6 years ago

I misunderstood the code. Turning off the pref is not what we want. We'd like a way to use NSVO combined with Sandbox. Right now only JSM loading can create the NSVO, so I'm exploring API changes.

	Alexandre Poirot [:ochameau]
	Comment 4 • 6 years ago

I think it should be able replacing this line:
https://searchfox.org/mozilla-central/source/devtools/shared/base-loader.js#196
to make it create a NSVO instead of a new sandbox object
and probably tweak loadSubScript call accordingly:
https://searchfox.org/mozilla-central/source/devtools/shared/base-loader.js#233

Note that, if that helps writting your patch, we can refactor base-loader to only support shareGlobal=true case.
This file is a simplified copy of old AddonSDK/Jetpack loader. But we only use it with shared globals.

	Ted Campbell [:tcampbell] Reporter
	Comment 5 • 6 years ago

As an update, I have a proof-of-concept at [1]. I need to sort out landing an API into xpc somewhere to create the NSVO. There are small bugs in devtools loader around 'this' binding that only (accidentally?) worked before for esoteric JS reasons (implicit 'this' forwarding for 'with'/object environments).

This gives some perf improvements, but I also have some JIT patches in progress to smooth things out (general wins for all chrome code). Once I'm happy with the JIT behavior, I'll sync up with Alexandre about any loader cleanup that might be done.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=478f4ab117e28c1cbe4168b602550ca3571e18ef

	Alexandre Poirot [:ochameau]
	Comment 6 • 6 years ago

(In reply to Ted Campbell [:tcampbell] from comment #5)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=478f4ab117e28c1cbe4168b602550ca3571e18ef

The win is mostly on:
    inspector.mutations -2.6%  (= the inspector is faster to refresh when DOM is updated)
and complicated.inspector.open -1.2%  (=the inspector is faster to load the first time we load in)

I was expecting a more general win, may be not very big, like here, but having an effect on most tests.

(In reply to Ted Campbell [:tcampbell] from comment #0)
> While this used to be an acceptable pattern, the perf of doing so is quite bad. We wind up
> using WithEnvironments and @@unscopables checks which bounces back and forth
> between JS and C++.

Is this something we can see with the profiler?
Is there symptomatic frames that indicate we are hitting this slow path?

Here is a profile when running all DAMP tests, that help identifying the common slownessses:
  https://perfht.ml/2EabI7p
  (because of bug 1431179 we only see the last 6 content processes, so we miss most of them
   but as devtools frontend runs in parent process, it shouldn't be an issue here)
  (here is some doc on how to run DAMP and get a profile:
   http://docs.firefox-dev.tools/tests/writing-perf-tests.html#how-to-run-your-new-test)

Searching for WithEnv, highlights "js::WithEnvironmentObject::create" but it only last for 40ms in this 400s+ profile.

	Ted Campbell [:tcampbell] Reporter
	Comment 7 • 6 years ago

Thanks for doing the analysis. There are for more things I need to optimize in the JITs to get further wins. (NSVO access ICs need to be added).

One problematic thing in the profile is |js::jit::DoGetNameFallback| (2.8sec). I hope it will almost all go away with NSVO + IC work. Those ICs are on my current work list.

	Ted Campbell [:tcampbell] Reporter
	Comment 8 • 6 years ago

Another aspect I see in those profiles is code seems to be running in Baseline still. Some of it is probably being blocked from being Ion compiled. It is a bit hard to tell from profile if things are blocked from compile, but something like |js::jit::DoGetNameFallback| only is called from Baseline code.

One way to get a sense of if this is happening a lot is to build browser with --enable-jitspew and then run with environment variable IONFLAGS=aborts set and look at the command line output. Things like async functions and generators currently do not compile in Ion.

	Alexandre Poirot [:ochameau]
	Comment 9 • 6 years ago

Hi Ted, any update about this bug?

In bug 1471853, we are getting results that may be explained by modules loaded through the loader being significantly slower. This performance regression prevent us from unifying our codebase and we are looking forward fixing the performance issues related by these patches.

Do we only miss some tweaks in base-loader.js? If not, and we miss patches in /js/, would you have WIP patches that we could try to see if the perf issue is related to NSVO?

Thanks!

	Alexandre Poirot [:ochameau]
	Comment 10 • 6 years ago

While investigating bug 1471853, I tried hacking around NSVO and reuse what is used for JSM.
And especially `js::NewJSMEnvironment`. I got something that "works", but with bad performance.
I suspect that this patch forces every module to be loaded in its own compartment.
It is not clear what is the final compartment being used whe I use NewJSMEnvironment like that:
  https://hg.mozilla.org/try/rev/be66d5df6133e270cbafd6c4d387cf652ba1423e
  See js/xpconnect/loader/mozJSSubScriptLoader.cpp
  And:
    sandbox = Services.scriptloader.createEnvObject();
  in devtools/shared/base-loader.js

	Alexandre Poirot [:ochameau]
	Comment 11 • 6 years ago

Could we possibly move forward with this WIP:
  https://hg.mozilla.org/try/rev/6edf40854a5906be7a2f8719eee03515fdc7cd8e
(We could take over this patch and lead it to mozilla-central)
While figuring out on our side how to integrate this with our loader.

I was just wondering with comment 7, if NSVO were ready to be used?
It is not clear if there is any blocker before being able to use NSVO for devtools other than getting the Components.utils method being expose and use it from the load?

> I hope it will almost all go away with NSVO + IC work. Those ICs are on my current work list.

What is "IC"? Has anything being done?

	Ted Campbell [:tcampbell] Reporter
	Comment 12 • 6 years ago

Alex and I synced up on this. The current state of NSVO is that are an implementation detail that will be phased out. It might still make sense to add devtools-only APIs to create environments backed on NSVOs. It will be important to avoid tying details of devtools modules and JSMs together as the freedom to change behavior/loaders of either is important.

A possible mid-term future might look like:
- A new parallel API to js::NewJSMEnvironment for devtools modules. Behind the scenes it would probably use NSVOs, but we don't want to tie devtools module loader behavior to JSM behavior.
- A new parallel API for ChromeUtils.Import (or a mode flag or whatever makes sense). Devtools would need to move away from loadSubscript entirely. Disallowing loadSubscript within devtools, and ignoring caching (which we should verify is already broken for devtools) would simplify this effort greatly.

Overall, it sounds like devtools is good shape to be able to support changes from load subscript. There are still some perf problems with using NSVOs but it is expected that things will be better than the current loadSubscript approach. The changes to the Gecko side are outside of my expertise and time-availability.

In the long-term, migrating to an ES6 Module flow is probably ideal (and JSMs should do the same), but there is no work progressing on that at the moment. As well, multiple-realms-per-compartment is on the way to being completed and will help cases that this environment stuff won't.