Skip to content
This repository has been archived by the owner on May 28, 2024. It is now read-only.

Make the static arrays of style-affecting attributes thread-local. #28

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pcwalton
Copy link

... instead of regenerating and dropping them on the stack over and over.

Reduces I-cache footprint and memory barrier traffic on ARM.

r? @SimonSapin

Review on Reviewable

…tead of

regenerating and dropping them on the stack over and over.

Reduces I-cache footprint and memory barrier traffic on ARM.
@SimonSapin
Copy link
Member

Could they be completely static, with https://crates.io/crates/lazy_static , rather than thread-local?

@Ms2ger
Copy link

Ms2ger commented Jun 12, 2015

I've never seen any proof that this code is actually necessary.

@pcwalton
Copy link
Author

@SimonSapin I looked into that, but lazy_static causes a full memory barrier (dmb ish) on ARM each time the static is accessed, which is too expensive.

@Manishearth
Copy link
Member

Can't we make atom! use a constfn?

@pcwalton
Copy link
Author

Do we have const fn in Servo's Rust?

@Manishearth
Copy link
Member

Not yet. I can rustup though. There's some major plugins renaming but not much more as far as I can tell.

(Having done the feature audit I sort of can gauge rustup difficulty now :) )

@michaelwu
Copy link

I thought we got it in the last rustup. At the very least, the last rustup had support for const functions in libsyntax asts.

@Manishearth
Copy link
Member

Oh, right. We should have it then.

@Ms2ger
Copy link

Ms2ger commented Sep 2, 2015

@pcwalton ping

@metajack
Copy link

metajack commented Nov 3, 2015

pinging @pcwalton again

@nox
Copy link

nox commented Mar 7, 2016

atom!() is const now, so we can just use static.

@SimonSapin
Copy link
Member

Hasn’t atom!() always been const, expanding to an struct literal with an integer literal ?

@SimonSapin
Copy link
Member

Ah, here is why they’re not const:

src/matching.rs:539:15: 539:30 error: statics are not allowed to have destructors [E0493]
src/matching.rs:539         atom: atom!("hidden"),
                                  ^~~~~~~~~~~~~~~
src/matching.rs:539:15: 539:30 note: in this expansion of atom! (defined in <string_cache macros>)

… which is not gonna change as long as we use reference counting.

@SimonSapin
Copy link
Member

… unless the language-level restriction is lifted: rust-lang/rfcs#1111

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants