Skip to content

Memoizes the functions for getting header both runs and scalar data tables #6836

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 3 commits into from
Apr 24, 2024
Merged

Memoizes the functions for getting header both runs and scalar data tables #6836

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
627d64d
Memoizes column header functions
hoonji Apr 21, 2024
24d5504
Fixes test
hoonji Apr 21, 2024
98f0260
Merge branch 'master' into memoize_column_fn
hoonji Apr 22, 2024
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
1 change: 1 addition & 0 deletions tensorboard/webapp/metrics/views/card_renderer/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,7 @@ tf_ng_module(
":utils",
"//tensorboard/webapp/metrics:types",
"//tensorboard/webapp/runs:types",
"//tensorboard/webapp/util:memoize",
"//tensorboard/webapp/widgets/card_fob:types",
"//tensorboard/webapp/widgets/data_table",
"//tensorboard/webapp/widgets/data_table:types",
Expand Down
4 changes: 2 additions & 2 deletions tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ng.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
(loadAllColumns)="loadAllColumns.emit()"
>
<ng-container header>
<ng-container *ngFor="let header of getHeaders()">
<ng-container *ngFor="let header of extendHeaders(columnHeaders)">
<tb-data-table-header-cell
*ngIf="header.enabled && (header.type !== ColumnHeaderType.SMOOTHED || smoothingEnabled)"
[header]="header"
Expand All @@ -42,7 +42,7 @@
<ng-container content>
<ng-container *ngFor="let dataRow of getTimeSelectionTableData()">
<tb-data-table-content-row>
<ng-container *ngFor="let header of getHeaders()">
<ng-container *ngFor="let header of extendHeaders(columnHeaders)">
<tb-data-table-content-cell
*ngIf="header.enabled && (header.type !== ColumnHeaderType.SMOOTHED || smoothingEnabled)"
[header]="header"
Expand Down
27 changes: 18 additions & 9 deletions tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import {
AddColumnEvent,
} from '../../../widgets/data_table/types';
import {isDatumVisible} from './utils';
import {memoize} from '../../../util/memoize';

@Component({
selector: 'scalar-card-data-table',
Expand Down Expand Up @@ -74,15 +75,23 @@ export class ScalarCardDataTable {

ColumnHeaderType = ColumnHeaderType;

getHeaders(): ColumnHeader[] {
return [
{
name: 'color',
displayName: '',
type: ColumnHeaderType.COLOR,
enabled: true,
},
].concat(this.columnHeaders);
// Columns must be memoized to stop needless re-rendering of the content and headers in these
// columns. This has been known to cause problems with the controls in these columns,
// specifically the add button.
extendHeaders = memoize(this.internalExtendHeaders);

private internalExtendHeaders(headers: ColumnHeader[]) {
return ([] as Array<ColumnHeader>).concat(
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, any idea why we are using concat instead of the spread operator?

return [
        {
          name: 'color',
          displayName: '',
          type: ColumnHeaderType.COLOR,
          enabled: true,
        },
        ...headers
      ],
    );

Is it also for performance?

Copy link
Contributor

Choose a reason for hiding this comment

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

Concat is more performant.

[
{
name: 'color',
displayName: '',
type: ColumnHeaderType.COLOR,
enabled: true,
},
],
headers
);
}

getMinPointInRange(
Expand Down
1 change: 1 addition & 0 deletions tensorboard/webapp/runs/views/runs_table/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ tf_ng_module(
"//tensorboard/webapp/types:ui",
"//tensorboard/webapp/util:colors",
"//tensorboard/webapp/util:matcher",
"//tensorboard/webapp/util:memoize",
"//tensorboard/webapp/widgets/custom_modal",
"//tensorboard/webapp/widgets/data_table",
"//tensorboard/webapp/widgets/data_table:filter_dialog",
Expand Down
4 changes: 2 additions & 2 deletions tensorboard/webapp/runs/views/runs_table/runs_data_table.ng.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@
(loadAllColumns)="loadAllColumns.emit()"
>
<ng-container header>
<ng-container *ngFor="let header of getHeaders()">
<ng-container *ngFor="let header of extendHeaders(headers)">
<tb-data-table-header-cell
[header]="header"
[sortingInfo]="sortingInfo"
Expand All @@ -67,7 +67,7 @@
<ng-container content>
<ng-container *ngFor="let dataRow of data; trackBy: trackByRuns">
<tb-data-table-content-row [attr.data-id]="dataRow.id">
<ng-container *ngFor="let header of getHeaders()">
<ng-container *ngFor="let header of extendHeaders(headers)">
<tb-data-table-content-cell
[header]="header"
[datum]="dataRow[header.name]"
Expand Down
47 changes: 23 additions & 24 deletions tensorboard/webapp/runs/views/runs_table/runs_data_table.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {
ReorderColumnEvent,
AddColumnEvent,
} from '../../../widgets/data_table/types';
import {memoize} from '../../../util/memoize';
@Component({
selector: 'runs-data-table',
templateUrl: 'runs_data_table.ng.html',
Expand Down Expand Up @@ -67,32 +68,30 @@ export class RunsDataTable {
@Output() addFilter = new EventEmitter<FilterAddedEvent>();
@Output() loadAllColumns = new EventEmitter<null>();

// These columns must be stored and reused to stop needless re-rendering of
// the content and headers in these columns. This has been known to cause
// problems with the controls in these columns, specifically the color picker.
beforeColumns = [
{
name: 'selected',
displayName: '',
type: ColumnHeaderType.CUSTOM,
enabled: true,
},
];
// Columns must be memoized to stop needless re-rendering of the content and headers in these
// columns. This has been known to cause problems with the controls in these columns,
// specifically the add button.
extendHeaders = memoize(this.internalExtendHeaders);

afterColumns = [
{
name: 'color',
displayName: '',
type: ColumnHeaderType.COLOR,
enabled: true,
},
];

getHeaders() {
private internalExtendHeaders(headers: ColumnHeader[]) {
return ([] as Array<ColumnHeader>).concat(
this.beforeColumns,
this.headers,
this.afterColumns
[
{
name: 'selected',
displayName: '',
type: ColumnHeaderType.CUSTOM,
enabled: true,
},
],
headers,
[
{
name: 'color',
displayName: '',
type: ColumnHeaderType.COLOR,
enabled: true,
},
]
);
}

Expand Down
10 changes: 5 additions & 5 deletions tensorboard/webapp/runs/views/runs_table/runs_data_table_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,25 +220,25 @@ describe('runs_data_table', () => {
});
});

// If the call to getHeaders produces a new color header on each call, then
// If the call to extendHeaders produces a new color header on each call, then
// the content gets re-rendered when the color picker opens. This causes the
// color picker modal to close immediately. This test ensures that we keep
// the same reference to the color header and continue to use that reference
// instead of creating a new one on each call.
it('getHeaders does not produce new color header on each call', () => {
it('extendHeaders does not produce new color header on each call', () => {
const fixture = createComponent({});

const runsDataTable = fixture.debugElement.query(
By.directive(RunsDataTable)
);
const {headers} = runsDataTable.componentInstance;

expect(
runsDataTable.componentInstance
.getHeaders()
.extendHeaders(headers)
.find((header: ColumnHeader) => header.name === 'color')
).toBe(
runsDataTable.componentInstance
.getHeaders()
.extendHeaders(headers)
.find((header: ColumnHeader) => header.name === 'color')
);
});
Expand Down