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

Minimize unnecessary message deserialization #6946

Merged
merged 1 commit into from
Aug 2, 2021

Conversation

DonJayamanne
Copy link
Contributor

Part of #5662
Simple change, deserialization is a little slower, hence using plain string operations over deserialization.

@DonJayamanne DonJayamanne requested a review from a team as a code owner August 2, 2021 19:27
// where this message contains some data we're interested in.
let mustDeserialize = false;
if (typeof data === 'string') {
mustDeserialize = data.includes(WIDGET_MIMETYPE) || data.includes(Identifiers.DefaultCommTarget);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a pretty big assumption. How do you know that all messages for a widget will fit into these subcategories? And what about when we start supporting synapse?

What's the perf improvement of this change? Seems risky to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Misread the section below. THis is just like the previous change.

@codecov-commenter
Copy link

Codecov Report

Merging #6946 (7946ea8) into main (13cf495) will increase coverage by 0%.
The diff coverage is n/a.

@@          Coverage Diff          @@
##            main   #6946   +/-   ##
=====================================
  Coverage     65%     65%           
=====================================
  Files        410     410           
  Lines      28528   28528           
  Branches    4263    4263           
=====================================
+ Hits       18584   18628   +44     
+ Misses      8316    8272   -44     
  Partials    1628    1628           
Impacted Files Coverage Δ
...ent/datascience/jupyter/pythonVariableRequester.ts 65% <0%> (-2%) ⬇️
...client/datascience/kernel-launcher/kernelDaemon.ts 53% <0%> (-2%) ⬇️
...ient/datascience/editor-integration/codewatcher.ts 68% <0%> (-1%) ⬇️
src/client/datascience/jupyter/jupyterNotebook.ts 66% <0%> (ø)
src/client/datascience/jupyter/kernels/helpers.ts 73% <0%> (+<1%) ⬆️
...t/datascience/notebook/vscodeNotebookController.ts 80% <0%> (+<1%) ⬆️
...lient/datascience/jupyter/kernels/cellExecution.ts 71% <0%> (+<1%) ⬆️
src/client/datascience/telemetry/telemetry.ts 73% <0%> (+1%) ⬆️
src/client/datascience/baseJupyterSession.ts 66% <0%> (+1%) ⬆️
src/client/datascience/notebook/notebookEditor.ts 41% <0%> (+1%) ⬆️
... and 10 more

@DonJayamanne DonJayamanne merged commit 7314781 into main Aug 2, 2021
@DonJayamanne DonJayamanne deleted the simplePerfImprovements branch August 2, 2021 20:52
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.

3 participants