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

Stop relying on linking details of std’s default allocator #18944

Merged
merged 1 commit into from Oct 19, 2017

Conversation

@SimonSapin
Copy link
Member

SimonSapin commented Oct 18, 2017

We’ve been bitten before by symbol names changing: servo/heapsize#46, and upstream is planning to stop using jemalloc by default: rust-lang/rust#33082 (comment)

So use the (relatively) new #[global_allocator] attribute to explicitly select the system allocator on Windows and jemalloc (now in an external crate) on other platforms. This choice matches current defaults.


This change is Reviewable

@jdm
Copy link
Member

jdm commented Oct 19, 2017

From appveyor:

    Running `rustc --crate-name servo_allocator C:\projects\servo\components\allocator\lib.rs --crate-type lib --emit=dep-info,link -C codegen-units=4 -C debuginfo=2 --cfg "feature=\"jemallocator\"" --cfg "feature=\"kernel32-sys\"" --cfg "feature=\"unstable\"" -C metadata=1d3d51af39724855 -C extra-filename=-1d3d51af39724855 --out-dir C:\projects\servo\target\debug\deps -L dependency=C:\projects\servo\target\debug\deps --extern kernel32=C:\projects\servo\target\debug\deps\libkernel32-b218c0c26de4e27c.rlib -W unused-extern-crates`
error: use of unstable library feature 'allocator_api' (see issue #32838)
  --> C:\projects\servo\components\allocator\lib.rs:11:15
   |
11 | static ALLOC: platform::Allocator = platform::Allocator;
   |               ^^^^^^^^^^^^^^^^^^^
   |
   = help: add #![feature(allocator_api)] to the crate attributes to enable
error: use of unstable library feature 'allocator_api' (see issue #32838)
  --> C:\projects\servo\components\allocator\lib.rs:11:37
   |
11 | static ALLOC: platform::Allocator = platform::Allocator;
   |                                     ^^^^^^^^^^^^^^^^^^^
   |
   = help: add #![feature(allocator_api)] to the crate attributes to enable
error: use of unstable library feature 'alloc_system': this library is unlikely to be stabilized in its current form or name (see issue #27783)
  --> C:\projects\servo\components\allocator\lib.rs:31:5
   |
31 |     extern crate alloc_system;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: add #![feature(alloc_system)] to the crate attributes to enable
error: use of unstable library feature 'allocator_api' (see issue #32838)
  --> C:\projects\servo\components\allocator\lib.rs:34:13
   |
34 |     pub use self::alloc_system::System as Allocator;
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: add #![feature(allocator_api)] to the crate attributes to enable
error: aborting due to 4 previous errors
error: Could not compile `servo_allocator`.

We’ve been bitten before by symbol names changing:
servo/heapsize#46
and upstream is planning to stop using jemalloc by default:
rust-lang/rust#33082 (comment)

So use the (relatively) new `#[global_allocator]` attribute
to explicitly select the system allocator on Windows
and jemalloc (now in an external crate) on other platforms.
This choice matches current defaults.
@SimonSapin SimonSapin force-pushed the jemallocator2 branch from 6c8ed61 to 959ce48 Oct 19, 2017
@SimonSapin
Copy link
Member Author

SimonSapin commented Oct 19, 2017

Thanks Josh. Fixed.

@nox
Copy link
Member

nox commented Oct 19, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Oct 19, 2017

📌 Commit 959ce48 has been approved by nox

@bors-servo
Copy link
Contributor

bors-servo commented Oct 19, 2017

Testing commit 959ce48 with merge e58a9f1...

bors-servo added a commit that referenced this pull request Oct 19, 2017
Stop relying on linking details of std’s default allocator

We’ve been bitten before by symbol names changing: servo/heapsize#46, and upstream is planning to stop using jemalloc by default: rust-lang/rust#33082 (comment)

So use the (relatively) new `#[global_allocator]` attribute to explicitly select the system allocator on Windows and jemalloc (now in an external crate) on other platforms. This choice matches current defaults.

<!-- 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/18944)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 19, 2017

💔 Test failed - linux-rel-css

@jdm
Copy link
Member

jdm commented Oct 19, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Oct 19, 2017

Testing commit 959ce48 with merge 07e9794...

bors-servo added a commit that referenced this pull request Oct 19, 2017
Stop relying on linking details of std’s default allocator

We’ve been bitten before by symbol names changing: servo/heapsize#46, and upstream is planning to stop using jemalloc by default: rust-lang/rust#33082 (comment)

So use the (relatively) new `#[global_allocator]` attribute to explicitly select the system allocator on Windows and jemalloc (now in an external crate) on other platforms. This choice matches current defaults.

<!-- 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/18944)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 19, 2017

@bors-servo bors-servo merged commit 959ce48 into master Oct 19, 2017
4 checks passed
4 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
dependency-ci Dependencies checked
Details
homu Test successful
Details
@emilio emilio deleted the jemallocator2 branch Oct 19, 2017
@glennw
Copy link
Member

glennw commented Oct 24, 2017

@jdm Were #18950 and #18949 opened due to crashes that started happening with this PR? I think they may be genuine crashes caused by this PR...

@jdm
Copy link
Member

jdm commented Oct 25, 2017

@glennw Probably? That's good to know!

@SimonSapin
Copy link
Member Author

SimonSapin commented Oct 26, 2017

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.