From 31837a1efa276a735ef9d0371af762dcc4d0a540 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Wed, 27 Nov 2019 23:24:22 +0100 Subject: [PATCH] style: Make Rust static atoms able to be used in const contexts. I see atom dropping code generated in release builds for stuff like dropping the "class" atom here: https://searchfox.org/mozilla-central/rev/4df8821c1b824db5f40f381f48432f219d99ae36/servo/components/style/gecko/wrapper.rs#592 That is silly, and I hope making Atom be able to be used in const context will help the compiler see that yeah, we're not doing anything interesting and the atom shouldn't get dropped. It also allows us to get rid of a few lazy_static!s, so we should do it anyway. In order to accomplish this, compute the offset into gGkAtoms manually instead of going through the static_atoms() array and then back to the byte offset. Differential Revision: https://phabricator.services.mozilla.com/D55039 --- components/style/gecko/regen_atoms.py | 2 +- components/style/gecko_string_cache/mod.rs | 32 ++++++------ components/style/style_adjuster.rs | 57 ++++++++++------------ 3 files changed, 45 insertions(+), 46 deletions(-) diff --git a/components/style/gecko/regen_atoms.py b/components/style/gecko/regen_atoms.py index 0066d06d054..cf7cc77c16f 100755 --- a/components/style/gecko/regen_atoms.py +++ b/components/style/gecko/regen_atoms.py @@ -132,7 +132,7 @@ PRELUDE = ''' RULE_TEMPLATE = ''' ("{atom}") => {{{{ #[allow(unsafe_code)] #[allow(unused_unsafe)] - unsafe {{ $crate::string_cache::Atom::from_index({index}) }} + unsafe {{ $crate::string_cache::Atom::from_index_unchecked({index}) }} }}}}; '''[1:] diff --git a/components/style/gecko_string_cache/mod.rs b/components/style/gecko_string_cache/mod.rs index 5d849a96f6e..e1da47e937e 100644 --- a/components/style/gecko_string_cache/mod.rs +++ b/components/style/gecko_string_cache/mod.rs @@ -49,10 +49,15 @@ macro_rules! local_name { }; } -/// A handle to a Gecko atom. +/// A handle to a Gecko atom. This is a type that can represent either: +/// +/// * A strong reference to a dynamic atom (an `nsAtom` pointer), in which case +/// the `usize` just holds the pointer value. +/// +/// * A byte offset from `gGkAtoms` to the `nsStaticAtom` object (shifted to +/// the left one bit, and with the lower bit set to `1` to differentiate it +/// from the above), so `(offset << 1 | 1)`. /// -/// This is either a strong reference to a dynamic atom (an nsAtom pointer), -/// or an offset from gGkAtoms to the nsStaticAtom object. #[derive(Eq, PartialEq)] #[repr(C)] pub struct Atom(NonZeroUsize); @@ -87,7 +92,7 @@ fn static_atoms() -> &'static [nsStaticAtom; STATIC_ATOM_COUNT] { fn valid_static_atom_addr(addr: usize) -> bool { unsafe { let atoms = static_atoms(); - let start = atoms.get_unchecked(0) as *const _; + let start = atoms.as_ptr(); let end = atoms.get_unchecked(STATIC_ATOM_COUNT) as *const _; let in_range = addr >= start as usize && addr < end as usize; let aligned = addr % mem::align_of::() == 0; @@ -379,17 +384,16 @@ impl Atom { } /// Creates a static atom from its index in the static atom table, without - /// checking in release builds. + /// checking. #[inline] - pub unsafe fn from_index(index: u16) -> Self { - let ptr = static_atoms().get_unchecked(index as usize) as *const _; - let handle = make_static_handle(ptr); - let atom = Atom(handle); - debug_assert!(valid_static_atom_addr(ptr as usize)); - debug_assert!(atom.is_static()); - debug_assert!((*atom).is_static()); - debug_assert!(handle == make_handle(atom.as_ptr())); - atom + pub const unsafe fn from_index_unchecked(index: u16) -> Self { + // FIXME(emilio): No support for debug_assert! in const fn for now. Note + // that violating this invariant will debug-assert in the `Deref` impl + // though. + // + // debug_assert!((index as usize) < STATIC_ATOM_COUNT); + let offset = (index as usize) * std::mem::size_of::() + kGkAtomsArrayOffset as usize; + Atom(NonZeroUsize::new_unchecked((offset << 1) | 1)) } /// Creates an atom from an atom pointer. diff --git a/components/style/style_adjuster.rs b/components/style/style_adjuster.rs index ac528f4fb37..6a3dc75f4d8 100644 --- a/components/style/style_adjuster.rs +++ b/components/style/style_adjuster.rs @@ -63,43 +63,38 @@ where { use crate::Atom; - // FIXME(emilio): This should be an actual static. - lazy_static! { - static ref SPECIAL_HTML_ELEMENTS: [Atom; 16] = [ - atom!("br"), - atom!("wbr"), - atom!("meter"), - atom!("progress"), - atom!("canvas"), - atom!("embed"), - atom!("object"), - atom!("audio"), - atom!("iframe"), - atom!("img"), - atom!("video"), - atom!("frame"), - atom!("frameset"), - atom!("input"), - atom!("textarea"), - atom!("select"), - ]; - } + const SPECIAL_HTML_ELEMENTS: [Atom; 16] = [ + atom!("br"), + atom!("wbr"), + atom!("meter"), + atom!("progress"), + atom!("canvas"), + atom!("embed"), + atom!("object"), + atom!("audio"), + atom!("iframe"), + atom!("img"), + atom!("video"), + atom!("frame"), + atom!("frameset"), + atom!("input"), + atom!("textarea"), + atom!("select"), + ]; // https://drafts.csswg.org/css-display/#unbox-svg // // There's a note about "Unknown elements", but there's not a good way to // know what that means, or to get that information from here, and no other // UA implements this either. - lazy_static! { - static ref SPECIAL_SVG_ELEMENTS: [Atom; 6] = [ - atom!("svg"), - atom!("a"), - atom!("g"), - atom!("use"), - atom!("tspan"), - atom!("textPath"), - ]; - } + const SPECIAL_SVG_ELEMENTS: [Atom; 6] = [ + atom!("svg"), + atom!("a"), + atom!("g"), + atom!("use"), + atom!("tspan"), + atom!("textPath"), + ]; // https://drafts.csswg.org/css-display/#unbox-html if element.is_html_element() {