Closed Bug 836488 Opened 7 years ago Closed 6 years ago

Use read-ahead when loading plugin shared libraries

Categories

(Core :: Plug-ins, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: vladan, Assigned: aklotz)

References

Details

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

Attachments

(2 files, 3 obsolete files)

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.
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)
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)
Forgot to include Telemetry.h
Attachment #710836 - Attachment is obsolete: true
Attachment #710836 - Flags: review?(benjamin)
Attachment #710867 - Flags: review?(benjamin)
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-
The load times might be less variable if we limit ourselves to only timing the Flash library
^ 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
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)
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)
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)
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)
Attachment #728126 - Flags: review?(benjamin) → review+
Attachment #728128 - Flags: review?(benjamin) → review+
Telemetry patch:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b23e5f546045
Whiteboard: [Snappy:P1] → [Snappy:P1][leave open]
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
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.