Skip to content
This repository was 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
pcwalton wants to merge 1 commit into
servo:masterfrom
pcwalton:thread-local-static-arrays
Open

Make the static arrays of style-affecting attributes thread-local.#28
pcwalton wants to merge 1 commit into
servo:masterfrom
pcwalton:thread-local-static-arrays

Conversation

@pcwalton

Copy link
Copy Markdown

... 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
Copy Markdown
Member

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

@Ms2ger

Ms2ger commented Jun 12, 2015

Copy link
Copy Markdown

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

@pcwalton

Copy link
Copy Markdown
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
Copy Markdown
Member

Can't we make atom! use a constfn?

@pcwalton

Copy link
Copy Markdown
Author

Do we have const fn in Servo's Rust?

@Manishearth

Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
Member

Oh, right. We should have it then.

@Ms2ger

Ms2ger commented Sep 2, 2015

Copy link
Copy Markdown

@pcwalton ping

@metajack

metajack commented Nov 3, 2015

Copy link
Copy Markdown
Collaborator

pinging @pcwalton again

@nox

nox commented Mar 7, 2016

Copy link
Copy Markdown

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

@SimonSapin

Copy link
Copy Markdown
Member

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

@SimonSapin

Copy link
Copy Markdown
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
Copy Markdown
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