Closed Bug 1381418 Opened 2 years ago Closed 2 years ago

Stop using an iframe for developer tools

Categories

(DevTools :: General, enhancement, P2)

51 Branch
enhancement

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: miker, Assigned: miker)

References

(Blocks 1 open bug)

Details

(Whiteboard: [todo-mr][t1])

Using iframes to host each of our developer tools has a huge impact on toolbox startup time. Each iframe needs to be created and then we need to wait for the iframe's contentDocument to be created before continuing.

We should change the toolbox code in devtools/client/framework/toolbox.js::loadTool() to create the tools without an iframe... maybe using HTML imports?

First I will need to see what level of support we give to HTML imports as I know we have had issues with the spec.

Filter on EdgarMallory
Removing sub-iframes inside each tool seems right, but removing the iframes we use to load the tools themselves sounds like a more important design change that I'd like other people to comment on.

For me, the advantage of keeping iframes for each tool is that it gives us a way to make each tool an entire web page, making it possible to test each one in isolation, without the toolbox.
Flags: needinfo?(poirot.alex)
Flags: needinfo?(jryans)
Flags: needinfo?(bgrinstead)
iframes don't have such impact. What you are seeing is the tools html,js and css load time.
Loading an empty iframe is cheap. Loading each tool ressources are slow!

Also, iframes are fantastic tools to segregate the memory by panel. They ensure that everything really go away if you remove one. They are also very beneficial to load things more easily.

Regarding performances we shouldn't do any premature optimization based on thoughts.
You should use the profiler or any other tooling (even dump/console.log/...) for making your assumptions!
Flags: needinfo?(poirot.alex)
(In reply to Alexandre Poirot [:ochameau] from comment #2)
> iframes don't have such impact. What you are seeing is the tools html,js and
> css load time.
> Loading an empty iframe is cheap. Loading each tool ressources are slow!
> 

We know it is cheap but how cheap? We are still investigating but when some tools are 3 iframes deep then these things need to be investigated.

> Also, iframes are fantastic tools to segregate the memory by panel. They
> ensure that everything really go away if you remove one. They are also very
> beneficial to load things more easily.
> 

I completely agree. My plan is to do some much more granular measurements before making any changes... I mean, if loading the markup view, which is 3 iframes deep, doesn't start until 700ms in we need to find out why.

> Regarding performances we shouldn't do any premature optimization based on
> thoughts.

That is not what we are doing here:
https://docs.google.com/spreadsheets/d/1pMGVudyqe5TV5mIkilfVRGX5u6Soc0ksPvBrfQdebX4/edit#gid=1156663486

And:
https://gist.github.com/MikeRatcliffe/56096f05d5de4aa4ce7c215f80987456

By far the thing that seems to be slowing things down the most is iframe initialization... we are still investigating but it is clear that the sidebar tools that don't use iframes only take around 25% of the time of our main tools and some of those tools are also fairly complex.

> You should use the profiler or any other tooling (even dump/console.log/...)
> for making your assumptions!

Of course not... I am planning on adding a whole host of timers to get a real granular list of timings as the tools load. I have chosen not to use the profiler for this because we only need to see e.g. iframe init times, load times, js parse, compile times etc. and these things can be easily listed using the kind of probe I want to use.
No longer depends on: 1381416
No longer blocks: 1381428
Depends on: 1381428
An iframe should take about 5ms to create and 25ms to fire its load event.
Then it is all about what you do within it.

What you see is ressources being loaded in each iframe.
If you happen to re-import in each iframe a lot of ressouces, it will surely increase loading time.
Did you verified that only the necessary ressources are being loaded in each of these sub-iframes?
Are we re-using React from the toolbox in these frames?
Isn't there more easy win: lazy loading? reusing ressources from the parent iframe like what we do for React? Stripping useless costly dependencies (like SDK)?

Please don't transform work on performance into yet another gigantic refactoring.
You are opening bugs with clear intents, it would be great to have the numbers upfront that justify each refactoring.
(In reply to Patrick Brosset <:pbro> from comment #1)
> Removing sub-iframes inside each tool seems right, but removing the iframes
> we use to load the tools themselves sounds like a more important design
> change that I'd like other people to comment on.

I agree about subframes in a tool - if they are causing perf issues and it makes sense architecturally we should get rid of them. The main one I can think of here in the core tools is the markupview, are there others?

> For me, the advantage of keeping iframes for each tool is that it gives us a
> way to make each tool an entire web page, making it possible to test each
> one in isolation, without the toolbox.

I'm against collapsing each tool into the same toolbox frame. It's nice to have the isolation because:
1) If one tool fails / gets corrupted it doesn't break the toolbox / other tools
2) I believe the web extension apis assume addons are framed - and regardless we wouldn't want addons to run in the top level frame
3) It provides CSS scoping which prevents changes in one tool from affecting other tools
4) Allows for 'launchpad' integration for HTML-ified tools (being able to develop them in a normal webpage)

