Use read-ahead when loading plugin shared libraries

RESOLVED WONTFIX

Status

()

Core
Plug-ins
RESOLVED WONTFIX
5 years ago
4 years ago

People

(Reporter: vladan, Assigned: aklotz)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy:P1][leave open])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
I frequently get main-thread jank of 100ms or more from the initial load of the Flash library. This is also the #1 source of chrome-hangs:

PR_WaitCondVar (in nspr4.dll)
 -> mozilla::CondVar::Wait(unsigned int) (in xul.dll)
 -> mozilla::ipc::GeckoChildProcessHost::SyncLaunch (in xul.dll)
 -> mozilla::plugins::PluginProcessParent::Launch(int) (in xul.dll)
 -> mozilla::plugins::PluginModuleParent::LoadModule(char const *) (in xul.dll)
 -> GetNewPluginLibrary(nsPluginTag *) (in xul.dll)
 -> nsNPAPIPlugin::CreatePlugin(nsPluginTag *,nsNPAPIPlugin * *) (in xul.dll)
 -> CreateNPAPIPlugin (in xul.dll)
 -> nsPluginHost::EnsurePluginLoaded(nsPluginTag *) (in xul.dll)

We should figure out if reading plugin libraries with the read-ahead flag (à la xul.dll) will help with plugin jank.  We can start by adding a histogram similar to PLUGIN_SHUTDOWN_MS.

* Preloading the Flash library after startup is another possibility but we can look into that in a separate bug if this fails.
Created attachment 710811 [details] [diff] [review]
Adds telemetry for plugin library load timing
Attachment #710811 - Flags: review?(benjamin)
Comment on attachment 710811 [details] [diff] [review]
Adds telemetry for plugin library load timing

Cancelling review request, this needs more work
Attachment #710811 - Flags: review?(benjamin)
Created attachment 710836 [details] [diff] [review]
Adds telemetry for plugin library load timing rev. 2

Here's the corrected version: Added telemetry for launching plugin-container in addition to in-process plugins. The histograms are separate for in-process vs OOP so that we aren't comparing apples to oranges.
Attachment #710811 - Attachment is obsolete: true
Attachment #710836 - Flags: review?(benjamin)
Created attachment 710867 [details] [diff] [review]
Adds telemetry for plugin library load timing rev. 3

Forgot to include Telemetry.h
Attachment #710836 - Attachment is obsolete: true
Attachment #710836 - Flags: review?(benjamin)
Attachment #710867 - Flags: review?(benjamin)

Comment 5

5 years ago
Comment on attachment 710867 [details] [diff] [review]
Adds telemetry for plugin library load timing rev. 3

The probe you have here won't actually hit the jank stack from comment 0: this codepath is for in-process plugins only (so only Java on Windows by default).

But note that probably most of the time spent here is in launching the process, not loading the library. We do also sometimes load the library (on Windows) in-process in order to pull the metadata such as the plugin MIME types/versions which are stored in the resource block.
Attachment #710867 - Flags: review?(benjamin) → review-
(Reporter)

Comment 6

5 years ago
The load times might be less variable if we limit ourselves to only timing the Flash library
(Reporter)

Comment 7

5 years ago
^ as opposed to other plugins
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #5)
> Comment on attachment 710867 [details] [diff] [review]
> Adds telemetry for plugin library load timing rev. 3
> 
> But note that probably most of the time spent here is in launching the
> process, not loading the library. We do also sometimes load the library (on
> Windows) in-process in order to pull the metadata such as the plugin MIME
> types/versions which are stored in the resource block.
My idea was that measuring the time from plugin-container launch to opening of the channel could be a proxy for the load time. We're not really interested in the load time itself; we're more interested in the change in load times between now and after the readahead patch lands.

Any suggestions?
Depends on: 845907

Comment 9

5 years ago
Benjamin is there a better way to measure plugin dll-loading overhead for out-of-process plugins? We need to test whether readahead will reduce occurrences of blocking on stack in comment 0.
Flags: needinfo?(benjamin)

Comment 10

5 years ago
I think this got resolved in person: measuring process launch time is going to be really noisy, so it would be valuable to measure the in-process load time (just for metadata) and to measure the load times in the other process I think we need to measure directly there.
Flags: needinfo?(benjamin)
Created attachment 728126 [details] [diff] [review]
Adds telemetry for plugin library metadata load timing

This patch is to measure the plugin metadata load time as discussed earlier this week.
Attachment #710867 - Attachment is obsolete: true
Attachment #728126 - Flags: review?(benjamin)
Created attachment 728128 [details] [diff] [review]
Patch for adding plugin readahead

This is the patch that actually implements the plugin readahead. This won't land until after we have gathered sufficient telemetry for the "before" case.
Attachment #728128 - Flags: review?(benjamin)

Updated

5 years ago
Attachment #728126 - Flags: review?(benjamin) → review+

Updated

5 years ago
Attachment #728128 - Flags: review?(benjamin) → review+
Telemetry patch:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b23e5f546045
Whiteboard: [Snappy:P1] → [Snappy:P1][leave open]
https://hg.mozilla.org/mozilla-central/rev/b23e5f546045
(Reporter)

Comment 15

4 years ago
Are we landing the 2nd patch?
Flags: needinfo?(aklotz)
Data gathered via SystemTap runs showed a significant increase in load time when testing with libflashplayer.so with readahead.

Average load time without readahead: 5419ms
Average load time with readahead: 13551ms

(Note that the hardware used for this test is fairly old, so the non-readahead baseline is pretty bad to begin with)
Flags: needinfo?(aklotz)
So was this found to not improve the situation and can be closed?
Correct.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.