1463908 - tooltip text provider uses about 20KB of memory per-process

Status: RESOLVED FIXED

(Firefox :: General, enhancement)

Description

[Boris Zbarsky [:bzbarsky]]

It's not a huge thing, but we have a JS-implemented default tooltip text provider and instantiating that service (which we do in all content processes via the ChromeTooltipListener) costs about 20KB of RAM.

Comment 1

[:Gijs (he/him)]

Maybe Felipe has ideas here. :-)

Comment 2

[:Felipe Gomes (needinfo for replies!)]

From what I found, the default tooltip provider is something that is needed in content processes, as it's responsible for providing tooltips for a lot of things (e.g., <input file>), etc.

However, it shouldn't need to be instantiated until getNodeText is called, from sTooltipCallback.

There are three options that we could do to reduce this memory overhead:

A) Rewrite the default tooltip provider in C++ so it doesn't have a per-process memory cost
B) Lazily instantiate the ChromeTooltipListener
C) Lazily instantiate the default tooltip provider


Option C) is super easy (as it is instantiated in the ChromeTooltipListener constructor, but it could be only when sTooltipCallback is called), and it should be more than enough to make this no longer an issue, so I'm gonna go with it as it's the simplest one.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 1463908 - Lazily instatiate the tooltip text provider.

The text provider will be instantiated only when sTooltipCallback is fired, which is triggered from user interactions (mousemove or keyboard navigation). This will avoid it being instantiated during startup, or for non-interactive content processes

Review commit: https://reviewboard.mozilla.org/r/247406/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/247406/

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Two things: I initially wrote this just returning a raw pointer, but I wasn't sure if it was certain that the object could be gone by the time this is called..  Thinking more about it, I think it would be ok

There's a change in behavior in that, if the do_getService calls fail, it will keep retrying now.. I believe it doesn't matter for Firefox, but let me know if I should keep the old behavior

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8981300 [details]
Bug 1463908 - Lazily instatiate the tooltip text provider.

https://reviewboard.mozilla.org/r/247406/#review254156

::: docshell/base/nsDocShellTreeOwner.h:172
(Diff revision 1)
>    NS_IMETHOD HideTooltip();
>  
>    nsWebBrowser* mWebBrowser;
>    nsCOMPtr<mozilla::dom::EventTarget> mEventTarget;
>    nsCOMPtr<nsITooltipTextProvider> mTooltipTextProvider;
> +  already_AddRefed<nsITooltipTextProvider> GetTooltipTextProvider();

Please put the methe next to the other methods, before the data members start.

::: docshell/base/nsDocShellTreeOwner.cpp:1063
(Diff revision 1)
>      mTooltipTextProvider = do_GetService(NS_DEFAULTTOOLTIPTEXTPROVIDER_CONTRACTID);
>    }
> -}
>  
> -ChromeTooltipListener::~ChromeTooltipListener()
> -{
> +  nsCOMPtr<nsITooltipTextProvider> provider = mTooltipTextProvider;
> +  return provider.forget();

Could also just return mTooltipTextProvider and change the return type to nsITooltipTextProvider*.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8981300 [details]
Bug 1463908 - Lazily instatiate the tooltip text provider.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/247406/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/53732e14e552
Lazily instatiate the tooltip text provider. r=bz

Comment 8

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/53732e14e552