But please do follow up with more details on the impact the frames are having on startup as you get measurements. I know that re-loading and parsing shared libraries (like react) for each frame is slow - I wonder if the JavaScript Start-up Bytecode Cache might be able to help here (https://groups.google.com/d/msg/mozilla.dev.platform/4u6EsaDC7kY/1TxDr3-BAgAJ).
Flags: needinfo?(bgrinstead)
I agree with the points Alex and Brian have raised.

Removing subframes within a tool seems fine in general.

At the tool level, frames offer a lot of useful isolation properties that seem important to preserve, especially since add-ons can add their own arbitrary content we don't control.

Instead, let's investigate other ways to improve tool startup, like sharing scripts across multiple tools, lazy loading, etc.
Flags: needinfo?(jryans)
To be clear, the purpose of this bug is discussion, which is what we have here.

(In reply to Alexandre Poirot [:ochameau] from comment #4)
> An iframe should take about 5ms to create and 25ms to fire its load event.
> Then it is all about what you do within it.
> 

Thanks, it is great to have some timing although I am adding probes to see how long this is taking in our case.

> What you see is ressources being loaded in each iframe.
> If you happen to re-import in each iframe a lot of ressouces, it will surely
> increase loading time.

Apparently, the browser already de-duplicates resources but, yes, we are planning on looking into that.

> Did you verified that only the necessary ressources are being loaded in each
> of these sub-iframes?

Nope, that is part of another bug.

> Are we re-using React from the toolbox in these frames?

On some, but not all.

> Isn't there more easy win: lazy loading? reusing ressources from the parent
> iframe like what we do for React? Stripping useless costly dependencies
> (like SDK)?
> 

We are investigating lazy loading as part of another bug.

> Please don't transform work on performance into yet another gigantic
> refactoring.

We don't plan to... we are just looking at our options at this stage.

> You are opening bugs with clear intents, it would be great to have the
> numbers upfront that justify each refactoring.

The bugs are opened for discussion and investigation... not necessarily for a particular action to be taken.

(In reply to Brian Grinstead [:bgrins] from comment #5)
> (In reply to Patrick Brosset <:pbro> from comment #1)
> > Removing sub-iframes inside each tool seems right, but removing the iframes
> > we use to load the tools themselves sounds like a more important design
> > change that I'd like other people to comment on.
> 
> I agree about subframes in a tool - if they are causing perf issues and it
> makes sense architecturally we should get rid of them. The main one I can
> think of here in the core tools is the markupview, are there others?
> 

Markup view is 3 levels deep and slowing the start of the inspector. Even with Alex's timings of 5ms creation and 25ms onload that is 90ms purely for iframe creation. The sidebar tools were removed from iframes and now only take 25% as long as the main tools to load so that is why we are discussing this here.

> > For me, the advantage of keeping iframes for each tool is that it gives us a
> > way to make each tool an entire web page, making it possible to test each
> > one in isolation, without the toolbox.
> 
> I'm against collapsing each tool into the same toolbox frame. It's nice to
> have the isolation because:
> 1) If one tool fails / gets corrupted it doesn't break the toolbox / other
> tools
> 2) I believe the web extension apis assume addons are framed - and
> regardless we wouldn't want addons to run in the top level frame
> 3) It provides CSS scoping which prevents changes in one tool from affecting
> other tools
> 4) Allows for 'launchpad' integration for HTML-ified tools (being able to
> develop them in a normal webpage)
> 

100% agreed.

> But please do follow up with more details on the impact the frames are
> having on startup as you get measurements. I know that re-loading and
> parsing shared libraries (like react) for each frame is slow.

That is the plan... I also plan on adding some kind of regression testing for this if we can.

> I wonder if
> the JavaScript Start-up Bytecode Cache might be able to help here
> (https://groups.google.com/d/msg/mozilla.dev.platform/4u6EsaDC7kY/1TxDr3-
> BAgAJ).

I asked John Galt about this and he says that our DevTools code is already in the startup cache (preparsed and compiled).
I just read the discussion again and will close the bug based on it.
It sounds like people are in favor of keeping iframes for each tools. However removing second (and third?) level iframes sounds like a good plan, and I think the main culprit here is the markup view which is already tracked in bug 1381416.
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.