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.

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

Heap size UI #718

Merged
merged 13 commits into from
Aug 26, 2022
Merged

Heap size UI #718

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
45be3d7
added heap size ui
Zhou-Ziheng Aug 12, 2022
eda96ae
added default value for bgMonitoring
Zhou-Ziheng Aug 12, 2022
4946213
improved styling
Zhou-Ziheng Aug 15, 2022
4a759dc
fixed usage bar overflow issue
Zhou-Ziheng Aug 15, 2022
7d214ea
removed used recharts package dependency
Zhou-Ziheng Aug 15, 2022
b0c7344
code style changes
Zhou-Ziheng Aug 17, 2022
38d1c93
fixed scss imports
Zhou-Ziheng Aug 17, 2022
ac6f7a4
Merge remote-tracking branch 'origin/main' into heap-size-ui
Zhou-Ziheng Aug 19, 2022
47d2717
merge build error
Zhou-Ziheng Aug 19, 2022
d755970
Minor cleanup
dsmmcken Aug 19, 2022
be6a953
Merge remote-tracking branch 'origin/main' into pr/SparkyFnay/718-1
dsmmcken Aug 22, 2022
8c61c9b
resolve a ts build issue
dsmmcken Aug 22, 2022
ec7d211
remove eslint disable, doesn't seem to be needed
dsmmcken Aug 25, 2022
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
2 changes: 2 additions & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions packages/chart/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@ export { default as ChartUtils } from './ChartUtils';
export { default as FigureChartModel } from './FigureChartModel';
export { default as MockChartModel } from './MockChartModel';
export { default as Plot } from './plotly/Plot';
export { default as ChartTheme } from './ChartTheme';
1 change: 1 addition & 0 deletions packages/console/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
"@deephaven/log": "file:../log",
"@deephaven/storage": "file:../storage",
"@deephaven/utils": "file:../utils",
"@deephaven/chart": "file:../chart",
"@fortawesome/react-fontawesome": "^0.1.18",
"classnames": "^2.3.1",
"lodash.debounce": "^4.0.8",
Expand Down
59 changes: 59 additions & 0 deletions packages/console/src/console-history/SizeUsageUI.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
@import '../../../components/scss/custom.scss';

.max-memory {
position: relative;
background-color: $gray-900;
width: 4rem;
dsmmcken marked this conversation as resolved.
Show resolved Hide resolved
vertical-align: middle;
text-align: center;
border: $black;
z-index: 1;
Zhou-Ziheng marked this conversation as resolved.
Show resolved Hide resolved
user-select: none;
}

.total-memory,
.used-memory {
position: absolute;
inset: 0;
margin: 1px;
z-index: -1;
}

.total-memory {
background-color: $gray-700;
}

.used-memory {
background-color: $gray-500;
}

.heap-overflow {
background-color: $red;
}

.heap-tooltip {
padding: $spacer-1;
width: 200px;
}

.heap-usage-info-row {
display: flex;
color: white;
padding: $spacer-1;
justify-content: space-between;
}

.heap-plot {
display: flex;
align-items: center;
}

.heading-bottom-border {
border-bottom: 1px solid $gray-900;
margin-bottom: 0.5rem;
}

.heap-utilisation-text {
margin-top: $spacer-1;
color: $gray-400;
}
218 changes: 218 additions & 0 deletions packages/console/src/console-history/SizeUsageUI.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,218 @@
import { Tooltip } from '@deephaven/components';
import { QueryConnectable, WorkerHeapInfo } from '@deephaven/jsapi-shim';
import React, { useEffect, useState, ReactElement, useRef } from 'react';
import { Plot, ChartTheme } from '@deephaven/chart';
import './SizeUsageUI.scss';
import classNames from 'classnames';

interface SIzeUsageUIProps {
Zhou-Ziheng marked this conversation as resolved.
Show resolved Hide resolved
connection: QueryConnectable;
UIParams: { defaultUpdateInterval: number; hoverUpdateInterval: number };
Zhou-Ziheng marked this conversation as resolved.
Show resolved Hide resolved
bgMonitoring?: boolean;

// in seconds
monitorDuration: number;
Zhou-Ziheng marked this conversation as resolved.
Show resolved Hide resolved
}

