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

Speed up query results copying #25866

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

Conversation

lewis-sanchez
Copy link
Contributor

@lewis-sanchez lewis-sanchez commented Aug 22, 2024

This PR will fix #25632

The other PR's needed for this PR:
sqlops-dataprotocolclient - microsoft/sqlops-dataprotocolclient#101
sqltoolsservice - microsoft/sqltoolsservice#2385

The reason copying was very slow from the SQL Tools Service was because ToString was repeatedly being called on the StringBuilder before EndsWith: https://github.com/microsoft/sqltoolsservice/blob/18ffbe6d76c511881a76f636d2e59467dfe8c4d6/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/QueryExecutionService.cs#L866

In the PR for the SQL Tools Service ToString is only called once to correct this issue. With this correction in place, copying from STS is significantly faster, as can be seen in this gif where copying is done in the SQL Tools Service process.

In this gif 100,000 query results are being copied Through the SQL Tools Service:
Copy 100000 query results STS

For comparison, this is what copying 100,000 query results from SSMS looks like:
Copy 100000 query results SSMS

Without the changes associated with this PR and the related PR's were made, it takes a very long time to copy query results. this following gif runs for over a minute and doesn't complete the copy operation for 100,000 query results.
Copy 100000 query results STS before change

Time Differences between copying from ADS UI and STS

6 Column Query Results:

SELECT TOP [25000|50000|75000|100000] sys.all_columns.name, column_id, system_type_id, user_type_id, max_length, [precision]
FROM sys.all_columns, sys.all_views;

Copying results with 6 columns from the UI:
ADS UI Copy with 25,000 rows: 813.864013671875 ms
ADS UI Copy with 50,000 rows: 1506.052001953125 ms
ADS UI Copy with 75,000 rows: 2277.085205078125 ms
ADS UI Copy with 100,000 rows: 2966.846923828125 ms

Copying results with 6 columns from STS:
STS Copy with 25,000 rows: 0.379150390625 ms
STS Copy with 50,000 rows: 0.408935546875 ms
STS Copy with 75,000 rows: 0.454833984375 ms
STS Copy with 100,000 rows: 0.570068359375 ms

53 Column Query Results:

SELECT TOP [25000|50000|75000|100000] *
FROM sys.all_columns, sys.all_views;

Copying results with 53 columns from the UI:
ADS UI Copy with 25,000 rows: 12446.373046875 ms
ADS UI Copy with 50,000 rows: 24727.3408203125 ms
ADS UI Copy with 75,000 rows: 38170.3740234375 ms
ADS UI Copy with 100,000 rows: 51372.80810546875 ms

Copying results with 53 columns from STS:
STS Copy with 25,000 rows: 0.575927734375 ms
STS Copy with 50,000 rows: 0.652099609375 ms
STS Copy with 75,000 rows: 0.81201171875 ms
STS Copy with 100,000 rows: 1.3330078125 ms

src/sql/azdata.proposed.d.ts Outdated Show resolved Hide resolved
src/sql/workbench/services/query/common/queryRunner.ts Outdated Show resolved Hide resolved
@lewis-sanchez lewis-sanchez marked this pull request as draft August 28, 2024 01:09
@lewis-sanchez lewis-sanchez changed the title Conditionally copy query results in backend if running on the same host machine. Speed up query results copying Aug 29, 2024
@lewis-sanchez lewis-sanchez marked this pull request as ready for review August 29, 2024 02:14
@caohai
Copy link
Member

caohai commented Aug 29, 2024

So this change (alongside with PRs in STS + sqlops-dataprotocolclient) will now do the two following things, right?

  1. revert the provider side copy feature to the state Alan initially implemented.
  2. set the default value for provider side copy config to false instead of true.

This means win/mac users will need to manually enable this config to get the fast copy feature. Can we enable this for win/mac automatically without breaking copy for linux?

Can we do something like this?

'default': isLinux ? false : true,

@lewis-sanchez
Copy link
Contributor Author

lewis-sanchez commented Aug 29, 2024

So this change (alongside with PRs in STS + sqlops-dataprotocolclient) will now do the two following things, right?

  1. revert the provider side copy feature to the state Alan initially implemented.
  2. set the default value for provider side copy config to false instead of true.

This means win/mac users will need to manually enable this config to get the fast copy feature. Can we enable this for win/mac automatically without breaking copy for linux?

Can we do something like this?

'default': isLinux ? false : true,

This change will

  1. Revert the provider side copy feature to the state Alan initially implemented
  2. will also set the query provider copy feature to false.

Win/Mac users don't need to change anything because copying from the UI was much faster than copying from the query provider.

@kburtram
Copy link
Member

kburtram commented Aug 29, 2024

Win/Mac users don't need to change anything because copying from the UI was much faster than copying from the query provider.

I'm confused by this comment. What is "query provider" referring to here, the query service in STS? The original bug for slow copy was reported on Windows with the current implementation (#25632). What is this PR doing to improve the copy performance on Windows?

Edit: I see the screenshots where copying from UI process is faster than copying directly from STS. Given the data is in STS and needs to be streamed to ADS UI through the extension host, how could it be faster to copy from the UI than directly from STS? 

@caohai
Copy link
Member

caohai commented Aug 29, 2024

So this change (alongside with PRs in STS + sqlops-dataprotocolclient) will now do the two following things, right?

  1. revert the provider side copy feature to the state Alan initially implemented.
  2. set the default value for provider side copy config to false instead of true.

This means win/mac users will need to manually enable this config to get the fast copy feature. Can we enable this for win/mac automatically without breaking copy for linux?
Can we do something like this?

'default': isLinux ? false : true,

This change will

  1. Revert the provider side copy feature to the state Alan initially implemented
  2. will also set the query provider copy feature to false.

Win/Mac users don't need to change anything because copying from the UI was much faster than copying from the query provider.

I was always under the impression that STS side copy is faster and not aware of the UI side copy optimization happened in #25152. Can you update the PR description to include the whole story?

@kburtram
Copy link
Member

So this change (alongside with PRs in STS + sqlops-dataprotocolclient) will now do the two following things, right?

  1. revert the provider side copy feature to the state Alan initially implemented.
  2. set the default value for provider side copy config to false instead of true.

This means win/mac users will need to manually enable this config to get the fast copy feature. Can we enable this for win/mac automatically without breaking copy for linux?
Can we do something like this?

'default': isLinux ? false : true,

This change will

  1. Revert the provider side copy feature to the state Alan initially implemented
  2. will also set the query provider copy feature to false.

Win/Mac users don't need to change anything because copying from the UI was much faster than copying from the query provider.

I was always under the impression that STS side copy is faster and not aware of the UI side copy optimization happened in #25152. Can you update the PR description to include the whole story?

It would also be interesting to see this PR compared to the current implementation. The two changes I see here for default Windows users is that the copy method is now awaited and the result string is no longer returned from STS (it's not clear that's even being called in default Windows code path). The recent bug reports are on Windows after #25152, so unclear how this change address this bug to me.

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.

copying to clipboard
3 participants