Skip to content
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.

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

feat(debug): Add timing information about time to first image/all images, and query time #3681

Merged
merged 5 commits into from
Oct 3, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
3 changes: 3 additions & 0 deletions extensions/cornerstone/src/init.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import interleaveTopToBottom from './utils/interleaveTopToBottom';
import initContextMenu from './initContextMenu';
import initDoubleClick from './initDoubleClick';
import { CornerstoneServices } from './types';
import initViewTiming from './utils/initViewTiming';

// TODO: Cypress tests are currently grabbing this from the window?
window.cornerstone = cornerstone;
Expand Down Expand Up @@ -235,6 +236,8 @@ export default async function init({
element.addEventListener(EVENTS.CAMERA_RESET, resetCrosshairs);

eventTarget.addEventListener(EVENTS.STACK_VIEWPORT_NEW_STACK, newStackCallback);

initViewTiming({ element, eventTarget });
}

function elementDisabledHandler(evt) {
Expand Down
45 changes: 45 additions & 0 deletions extensions/cornerstone/src/utils/initViewTiming.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import { log, Types } from '@ohif/core';
import { EVENTS } from '@cornerstonejs/core';

const { TimingEnum } = Types;

const IMAGE_TIMING_KEYS = [
TimingEnum.DISPLAY_SETS_TO_ALL_IMAGES,
TimingEnum.DISPLAY_SETS_TO_FIRST_IMAGE,
TimingEnum.STUDY_TO_FIRST_IMAGE,
];

const imageTiming = {
viewportsWaiting: 0,
};

/**
* Defines the initial view timing reporting.
* This allows knowing how many viewports are waiting for initial views and
* when the IMAGE_RENDERED gets sent out.
* The first image rendered will fire the FIRST_IMAGE timeEnd logs, while
* the last of the enabled viewport will fire the ALL_IMAGES timeEnd logs.
*
*/

export default function initViewTiming({ element }) {
if (!IMAGE_TIMING_KEYS.find(key => log.timingKeys[key])) {
return;
}
imageTiming.viewportsWaiting += 1;
element.addEventListener(EVENTS.IMAGE_RENDERED, imageRenderedListener);
}

function imageRenderedListener(evt) {
if (evt.detail.viewportStatus === 'preRender') {
Copy link
Member

Choose a reason for hiding this comment

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

this has a bug regarding which image rendered, you should check the viewportId and check if it is the same id no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, because we only care that the count of viewports matches - we un subscribe after the first event that is rendered from each viewport, so each viewport can only get 1 count for it, and we have the total count number. The logic of the event handling deals with matching up viewport id's to renders. The logging isn't for this viewport rendered for first image, it is for any cornerstone viewport rendering.

return;
}
log.timeEnd(TimingEnum.DISPLAY_SETS_TO_FIRST_IMAGE);
log.timeEnd(TimingEnum.STUDY_TO_FIRST_IMAGE);
log.timeEnd(TimingEnum.SCRIPT_TO_VIEW);
imageTiming.viewportsWaiting -= 1;
evt.detail.element.removeEventListener(EVENTS.IMAGE_RENDERED, imageRenderedListener);
if (!imageTiming.viewportsWaiting) {
log.timeEnd(TimingEnum.DISPLAY_SETS_TO_ALL_IMAGES);
}
}
7 changes: 4 additions & 3 deletions platform/app/public/html-templates/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@
name="msapplication-config"
content="<%= PUBLIC_URL %>assets/browserconfig.xml"
/>
<script type="text/javascript">
console.time('scriptToView');
window.PUBLIC_URL = '<%= PUBLIC_URL %>';
</script>
<link
rel="manifest"
href="<%= PUBLIC_URL %>manifest.json"
Expand Down Expand Up @@ -183,9 +187,6 @@
<!-- Targets IE11 -->
<!-- <script
src="https://polyfill.io/v3/polyfill.min.js?flags=gated&features=default%2CObject.values%2CArray.prototype.flat%2CObject.entries%2CSymbol%2CArray.prototype.includes%2CString.prototype.repeat%2CArray.prototype.find"></script>!-->
<script type="text/javascript">
window.PUBLIC_URL = '<%= PUBLIC_URL %>';
</script>
<script
rel="preload"
as="script"
Expand Down
8 changes: 6 additions & 2 deletions platform/app/src/routes/DataSourceWrapper.tsx
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
/* eslint-disable react/jsx-props-no-spreading */
import React, { useCallback, useEffect, useState } from 'react';
import PropTypes from 'prop-types';
import { ExtensionManager, MODULE_TYPES } from '@ohif/core';
import { ExtensionManager, MODULE_TYPES, Types, log } from '@ohif/core';
//
import { extensionManager } from '../App.tsx';
import { useParams, useLocation } from 'react-router';
import { useNavigate } from 'react-router-dom';
import useSearchParams from '../hooks/useSearchParams.ts';

const { TimingEnum } = Types;

/**
* Determines if two React Router location objects are the same.
*/
Expand Down Expand Up @@ -149,7 +151,7 @@ function DataSourceWrapper(props) {
// 204: no content
async function getData() {
setIsLoading(true);

log.time(TimingEnum.SEARCH_TO_LIST);
const studies = await dataSource.query.studies.search(queryFilterValues);

setData({
Expand All @@ -159,6 +161,8 @@ function DataSourceWrapper(props) {
pageNumber: queryFilterValues.pageNumber,
location,
});
log.timeEnd(TimingEnum.SCRIPT_TO_VIEW);
log.timeEnd(TimingEnum.SEARCH_TO_LIST);

setIsLoading(false);
}
Expand Down
10 changes: 8 additions & 2 deletions platform/app/src/routes/Mode/Mode.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import React, { useEffect, useState, useRef } from 'react';
import { useParams, useLocation, useNavigate } from 'react-router';
import PropTypes from 'prop-types';
// TODO: DicomMetadataStore should be injected?
import { DicomMetadataStore, ServicesManager, utils } from '@ohif/core';
import { DicomMetadataStore, ServicesManager, utils, Types, log } from '@ohif/core';
import { DragAndDropProvider, ImageViewerProvider } from '@ohif/ui';
import { useSearchParams } from '@hooks';
import { useAppConfig } from '@state';
Expand All @@ -14,6 +14,7 @@ import loadModules from '../../pluginImports';
import isSeriesFilterUsed from '../../utils/isSeriesFilterUsed';

const { getSplitParam } = utils;
const { TimingEnum } = Types;

/**
* Initialize the route.
Expand Down Expand Up @@ -60,6 +61,8 @@ function defaultRouteInit(

unsubscriptions.push(instanceAddedUnsubscribe);

log.time(TimingEnum.STUDY_TO_DISPLAY_SETS);
log.time(TimingEnum.STUDY_TO_FIRST_IMAGE);
const allRetrieves = studyInstanceUIDs.map(StudyInstanceUID =>
dataSource.retrieve.series.metadata({
StudyInstanceUID,
Expand All @@ -81,6 +84,9 @@ function defaultRouteInit(
// until we run the hanging protocol matching service.

Promise.allSettled(allRetrieves).then(() => {
log.timeEnd(TimingEnum.STUDY_TO_DISPLAY_SETS);
log.time(TimingEnum.DISPLAY_SETS_TO_FIRST_IMAGE);
log.time(TimingEnum.DISPLAY_SETS_TO_ALL_IMAGES);
const displaySets = displaySetService.getActiveDisplaySets();

if (!displaySets || !displaySets.length) {
Expand Down Expand Up @@ -443,7 +449,7 @@ export default function ModeRoute({
<ImageViewerProvider
// initialState={{ StudyInstanceUIDs: StudyInstanceUIDs }}
StudyInstanceUIDs={studyInstanceUIDs}
// reducer={reducer}
// reducer={reducer}
>
<CombinedContextProvider>
<DragAndDropProvider>
Expand Down
19 changes: 17 additions & 2 deletions platform/core/src/log.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,23 @@ const log = {
info: console.log,
trace: console.trace,
debug: console.debug,
time: console.time,
timeEnd: console.timeEnd,
time: key => {
log.timingKeys[key] = true;
console.time(key);
},
timeEnd: key => {
if (!log.timingKeys[key]) {
Copy link
Member

Choose a reason for hiding this comment

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

can we consle.warn here before retun that key does not exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is specifically avoided - we get lots more time ends than we do time starts, and we don't care that the key doesn't exist any longer.

return;
}
log.timingKeys[key] = false;
console.timeEnd(key);
},
// Store the timing keys to allow knowing whether or not to log events
timingKeys: {
// script time values are added before log is loaded, and the log
// shouldn't depend on the enums, so for this case recreate the string.
scriptToView: true,
},
wayfarer3130 marked this conversation as resolved.
Show resolved Hide resolved
};

export default log;
8 changes: 8 additions & 0 deletions platform/core/src/types/TimingEnum.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
export enum TimingEnum {
STUDY_TO_DISPLAY_SETS = 'studyToDisplaySetsLoaded',
STUDY_TO_FIRST_IMAGE = 'studyToFirstImage',
DISPLAY_SETS_TO_FIRST_IMAGE = 'displaySetsToFirstImage',
sedghi marked this conversation as resolved.
Show resolved Hide resolved
DISPLAY_SETS_TO_ALL_IMAGES = 'displaySetsToAllImages',
wayfarer3130 marked this conversation as resolved.
Show resolved Hide resolved
SCRIPT_TO_VIEW = 'scriptToView',
SEARCH_TO_LIST = 'searchToList',
wayfarer3130 marked this conversation as resolved.
Show resolved Hide resolved
}
3 changes: 3 additions & 0 deletions platform/core/src/types/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ export * from './PanelModule';
export * from './IPubSub';
export * from './Color';

// Enum exports
export * from './TimingEnum';

/**
* Export the types used within the various services and managers, but
* not the services/managers themselves, which are exported at the top level.
Expand Down
Loading