const SizeUsageUI = ({
Zhou-Ziheng marked this conversation as resolved.
Show resolved Hide resolved
connection,
UIParams,
bgMonitoring = true,
monitorDuration,
}: SIzeUsageUIProps): ReactElement => {
const [freeMemory, setFreeMemory] = useState<number>(0);
const [maxHeapSize, setMaxHeapSize] = useState<number>(999);
const [totalHeapSize, setTotalHeapSize] = useState<number>(0);
Zhou-Ziheng marked this conversation as resolved.
Show resolved Hide resolved
const [hover, setHover] = useState(false);

const historyUsage = useRef<{ time: number[]; usage: number[] }>({
time: [],
usage: [],
});

const { defaultUpdateInterval, hoverUpdateInterval } = UIParams;

const setUsage = (usage: WorkerHeapInfo) => {
setFreeMemory(usage.freeMemory);
setMaxHeapSize(usage.maximumHeapSize);
setTotalHeapSize(usage.totalHeapSize);
};
Zhou-Ziheng marked this conversation as resolved.
Show resolved Hide resolved

useEffect(
function setUsageUpdateInterval() {
const fetchAndUpdate = async () => {
const newUsage = await connection.getWorkerHeapInfo();

if (bgMonitoring || hover) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since you're clearing the info if this condition is false, you should check the condition before making the network request. No need to make the request if you know the data will be discarded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The usage is not discarded though, it's passed as a attribute to the setState call later. I moved the setState call to the top to be more clear.

const usage =
(newUsage.totalHeapSize - newUsage.freeMemory) /
newUsage.maximumHeapSize;
const time = Date.now();

const past = historyUsage.current;
while (
past.time.length !== 0 &&
time - past.time[0] > monitorDuration * 1000 * 1.5
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the * 1.5 for here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just a buffer in the historyUsage array so that the graph produced isn't cut short at the beginning

) {
past.time.shift();
past.usage.shift();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

It probably won't make much of a difference here, but it would possibly be more efficient to do something like this. I'm assuming in most cases you would be shifting just 1 element off at a time, so it's probably negligible

slice is slower than shift, but where this would shine is if you were often removing many elements at once. Even if shift is a faster operation, you could be doing 50 shifts instead of just 1 slice which would make the 1 slower slice faster overall

const discardIndex = past.time.findIndex(t => t >= earliestTimeToKeep);
if (discardIndex >= 0) {
  past.time = past.time.slice(discardIndex);
  past.usage = past.usage.slice(discardIndex);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, so the most shifting we'll do is 3 at a time, that happens when the user 'unhovers' over the usage info.


past.time.push(time);
past.usage.push(usage);

historyUsage.current = past;
Zhou-Ziheng marked this conversation as resolved.
Show resolved Hide resolved
} else {
historyUsage.current = { time: [], usage: [] };
}

setUsage(newUsage);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this still happen if background monitoring is disabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the background monitoring is only for the graph.

};
fetchAndUpdate();

const updateUsage = setInterval(
fetchAndUpdate,
hover ? hoverUpdateInterval : defaultUpdateInterval
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like there's a case where you don't want to set the interval if there's no background monitoring

Copy link
Contributor Author

Choose a reason for hiding this comment

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

background monitoring is only for the graph, i.e., the historyUsage array. The memory usage data still needs to be updated at regular intervals regardless of bgmonitoring

return () => {
clearInterval(updateUsage);
};
},
[
hover,
hoverUpdateInterval,
connection,
defaultUpdateInterval,
monitorDuration,
bgMonitoring,
]
);

const toDecimalPlace = (num: number, dec: number) =>
Math.round(num * 10 ** dec) / 10 ** dec;

const decimalPlace = 2;
const GbToByte = 1024 ** 3;

const freeMemoryGB = toDecimalPlace(freeMemory / GbToByte, decimalPlace);
const totalHeapGB = toDecimalPlace(totalHeapSize / GbToByte, decimalPlace);
const maxHeapGB = toDecimalPlace(maxHeapSize / GbToByte, decimalPlace);
const inUseGB = totalHeapGB - freeMemoryGB;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are you trimming to 2 decimal places here and then using toFixed later? Is it because the toFixed doesn't round properly?

You could instead use a number formatter which is built in now. See MDN and stack overflow example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the toDecimalPlace() rounds the values to two decimal places, but it doesn't guarantee that the result has 2 decimal places when printed. For example, toDecimalPlace(1.9999, 2) => 2. Which is why I need the toFixed() to force the values to have two decimal places, filling in with 0s if needed. I need this two step process because I want to guarantee inUseGB + freeMemoryGB = totalHeapGB. Previously, the value may be off by 0.01 GB.


const getRow = (text: string, size: string, bottomBorder = false) => (
<div
className={classNames(`heap-usage-info-row`, {
'heading-bottom-border': bottomBorder,
})}
>
<div>
<h6
style={{
fontWeight: 'bold',
}}
>
Copy link
Collaborator

Choose a reason for hiding this comment

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

@dsmmcken Do we have Sass stuff for the font weight? Should we be using fontWeight: 'bold' or a numeric value instead?

{text}
</h6>
</div>
<div style={{}}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the empty style?

<h6>{size}</h6>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if this should be a h6 tag as well. While our app isn't the best from an accessibility standpoint (and we don't enforce accessibility things), it is not great to put the same tag for different levels of importance in the dom. Visually it's fine, but semantically it's probably wrong.

Is the styling of h6 even different from a normal div or span?

</div>
</div>
);

const timestamps = historyUsage.current.time;
const usages = historyUsage.current.usage;
Zhou-Ziheng marked this conversation as resolved.
Show resolved Hide resolved
const lastTimestamp = timestamps[timestamps.length - 1] ?? 0;

const totalPercentage = totalHeapSize / maxHeapSize;
const usedPercentage = (totalHeapSize - freeMemory) / maxHeapSize;
return (
<>
<div
className="max-memory"
onMouseEnter={() => setHover(true)}
onMouseLeave={() => setHover(false)}
>
<div
className="total-memory"
style={{
width: `calc(${totalPercentage * 100}% - ${totalPercentage * 2}px`,
}}
/>
<div
className={classNames('used-memory', {
'heap-overflow': (totalHeapSize - freeMemory) / maxHeapSize > 0.95,
})}
style={{
width: `calc(${usedPercentage * 100}% - ${usedPercentage * 2}px`,
}}
/>

<div className="memory-text">{maxHeapGB.toFixed(1)} GB</div>
</div>

<Tooltip>
<div className="heap-tooltip">
{getRow(
'In use:',
`${inUseGB.toFixed(decimalPlace)} of ${maxHeapGB.toFixed(
decimalPlace
)} GB`,
true
)}
{getRow('Free:', `${freeMemoryGB.toFixed(decimalPlace)} GB`)}
{getRow('Total:', `${totalHeapGB.toFixed(decimalPlace)} GB`)}
{getRow('Max:', `${maxHeapGB.toFixed(decimalPlace)} GB`)}
<div className="heap-plot">
<Plot
Zhou-Ziheng marked this conversation as resolved.
Show resolved Hide resolved
data={[
{
x: [...timestamps],
y: [...usages],
type: 'scatter',
mode: 'lines',
},
]}
config={{ staticPlot: true, responsive: true }}
style={{
width: '196px',
height: '100px',
}}
layout={{
legend: false,
margin: { l: 2, t: 2, r: 2, b: 2 },
plot_bgcolor: 'transparent',
paper_bgcolor: 'transparent',
colorway: ['#4878ea'],
xaxis: {
dtick: Math.round((monitorDuration * 1000) / 6),
gridcolor: ChartTheme.linecolor,
range: [
lastTimestamp - monitorDuration * 1000,
lastTimestamp,
],
linecolor: ChartTheme.linecolor,
linewidth: 2,
mirror: true,
},
yaxis: {
dtick: 0.2,
gridcolor: ChartTheme.linecolor,
range: [0, 1],
linecolor: ChartTheme.linecolor,
linewidth: 2,
mirror: true,
},
}}
/>
</div>
<div className="heap-utilisation-text">
<h6>% utilization over {Math.round(monitorDuration / 60)} min.</h6>
</div>
</div>
</Tooltip>
</>
);
};

export default SizeUsageUI;
1 change: 1 addition & 0 deletions packages/console/src/console-history/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@ export { default as ConsoleHistoryItem } from './ConsoleHistoryItem';
export { default as ConsoleHistoryItemResult } from './ConsoleHistoryItemResult';
export { default as ConsoleHistoryResultErrorMessage } from './ConsoleHistoryResultErrorMessage';
export { default as ConsoleHistoryResultInProgress } from './ConsoleHistoryResultInProgress';
export { default as SizeUsageUI } from './SizeUsageUI';
Zhou-Ziheng marked this conversation as resolved.
Show resolved Hide resolved
3 changes: 3 additions & 0 deletions packages/console/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@
},
{
"path": "../utils"
},
{
"path": "../chart"
}
]
}
17 changes: 15 additions & 2 deletions packages/dashboard-core-plugins/src/panels/ConsolePanel.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import PropTypes from 'prop-types';
import shortid from 'shortid';
import debounce from 'lodash.debounce';
import { connect } from 'react-redux';
import { Console, ConsoleConstants } from '@deephaven/console';
import { Console, ConsoleConstants, SizeUsageUI } from '@deephaven/console';
import { GLPropTypes, PanelEvent } from '@deephaven/dashboard';
import { PropTypes as APIPropTypes } from '@deephaven/jsapi-shim';
import Log from '@deephaven/log';
Expand Down Expand Up @@ -272,7 +272,8 @@ export class ConsolePanel extends PureComponent {
unzip,
} = this.props;
const { consoleSettings, error, objectMap } = this.state;
const { config, session } = sessionWrapper;
// eslint-disable-next-line react/prop-types
dsmmcken marked this conversation as resolved.
Show resolved Hide resolved
const { config, session, connection } = sessionWrapper;
const { id: sessionId, type: language } = config;

