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

Add linux sampler #22355

Merged
merged 3 commits into from Mar 30, 2019

Conversation

Projects
None yet
8 participants
@gterzian
Copy link
Collaborator

commented Dec 3, 2018


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

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Dec 3, 2018

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

@gterzian gterzian force-pushed the gterzian:add_linux_sampler branch 4 times, most recently from 99e576d to ff2ea42 Dec 3, 2018

@gterzian

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 7, 2018

@nikhilm Do you perhaps remember how you made unwind_sys work? Currently it's complaining about a non-existing libunwind-generic on Ubuntu(in the build script). I've tried installing libunwind-setjmp0, with no luck...

@nikhilm

This comment has been minimized.

Copy link

commented Dec 7, 2018

I have a default chromeos container, which is Debian, and I have these packages

ii  libunwind-dev                     1.1-4.1                           amd64        library to determine the call-chain of a program - development
ii  libunwind8                        1.1-4.1                           amd64        library to determine the call-chain of a program - runtime
ii  libunwind8-dbg                    1.1-4.1                           amd64        library to determine the call-chain of a program - runtime
ii  libunwind8-dev                    1.1-4.1                           amd64        library to determine the call-chain of a program - development

Perhaps you are just missing the -dev package.

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2019

☔️ The latest upstream changes (presumably #22692) made this pull request unmergeable. Please resolve the merge conflicts.

@jdm jdm force-pushed the gterzian:add_linux_sampler branch from ff2ea42 to 67b5f14 Mar 22, 2019

@jdm jdm removed the S-needs-rebase label Mar 22, 2019

@jdm jdm assigned jdm and unassigned ferjm Mar 22, 2019

@jdm

This comment has been minimized.

Copy link
Member

commented Mar 23, 2019

running 2 tests
test test_hang_monitoring_unregister ... ok
error: process didn't exit successfully: `/home/travis/build/servo/servo/target/debug/deps/hang_monitor_tests-73c7f05bfe8ba326` (signal: 27)
The command "./mach test-unit" exited with 101.
@gterzian

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 25, 2019

Given that it's test_hang_monitoring_unregister that errors(is it?), and signal 27 indicates it fails in relations to the SIGPROF signal, I guess the error happens in the Drop implementation of LinuxSampler?

Thanks for working on this one, I'm not really setup for running Linux(even on a VM of some sort) at the moment and I let this one slide once I ran into some problems on CI :)

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2019

☔️ The latest upstream changes (presumably #22944) made this pull request unmergeable. Please resolve the merge conflicts.

@jdm

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

It's test_hang_monitoring that has a problem, and there's output that looks like Profiling timer expired which I can't figure out where it's coming from.

@jdm

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

I suspect that since we run tests in parallel in different threads the signals are triggering unexpected behaviour in tests. I'll try serializing them.

@jdm jdm force-pushed the gterzian:add_linux_sampler branch from 6c34b1e to 8715910 Mar 25, 2019

@jdm

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

Requires servo/saltfs#948 to be deployed before we can merge.

@jdm jdm force-pushed the gterzian:add_linux_sampler branch from 8715910 to 7898ca8 Mar 25, 2019

@jdm jdm changed the title [WIP] Add linux sampler Add linux sampler Mar 25, 2019

@jdm jdm removed the S-needs-rebase label Mar 25, 2019

@jdm jdm changed the title Add linux sampler [WIP] Add linux sampler Mar 25, 2019

@jdm jdm changed the title [WIP] Add linux sampler Add linux sampler Mar 25, 2019

@jdm

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

I was able to generate https://perfht.ml/2UV6uBB with the latest changes.

@jdm jdm force-pushed the gterzian:add_linux_sampler branch from 36ca594 to 53e20d7 Mar 25, 2019

@jdm

This comment has been minimized.

Copy link
Member

commented Mar 30, 2019

@bors-servo r=gterzian,asajeffrey

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2019

📌 Commit b269f87 has been approved by gterzian,asajeffrey

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2019

⌛️ Testing commit b269f87 with merge 3a9135a...

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

Auto merge of #22355 - gterzian:add_linux_sampler, r=gterzian,asajeffrey
Add linux sampler

<!-- 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: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #22203 (github issue number if applicable).

<!-- Either: -->
- [x] There are tests for these changes
- [ ] 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/22355)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2019

💔 Test failed - magicleap

@jdm jdm force-pushed the gterzian:add_linux_sampler branch from b269f87 to 57c3b3d Mar 30, 2019

@jdm jdm force-pushed the gterzian:add_linux_sampler branch from 57c3b3d to 6c1bf6a Mar 30, 2019

@jdm

This comment has been minimized.

Copy link
Member

commented Mar 30, 2019

@bors-servo r=asajeffrey,gterzian

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2019

📌 Commit 6c1bf6a has been approved by asajeffrey,gterzian

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2019

⌛️ Testing commit 6c1bf6a with merge 41feb83...

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

Auto merge of #22355 - gterzian:add_linux_sampler, r=asajeffrey,gterzian
Add linux sampler

<!-- 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: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #22203 (github issue number if applicable).

<!-- Either: -->
- [x] There are tests for these changes
- [ ] 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/22355)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2019

☀️ Test successful - android-mac, arm32, arm64, linux-rel-css, linux-rel-wpt, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, magicleap, status-taskcluster
Approved by: asajeffrey,gterzian
Pushing 41feb83 to master...

@bors-servo bors-servo merged commit 6c1bf6a into servo:master Mar 30, 2019

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

@gterzian gterzian deleted the gterzian:add_linux_sampler branch Mar 30, 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.