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

De-reference the constructor from the rendering of VariableExplorerButtonCellFormatter #10310

Closed
sadasant opened this issue Jun 3, 2022 · 4 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug debt Code quality issues papercut 🩸 Something affecting the productivity of the team perf Performance issues variable-explorer verified Verification succeeded
Milestone

Comments

@sadasant
Copy link
Contributor

sadasant commented Jun 3, 2022

The VariableExplorerButtonCellFormatter is currently tying the constructor of the VariableExplorer in: https://github.com/microsoft/vscode-jupyter/blob/main/src/webviews/webview-side/interactive-common/variableExplorer.tsx#L125

We should not take the reference from the constructor, for performance reasons.
(though I haven't tested this kind of performance in a while, but seems reasonable to move this state-referencing out of the constructor)

@sadasant sadasant added bug Issue identified by VS Code Team member as probable bug papercut 🩸 Something affecting the productivity of the team variable-explorer perf Performance issues labels Jun 3, 2022
@sadasant sadasant added this to the June 2022 milestone Jun 3, 2022
@sadasant sadasant self-assigned this Jun 3, 2022
@github-actions github-actions bot added the triage-needed Issue needs to be triaged label Jun 3, 2022
@greazer greazer added debt Code quality issues and removed triage-needed Issue needs to be triaged labels Jun 6, 2022
sadasant added a commit to sadasant/vscode-jupyter that referenced this issue Jun 16, 2022
sadasant added a commit to sadasant/vscode-jupyter that referenced this issue Jun 23, 2022
@rchiodo rchiodo modified the milestones: June 2022, July 2022 Jul 1, 2022
@DonJayamanne
Copy link
Contributor

DonJayamanne commented Jul 5, 2022

To my knowledge this kind of this only impacts perf in react only when dealing with components that get rendered 100s if not 1000s of times e.g. within a cell of a grid.
The variable explorer here is a top level component, hence i don't think this will introduce any perf issues,

e reference from the constructor, for performance reasons.(though I haven't tested this kind of performance in a while,

personally i'd like to see the perf impact on this, as I don't think there is any.
Meaning users will not be impacted in any way, thus making this more of a debt than a perf or even a paper cut issue (because perf or papercut would imply its proven to be a problem, but something we can live with)

@sadasant
Copy link
Contributor Author

sadasant commented Jul 5, 2022

@DonJayamanne You may be correct! I made this issue to keep the open question and come back to it later.

However, since our goal is to re-enable the data viewer on the web asap, this issue will be fixed soon when we remove the isWeb clause on that component. So, we can forget about this!

Thank you for taking the time to think it through though.

@DonJayamanne
Copy link
Contributor

Should we then close this issue?

@sadasant
Copy link
Contributor Author

sadasant commented Jul 5, 2022

Yup yup! Thanks!

@sadasant sadasant closed this as completed Jul 5, 2022
@connor4312 connor4312 added the verified Verification succeeded label Jul 27, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug debt Code quality issues papercut 🩸 Something affecting the productivity of the team perf Performance issues variable-explorer verified Verification succeeded
Projects
None yet
5 participants