-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add ParallelCountBytes #983
Add ParallelCountBytes #983
Conversation
Made it and CountBytes compute an MD5, so I could check that they match (they do).
Inspired by the performance difference between the previous CL and gsutil I wrote a simple example of a multithreaded reader that keeps the bytes in order for processing. This indeed goes faster than before: 12s vs the earlier 42s. So now we match gsutil's 11.98s.
|
|
||
import com.google.common.base.Stopwatch; | ||
|
||
import javax.xml.bind.annotation.adapters.HexBinaryAdapter; |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This looks fine to me, however I believe this example cloud be easier if using a fixed size ExecutorService. Callables could be passed to it and futures could be used to consume results. |
OK, I think you'll find this version much cleaner. Its output still matches. |
6c765f8
to
966679a
Compare
Plus a little bit of cleanup.
@mziccard Is this OK for merge? |
@@ -70,7 +79,7 @@ public WorkUnit call() throws IOException { | |||
return this; | |||
} | |||
chan.position(pos); | |||
// read until buffer is full, or EOF | |||
// read until buffer it is full, or EOF |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
You know, this PR is now a perfect illustration of a problem with the current adversarial way that review is done. One person's job is to write the code and fix it, the other person's job is to make sure it doesn't go in with anything bad in it. This seems reasonable but the result is that even with perfectly reasonable and capable people we occasionally end up with a PR like this one that is delayed by 3 weeks just because of a typo and a disagreement about what belongs in this PR or another. This sort of delay could be very painful and make a PR hard to merge (luckily that doesn't seem to be the case here). If we instead had a collaborative model where the coder could indicate that they trust the reviewer to just go ahead and fix typos directly themselves, together we could get things moving a lot quicker. |
Jean - apologies - I'm trying to help clear a bit of the backlog, but it's taking longer for me to be meaningful than it should. It looks like this can still be merged. Unfortunately, GH tools aren't the best for reviewer submitted changes to a PR. (unlike internal ones) |
LGTM |
@jean-philippe-martin Please don't read this latency in merging the PR as me not appreciating your work/effort (I appreciate both in fact). This PR adds an example to a non-master branch that is (sadly) stalling for other reasons (see #770). So, even though it might be frustrating from your viewpoint, there was no actual reason to speed the merge. Otherwise, I would have cherry-picked your comments in a different PR and added the fixes myself. That said, thanks Jean-Philippe :) @jean-philippe-martin @lesv Regardless of the more general discussion, I would still love to see my comments (unnecessary "it" and closing channel in |
@mziccard no offense taken, this is really about the process and not the people. And you're absolutely right, since this branch is off-master there isn't as much change and so not as much urgency to merge. I agree 100% with you that both your comments should be addressed. Since this was merged I'll do it in a different PR. @lesv thanks, and no need to apologize! We're doing things the normal way, it's just that the normal way might perhaps be improvable. No harm done in this case anyways. |
Source-Link: googleapis/synthtool@7a220e2 Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-java:latest@sha256:6d4e3a15c62cfdcb823d60e16da7521e7c6fc00eba07c8ff12e4de9924a57d28
…1.0 (#983) [![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [com.google.cloud:libraries-bom](https://cloud.google.com/java/docs/bom) ([source](https://togithub.com/googleapis/java-cloud-bom)) | `26.0.0` -> `26.1.0` | [![age](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/26.1.0/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/26.1.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/26.1.0/compatibility-slim/26.0.0)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/26.1.0/confidence-slim/26.0.0)](https://docs.renovatebot.com/merge-confidence/) | --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Renovate will not automatically rebase this PR, because other commits have been found. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox. ⚠ **Warning**: custom changes will be lost. --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/java-dialogflow). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4xNTguMCIsInVwZGF0ZWRJblZlciI6IjMyLjE1OC4wIn0=-->
🤖 I have created a release *beep* *boop* --- ### Updating meta-information for bleeding-edge SNAPSHOT release. --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
🤖 I have created a release *beep* *boop* --- ## [3.1.2](https://togithub.com/googleapis/java-vision/compare/v3.1.1...v3.1.2) (2022-10-03) ### Documentation * Update comments for image annotator OCR models ([#983](https://togithub.com/googleapis/java-vision/issues/983)) ([5c408fe](https://togithub.com/googleapis/java-vision/commit/5c408fef37777fb9899d9190b89896318f64d262)) ### Dependencies * Update dependency com.google.cloud:google-cloud-core to v2.8.12 ([#984](https://togithub.com/googleapis/java-vision/issues/984)) ([59f683c](https://togithub.com/googleapis/java-vision/commit/59f683ce1933b80a3b6256d9cf7f737c689ba711)) * Update dependency com.google.cloud:google-cloud-core to v2.8.13 ([#992](https://togithub.com/googleapis/java-vision/issues/992)) ([e0882e8](https://togithub.com/googleapis/java-vision/commit/e0882e83f338f4fb530a0c304dfa97bc1977fc56)) * Update dependency com.google.cloud:google-cloud-core to v2.8.14 ([#993](https://togithub.com/googleapis/java-vision/issues/993)) ([efd9c3f](https://togithub.com/googleapis/java-vision/commit/efd9c3fc7863dd4d819e9628d18bb43998f73ef8)) * Update dependency com.google.cloud:google-cloud-core to v2.8.18 ([#995](https://togithub.com/googleapis/java-vision/issues/995)) ([ead37cc](https://togithub.com/googleapis/java-vision/commit/ead37cc0cf1a1af8bb513d89153e30f06ea71f20)) * Update dependency com.google.cloud:google-cloud-core to v2.8.19 ([#1023](https://togithub.com/googleapis/java-vision/issues/1023)) ([ce23a3b](https://togithub.com/googleapis/java-vision/commit/ce23a3b2ae1c49a8df71fd66eda38f4845763cad)) * Update dependency com.google.cloud:google-cloud-shared-dependencies to v3.0.4 ([#1026](https://togithub.com/googleapis/java-vision/issues/1026)) ([71a4c70](https://togithub.com/googleapis/java-vision/commit/71a4c70366ab05af1307d914975aa43a2e5679a7)) * Update dependency com.google.cloud:google-cloud-storage to v2.12.0 ([#986](https://togithub.com/googleapis/java-vision/issues/986)) ([08262f8](https://togithub.com/googleapis/java-vision/commit/08262f87c3ce67eed973ccf9fc2f1f63525f5857)) * Update spring.version to v2.7.4 ([#994](https://togithub.com/googleapis/java-vision/issues/994)) ([8688790](https://togithub.com/googleapis/java-vision/commit/868879044e4296f524d6157d37b8601a5fc8dc4d)) --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
….12.1 (#983) [![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [com.google.auth:google-auth-library-bom](https://togithub.com/googleapis/google-auth-library-java) | `1.12.0` -> `1.12.1` | [![age](https://badges.renovateapi.com/packages/maven/com.google.auth:google-auth-library-bom/1.12.1/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/com.google.auth:google-auth-library-bom/1.12.1/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/com.google.auth:google-auth-library-bom/1.12.1/compatibility-slim/1.12.0)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/com.google.auth:google-auth-library-bom/1.12.1/confidence-slim/1.12.0)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>googleapis/google-auth-library-java</summary> ### [`v1.12.1`](https://togithub.com/googleapis/google-auth-library-java/blob/HEAD/CHANGELOG.md#​1121-httpsgithubcomgoogleapisgoogle-auth-library-javacomparev1120v1121-2022-10-18) ##### Bug Fixes - Resolve race condition reported in [#​692](https://togithub.com/googleapis/google-auth-library-java/issues/692) ([#​1031](https://togithub.com/googleapis/google-auth-library-java/issues/1031)) ([87a6606](https://togithub.com/googleapis/google-auth-library-java/commit/87a66067dff49d68f5b01cfe4c3f755fbf6b44fb)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/java-core). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4yMzguNCIsInVwZGF0ZWRJblZlciI6IjMyLjI0MC41In0=-->
Made it and CountBytes compute an MD5, so I could check that they match
(they do).