perf(material/tooltip): Defer injection of injectables not needed unt… #30440
Conversation
angular-robot
bot
added
area: performance
kseamon
force-pushed
the
tooltip-inject
branch
from
2f01128 to
584d528
Compare
…il tooltip is shown.
kseamon
force-pushed
the
tooltip-inject
branch
from
efe3a80 to
89face5
Compare
kseamon
requested review from
crisbeto and
andrewseguin
and removed request for
a team
| @@ -187,10 +187,7 @@ const MAX_WIDTH = 200; | |||
| }, | |||
| }) | |||
| export class MatTooltip implements OnDestroy, AfterViewInit { | |||
| private _overlay = inject(Overlay); | |||
There was a problem hiding this comment.
Do we know what the impact of this is? These are likely already instantiated by the time the tooltip is rendered.
There was a problem hiding this comment.
The impact is to move a small cost (resolving the injectable) from instantiation time to if/when the tooltip is first shown.
The cost should be small enough that it's not noticeable when showing a tooltip, but does add up during instantiation if lots of MatTooltip instances are being created (imagine a big screen full of content that has tooltips).
Further, most MatTooltip instances will be created but not shown, so on net this reduces the cpu burden of MatTooltip, even if only a smidge.
There was a problem hiding this comment.
From an example profiler run:
Of the 15.3ms spent instantiating MatTooltip instances, 8.8ms was spent in calls to inject(). This PR removes 1/3 of the inject calls, so we might naively guess that it saves about 3ms, or almost 1/5 of overall MatTooltip instantiation time.
There was a problem hiding this comment.
Is that 15ms per tooltip instance or 15ms for all the instances on the page? I would be hesitant with sprinkling this pattern across the codebase since it makes the component harder to navigate IMO. The tooltip is already a bit messy because of all the manual even bindings.
There was a problem hiding this comment.
This is across many instances, which is sort of the point.
For the page I'm profiling, a big mat-table, the only Directive/component whose factory/constructor took more time is MatCell (3.3% of table render time), and there are a lot more of those (alas chrome's profiler doesn't give exact numbers). MatTooltip's factory took 1.3%, which is quite significant considering how much else is being rendered.
Note: I separately asked Jeremy and Andrew if I could add a lazy version of inject() to the framework and they suggested keep with this pattern instead for the time being.
There was a problem hiding this comment.
Alright, I think it makes sense for commonly used components like MatTooltip, but I don't think that we should apply it everywhere.
andrewseguin
added
action: merge
|
The changes were merged into the following branches: main, 19.1.x |
angular#30440 deferred the injection of some dependencies in the tooltip, including `ViewContainerRef`. This is problematic, because the act of injecting `ViewContainerRef` changes the shape of some internal data structures in the framework's runtime. As a result, hovering over a tooltip that has been hydrated will throw a runtime error because the structure from the server no longer matches the structure on the client. Fixes angular#30498.
…0500) #30440 deferred the injection of some dependencies in the tooltip, including `ViewContainerRef`. This is problematic, because the act of injecting `ViewContainerRef` changes the shape of some internal data structures in the framework's runtime. As a result, hovering over a tooltip that has been hydrated will throw a runtime error because the structure from the server no longer matches the structure on the client. Fixes #30498.
…0500) #30440 deferred the injection of some dependencies in the tooltip, including `ViewContainerRef`. This is problematic, because the act of injecting `ViewContainerRef` changes the shape of some internal data structures in the framework's runtime. As a result, hovering over a tooltip that has been hydrated will throw a runtime error because the structure from the server no longer matches the structure on the client. Fixes #30498. (cherry picked from commit 68b267d)
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
…il tooltip is shown.