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

Bound sampler buffer, add length feedback message, fix intermittence in test #23139

Merged

Conversation

Projects
None yet
5 participants
@gterzian
Copy link
Collaborator

commented Mar 30, 2019


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #23109 (GitHub issue number if applicable)
  • There are tests for these changes OR
  • These changes do not require tests because ___

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Mar 30, 2019

Heads up! This PR modifies the following files:

  • @asajeffrey: components/constellation/constellation.rs
  • @cbrewster: components/constellation/constellation.rs
  • @paulrouget: components/constellation/constellation.rs

@gterzian gterzian requested a review from jdm Mar 30, 2019

@gterzian

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 30, 2019

@jdm I think this should fix the intermittent issue in the tests, and I hope this isn't just extra code to support the tests, since one can also imagine the extra message later becoming useful to show feedback on the sampling process in devtools and the likes, a little bit like the performance feature show below...

Screen Shot 2019-03-30 at 1 37 23 PM

@gterzian gterzian force-pushed the gterzian:bounded_profiler_with_buffer_feedback branch 3 times, most recently from 1643138 to a317e1a Mar 30, 2019

@jdm

This comment has been minimized.

Copy link
Member

commented Mar 30, 2019

Sending the constellation a message after every single sample for each thread seems excessive :) to fix the intermittency of the test, I think we should just check that >0 samples were taken and wait for 100ms.

For the bounding, a VecDeque is a good idea, but 100 is a really small number of samples. It should be configurable via the enabling message, and we should try to bound it by time, rather than number of samoles, defaulting to 10s.

@atouchet atouchet changed the title Bound sampler buffer, add lenght feedback message, fix intermittence in test Bound sampler buffer, add length feedback message, fix intermittence in test Mar 30, 2019

@gterzian gterzian force-pushed the gterzian:bounded_profiler_with_buffer_feedback branch 3 times, most recently from a2afb54 to fb8cba1 Mar 31, 2019

@gterzian

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 31, 2019

@jdm Ok, I've added an attempt to limit the sampling in time.

Re checking the number of samples in the test, it appears to me that an issue there is that we only receive the data after having stopped the sampling, so we could do another round if not enough have been taken, and it would seem to make the test too complicated(we would also have to limit the number of tries, and decide on what to do in the unlikely case that all tries resulted in not enough frames having been sampled).

I've instead increased the sleep to 100ms, which seems to reduce the chance of sampling less than 2 rounds to something close to zero(I get 8 or 9 samples taken on a few test runs locally with 100ms, as opposed to 2 or 3 with 30ms).

@gterzian gterzian force-pushed the gterzian:bounded_profiler_with_buffer_feedback branch from fb8cba1 to 0780298 Mar 31, 2019

@jdm

This comment has been minimized.

Copy link
Member

commented Mar 31, 2019

@bors-servo r+
Nuce!

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2019

📌 Commit 0780298 has been approved by jdm

@highfive highfive assigned jdm and unassigned asajeffrey Mar 31, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2019

⌛️ Testing commit 0780298 with merge 31c0261...

bors-servo added a commit that referenced this pull request Mar 31, 2019

Auto merge of #23139 - gterzian:bounded_profiler_with_buffer_feedback…
…, r=jdm

Bound sampler buffer, add length feedback message, fix intermittence in test

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #23109 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23139)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2019

@bors-servo bors-servo merged commit 0780298 into servo:master Mar 31, 2019

4 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

@gterzian gterzian deleted the gterzian:bounded_profiler_with_buffer_feedback branch Apr 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.