Use download thread to speed up result retrieval #280
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mdesmet
force-pushed
the
feature/multithread-fetch
branch
2 times, most recently
from
3a5e182 to
9b3fb71
Compare
mdesmet
force-pushed
the
feature/multithread-fetch
branch
from
9b3fb71 to
15de687
Compare
|
Performance tests: TPC-current prod TPC - Download thread |
nineinchnick
reviewed
Jan 8, 2023
| @@ -684,6 +686,27 @@ def _verify_extra_credential(self, header): | |||
| raise ValueError(f"only ASCII characters are allowed in extra credential '{key}'") | |||
|
|
|||
|
|
|||
| class ResultDownloader(): | |||
| def __init__(self): | |||
| self.queue: queue.Queue = queue.Queue() | |||
There was a problem hiding this comment.
According to the docs, the default maxsize is zero, which means unbounded. When I was testing similar changes in the Go driver, it turned out that the queue size didn't matter, so even a queue of size 1 is fine. Did you test different queue sizes, and if not, can you check if maxsize greater than 1 makes a notable difference?
|
There is a lot of noise in the benchmark results. But ignoring that the results seem like a mixed bag, there is noticeable improvement for some data types/queries with legacy type mapping but the same types also show regressions as soon as type mapping is introduced - which seems a bit unintuitive. Some regressions which we should look at, I think the ones with type mapping actually show that type mapping on main thread actually introduces some contention/waiting? The ones without type mapping regressing are the more interesting results.
As a side note there's no point to using the TPCH connector to benchmark, it's often bottlenecked by data generation. Using the memory connector is the correct way. |
|
Oh, one more thing - are you testing with the server running locally or somewhere remote? Because the local network is so fast that you probably won't be able to measure any benefits of faster downloads. |
|
Python's Queue is deeply flawed and queue raising Empty is unreliable, source: I've done tons of multi-threaded programming in Python. Also, the semaphore that the Queue class uses internally is slow. Queue is designed to protect the programmer from themselves when unknown objects are being synchronized. Because of the Global Interpreter Lock, most methods of a list are thread safe. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Use a separate thread for downloading and do parsing in the main thread.
Non-technical explanation
Release notes
( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(x) Release notes are required, with the following suggested text:
* Improved result retrieval performance by using a download thread to allow for simultaneous mapping and downloading of results