return (
Expand Down Expand Up @@ -302,6 +303,18 @@ export class ConsolePanel extends PureComponent {
<>
<div>&nbsp;</div>
<div>{ConsoleConstants.LANGUAGE_MAP.get(language)}</div>
<div>&nbsp;</div>
<div>
<SizeUsageUI
connection={connection}
UIParams={{
defaultUpdateInterval: 10000,
hoverUpdateInterval: 3000,
}}
monitorDuration={60 * 10}
Copy link
Contributor

Choose a reason for hiding this comment

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

Now I wonder if 10 minutes is too long, graph moves so slow. Maybe 5?

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Windows task manager shows performance over 60s, so maybe we move to 5 minutes and double the poll rate?

Zhou-Ziheng marked this conversation as resolved.
Show resolved Hide resolved
/>
</div>
<div>&nbsp;</div>
</>
}
scope={sessionId}
Expand Down
10 changes: 10 additions & 0 deletions packages/jsapi-shim/src/dh.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -865,7 +865,17 @@ export interface TableMap extends Evented {
getTable(key: object): Promise<Table>;
}

export interface WorkerHeapInfo {
readonly maximumHeapSize: number;
readonly freeMemory: number;
readonly totalHeapSize: number;
getMaximumHeapSize: () => number;
getFreeMemory: () => number;
getTotalHeapSize: () => number;
Zhou-Ziheng marked this conversation as resolved.
Show resolved Hide resolved
}

export interface QueryConnectable extends Evented {
getWorkerHeapInfo(): Promise<WorkerHeapInfo>;
getConsoleTypes(): Promise<string[]>;
startSession(type: string): Promise<IdeSession>;
}
Expand Down