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

graph performance #2336

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

graph performance #2336

wants to merge 3 commits into from

Conversation

seeden
Copy link
Contributor

@seeden seeden commented May 7, 2024

fix line graph performance

@seeden seeden requested a review from ChiaMineJP May 7, 2024 20:50
@seeden seeden requested a review from a team as a code owner May 7, 2024 20:50
@ChiaMineJP
Copy link
Contributor

@seeden looking into. Please give me time.

Comment on lines +68 to +69
const stringifiedData = useMemo(() => JSONbig.stringify(data), [data]);
const freezedData = useMemo<Point[]>(() => JSONbig.parse(stringifiedData), [stringifiedData]);
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, why stringified then parse?
Was it for debug?
Since it seems there are no array/object manipulation, I think you can directly use data to get yData and xData without any side effects

Comment on lines +71 to +77
const yData = useMemo(() => freezedData.map((item) => item.y), [freezedData]);
const xData = useMemo(() => freezedData.map((item) => item.x), [freezedData]);

const yDataNumber = useMemo(
() => yData.map((value) => (value instanceof BigNumber ? value.toNumber() : value)),
[yData],
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a big deal.
I think summarizing the use of React.useMemo can slightly reduce the CPU/RAM consumption because the number of [deps] to remember / check diff is reduced.

So I'd suggest to remove yData lines and integrate it to code for yDataNumber

const yDataNumber = useMemo(
  () => data.map((item) => item.y instanceof BigNumber ? item.y.toNumber() : item.y),
  [data],
);

Comment on lines +82 to +88
const xAxis = useMemo(
() => ({
data: xData,
valueFormatter: xValueFormatter,
}),
[xData, xValueFormatter],
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to above.
Since It seems xData is only an intermediate variable and not be used after getting the xAxis, the code to get xData can be summarized into the code for xAxis.

const max = Math.max(min, ...data.map((item) => item.y));
const yValueFormatter = useCallback(
(value) => {
const formatedValue = isCAT ? mojoToCAT(value) : mojoToChia(value);
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: formatedValue -> formattedValue

const min = data.length ? Math.min(...data.map((item) => item.y)) : 0;
const max = Math.max(min, ...data.map((item) => item.y));
const yValueFormatter = useCallback(
(value) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

My VSCode complains a missing type.
According to the yValueFormatter definition by LineChart, this should be (value: number | BigNumber | null) => {

Copy link

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@mui/x-charts@7.3.2 environment +45 17.5 MB jcquintas

View full report↗︎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants