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

Set a reasonable max count on open files and don't panic if set fails #7992

Merged
merged 1 commit into from Oct 13, 2015

Conversation

@connorimes
Copy link
Contributor

connorimes commented Oct 13, 2015

Review on Reviewable

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Oct 13, 2015

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.


components/script/lib.rs, line 106 [r1] (raw file):
Why 4096? In the code from gecko that I linked to you yesterday (http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsSocketTransportService2.cpp#1426 ), Gecko sets the limit to 550+250=800.


Comments from the review on Reviewable.io

@connorimes
Copy link
Contributor Author

connorimes commented Oct 13, 2015

setrlimit was failing on an ARM system and it's unclear why. A C program that does the same thing worked, but we may want to track down why it failed. A Rust problem?

@connorimes
Copy link
Contributor Author

connorimes commented Oct 13, 2015

Yes, I saw that. I picked 4096 because of the extra parallelization in servo and didn't see any harm in requesting a higher limit (doesn't mean we'll use it). I was originally thinking 1024, but we can default to 800 if you want, as long we you think it's enough. Before we were requesting the max possible, which at least on my laptop was 65536. The ARM system I'm testing on has a max of 4096 and a default of 1024.

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Oct 13, 2015

I'm a little worried about http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsSocketTransportService2.cpp#1432, which uses 'at least', implying only 1k somewhere? Maybe @vvuk can comment - did you run into anything with setrlimit calls when doing the Windows port?

@connorimes
Copy link
Contributor Author

connorimes commented Oct 13, 2015

FYI, this perform_platform_specific_initialization fn is for linux only. For other OS targets the fn is currently empty.

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Oct 13, 2015

Hrm, yeah, let's just stick with 4096, then, and see how it works out.

@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Oct 13, 2015

📌 Commit fd710ee has been approved by larsbergstrom

@bors-servo
Copy link
Contributor

bors-servo commented Oct 13, 2015

Testing commit fd710ee with merge 7babf4a...

bors-servo pushed a commit that referenced this pull request Oct 13, 2015
…gstrom

Set a reasonable max count on open files and don't panic if set fails



<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7992)
<!-- Reviewable:end -->
@vvuk
Copy link
Contributor

vvuk commented Oct 13, 2015

Windows doesn't really have such a limit -- each process can have as many as 16M (2^24) handles open, and there isn't a soft limit that needs to be raised.

@bors-servo
Copy link
Contributor

bors-servo commented Oct 13, 2015

💔 Test failed - mac-rel-wpt

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Oct 13, 2015

Ran 213204 tests (3286 parents, 209918 subtests)
Expected results: 212504
Unexpected results: 1 (TIMEOUT: 1)
Skipped: 699

Unexpected Results
==================

/_mozilla/mozilla/canvas/drawimage_html_image_10.html
-----------------------------------------------------
@larsbergstrom
Copy link
Contributor

larsbergstrom commented Oct 13, 2015

@bors-servo retry

@bors-servo
Copy link
Contributor

bors-servo commented Oct 13, 2015

Previous build results for android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css are reusable. Rebuilding only mac-rel-wpt...

@bors-servo
Copy link
Contributor

bors-servo commented Oct 13, 2015

@bors-servo bors-servo merged commit fd710ee into servo:master Oct 13, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.