Skip to content

perf(material/tooltip): Defer injection of injectables not needed unt… #30440

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 7, 2025
Merged

perf(material/tooltip): Defer injection of injectables not needed unt… #30440

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 13 additions & 15 deletions src/material/tooltip/tooltip.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,10 +187,7 @@ const MAX_WIDTH = 200;
},
})
export class MatTooltip implements OnDestroy, AfterViewInit {
private _overlay = inject(Overlay);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know what the impact of this is? These are likely already instantiated by the time the tooltip is rendered.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I think it makes sense for commonly used components like MatTooltip, but I don't think that we should apply it everywhere.

private _elementRef = inject<ElementRef<HTMLElement>>(ElementRef);
private _scrollDispatcher = inject(ScrollDispatcher);
private _viewContainerRef = inject(ViewContainerRef);
private _ngZone = inject(NgZone);
private _platform = inject(Platform);
private _ariaDescriber = inject(AriaDescriber);
Expand All @@ -209,7 +206,6 @@ export class MatTooltip implements OnDestroy, AfterViewInit {
private _positionAtOrigin: boolean = false;
private _disabled: boolean = false;
private _tooltipClass: string | string[] | Set<string> | {[key: string]: any};
private _scrollStrategy = inject(MAT_TOOLTIP_SCROLL_STRATEGY);
private _viewInitialized = false;
private _pointerExitEventsInitialized = false;
private readonly _tooltipComponent = TooltipComponent;
Expand Down Expand Up @@ -362,9 +358,6 @@ export class MatTooltip implements OnDestroy, AfterViewInit {
private readonly _passiveListeners: (readonly [string, EventListenerOrEventListenerObject])[] =
[];

/** Reference to the current document. */
private _document = inject(DOCUMENT);

/** Timer started at the last `touchstart` event. */
private _touchstartTimeout: null | ReturnType<typeof setTimeout> = null;

Expand Down Expand Up @@ -462,7 +455,8 @@ export class MatTooltip implements OnDestroy, AfterViewInit {
const overlayRef = this._createOverlay(origin);
this._detach();
this._portal =
this._portal || new ComponentPortal(this._tooltipComponent, this._viewContainerRef);
this._portal ||
new ComponentPortal(this._tooltipComponent, this._injector.get(ViewContainerRef));
const instance = (this._tooltipInstance = overlayRef.attach(this._portal).instance);
instance._triggerElement = this._elementRef.nativeElement;
instance._mouseLeaveHideDelay = this._hideDelay;
Expand Down Expand Up @@ -512,12 +506,14 @@ export class MatTooltip implements OnDestroy, AfterViewInit {
this._detach();
}

const scrollableAncestors = this._scrollDispatcher.getAncestorScrollContainers(
this._elementRef,
);
const scrollableAncestors = this._injector
.get(ScrollDispatcher)
.getAncestorScrollContainers(this._elementRef);

const overlay = this._injector.get(Overlay);

// Create connected position strategy that listens for scroll events to reposition.
const strategy = this._overlay
const strategy = overlay
.position()
.flexibleConnectedTo(this.positionAtOrigin ? origin || this._elementRef : this._elementRef)
.withTransformOriginOn(`.${this._cssClassPrefix}-tooltip`)
Expand All @@ -537,11 +533,11 @@ export class MatTooltip implements OnDestroy, AfterViewInit {
}
});

this._overlayRef = this._overlay.create({
this._overlayRef = overlay.create({
direction: this._dir,
positionStrategy: strategy,
panelClass: `${this._cssClassPrefix}-${PANEL_CLASS}`,
scrollStrategy: this._scrollStrategy(),
scrollStrategy: this._injector.get(MAT_TOOLTIP_SCROLL_STRATEGY)(),
});

this._updatePosition(this._overlayRef);
Expand Down Expand Up @@ -874,7 +870,9 @@ export class MatTooltip implements OnDestroy, AfterViewInit {
/** Listener for the `wheel` event on the element. */
private _wheelListener(event: WheelEvent) {
if (this._isTooltipVisible()) {
const elementUnderPointer = this._document.elementFromPoint(event.clientX, event.clientY);
const elementUnderPointer = this._injector
.get(DOCUMENT)
.elementFromPoint(event.clientX, event.clientY);
const element = this._elementRef.nativeElement;

// On non-touch devices we depend on the `mouseleave` event to close the tooltip, but it
Expand Down
Loading