Bug 1500362 - Use atom handles in favour of atom pointers in style system code r=emilio
authorCameron McCormack <cam@mcc.id.au>
Mon, 07 Jan 2019 07:54:46 +0000
changeset 509869 f4151dfd56e567676cd32ea74a46626550a78763
parent 509868 c8d48fc7201d7fea60354fa81c4dce0fc5d85597
child 509870 b7e4f2bbfe5705e81877fdbe2170445f5979a65c
push id10547
push userffxbld-merge
push dateMon, 21 Jan 2019 13:03:58 +0000
treeherdermozilla-beta@24ec1916bffe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersemilio
bugs1500362
milestone66.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1500362 - Use atom handles in favour of atom pointers in style system code r=emilio Differential Revision: https://phabricator.services.mozilla.com/D15078
Cargo.lock
layout/style/ServoBindings.toml
servo/components/style/Cargo.toml
servo/components/style/gecko/regen_atoms.py
servo/components/style/gecko_string_cache/mod.rs
servo/components/style/lib.rs
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -2415,17 +2415,16 @@ name = "style"
 version = "0.0.1"
 dependencies = [
  "app_units 0.7.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "arrayvec 0.4.6 (registry+https://github.com/rust-lang/crates.io-index)",
  "atomic_refcell 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "bindgen 0.43.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "bitflags 1.0.4 (registry+https://github.com/rust-lang/crates.io-index)",
  "byteorder 1.2.7 (registry+https://github.com/rust-lang/crates.io-index)",
- "cfg-if 0.1.2 (registry+https://github.com/rust-lang/crates.io-index)",
  "cssparser 0.25.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "euclid 0.19.4 (registry+https://github.com/rust-lang/crates.io-index)",
  "fallible 0.0.1",
  "fxhash 0.2.1 (registry+https://github.com/rust-lang/crates.io-index)",
  "hashglobe 0.1.0",
  "itertools 0.7.6 (registry+https://github.com/rust-lang/crates.io-index)",
  "itoa 0.4.1 (registry+https://github.com/rust-lang/crates.io-index)",
  "lazy_static 1.0.1 (registry+https://github.com/rust-lang/crates.io-index)",
--- a/layout/style/ServoBindings.toml
+++ b/layout/style/ServoBindings.toml
@@ -161,16 +161,17 @@ whitelist-vars = [
     "BORDER_STYLE_.*",
     "CSS_PSEUDO_ELEMENT_.*",
     "SERVO_CSS_PSEUDO_ELEMENT_FLAGS_.*",
     "kNameSpaceID_.*",
     "kGenericFont_.*",
     "kPresContext_.*",
     "nsContentUtils_.*",
     "GECKO_IS_NIGHTLY",
+    "mozilla::detail::gGkAtoms",
 ]
 whitelist-types = [
     "RawGecko.*",
     "RawServoAnimationValueMap",  # TODO(heycam): Handle this elsewhere.
     "mozilla::DeclarationBlockMutationClosure",
     "mozilla::AnimationPropertySegment",
     "mozilla::AnonymousCounterStyle",
     "mozilla::AtomArray",
--- a/servo/components/style/Cargo.toml
+++ b/servo/components/style/Cargo.toml
@@ -24,17 +24,16 @@ servo = ["serde", "style_traits/servo", 
 gecko_debug = []
 
 [dependencies]
 app_units = "0.7"
 arrayvec = "0.4.6"
 atomic_refcell = "0.1"
 bitflags = "1.0"
 byteorder = "1.0"
-cfg-if = "0.1.0"
 cssparser = "0.25"
 crossbeam-channel = { version = "0.3", optional = true }
 new_debug_unreachable = "1.0"
 encoding_rs = {version = "0.8", optional = true}
 euclid = "0.19"
 fallible = { path = "../fallible" }
 fxhash = "0.2"
 hashglobe = { path = "../hashglobe" }
--- a/servo/components/style/gecko/regen_atoms.py
+++ b/servo/components/style/gecko/regen_atoms.py
@@ -117,94 +117,35 @@ PRELUDE = '''
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
 
 // Autogenerated file created by components/style/gecko/regen_atoms.py.
 // DO NOT EDIT DIRECTLY
 '''[1:]
 
-IMPORTS = '''
-use gecko_bindings::structs::nsStaticAtom;
-use string_cache::Atom;
-'''
-
-UNSAFE_STATIC = '''
-#[inline(always)]
-pub unsafe fn atom_from_static(ptr: *const nsStaticAtom) -> Atom {
-    Atom::from_static(ptr)
-}
-'''
-
-SATOMS_TEMPLATE = '''
-            #[link_name = \"{link_name}\"]
-            pub static nsGkAtoms_sAtoms: *const nsStaticAtom;
-'''[1:]
-
-CFG_IF_TEMPLATE = '''
-cfg_if! {{
-    if #[cfg(not(target_env = "msvc"))] {{
-        extern {{
-{gnu}\
-        }}
-    }} else if #[cfg(target_pointer_width = "64")] {{
-        extern {{
-{msvc64}\
-        }}
-    }} else {{
-        extern {{
-{msvc32}\
-        }}
-    }}
-}}\n
-'''
-
-CONST_TEMPLATE = '''
-pub const k_{name}: isize = {index};
-'''[1:]
-
 RULE_TEMPLATE = '''
-("{atom}") =>
-    {{{{
-        use $crate::string_cache::atom_macro;
+    ("{atom}") => {{{{
         #[allow(unsafe_code)] #[allow(unused_unsafe)]
-        unsafe {{ atom_macro::atom_from_static(atom_macro::nsGkAtoms_sAtoms.offset(atom_macro::k_{name})) }}
+        unsafe {{ $crate::string_cache::Atom::from_index({index}) }}
     }}}};
 '''[1:]
 
 MACRO_TEMPLATE = '''
 #[macro_export]
 macro_rules! atom {{
 {body}\
 }}
 '''
 
 def write_atom_macro(atoms, file_name):
     with FileAvoidWrite(file_name) as f:
         f.write(PRELUDE)
-        f.write(IMPORTS)
-        f.write(UNSAFE_STATIC)
-
-        gnu_name='_ZN9nsGkAtoms6sAtomsE'
-        gnu_symbols = SATOMS_TEMPLATE.format(link_name=gnu_name)
-
-        # Prepend "\x01" to avoid LLVM prefixing the mangled name with "_".
-        # See https://github.com/rust-lang/rust/issues/36097
-        msvc32_name = '\\x01?sAtoms@nsGkAtoms@@0QBVnsStaticAtom@@B'
-        msvc32_symbols = SATOMS_TEMPLATE.format(link_name=msvc32_name)
-
-        msvc64_name = '?sAtoms@nsGkAtoms@@0QEBVnsStaticAtom@@EB'
-        msvc64_symbols = SATOMS_TEMPLATE.format(link_name=msvc64_name)
-
-        f.write(CFG_IF_TEMPLATE.format(gnu=gnu_symbols, msvc32=msvc32_symbols, msvc64=msvc64_symbols))
-
-        consts = [CONST_TEMPLATE.format(name=atom.ident, index=i) for (i, atom) in enumerate(atoms)]
-        f.write('{}'.format(''.join(consts)))
-
-        macro_rules = [RULE_TEMPLATE.format(atom=atom.value, name=atom.ident) for atom in atoms]
+        macro_rules = [RULE_TEMPLATE.format(atom=atom.value, name=atom.ident, index=i)
+                       for (i, atom) in enumerate(atoms)]
         f.write(MACRO_TEMPLATE.format(body=''.join(macro_rules)))
 
 
 def write_pseudo_elements(atoms, target_filename):
     pseudos = []
     for atom in atoms:
         if atom.type() == "nsCSSPseudoElementStaticAtom" or atom.type() == "nsCSSAnonBoxPseudoStaticAtom":
             pseudos.append(atom)
--- a/servo/components/style/gecko_string_cache/mod.rs
+++ b/servo/components/style/gecko_string_cache/mod.rs
@@ -10,16 +10,17 @@
 
 //! A drop-in replacement for string_cache, but backed by Gecko `nsAtom`s.
 
 use crate::gecko_bindings::bindings::Gecko_AddRefAtom;
 use crate::gecko_bindings::bindings::Gecko_Atomize;
 use crate::gecko_bindings::bindings::Gecko_Atomize16;
 use crate::gecko_bindings::bindings::Gecko_ReleaseAtom;
 use crate::gecko_bindings::structs::{nsAtom, nsDynamicAtom, nsStaticAtom};
+use crate::gecko_bindings::structs::root::mozilla::detail::gGkAtoms;
 use nsstring::{nsAString, nsStr};
 use precomputed_hash::PrecomputedHash;
 use std::borrow::{Borrow, Cow};
 use std::char::{self, DecodeUtf16};
 use std::fmt::{self, Write};
 use std::hash::{Hash, Hasher};
 use std::iter::Cloned;
 use std::ops::Deref;
@@ -38,32 +39,54 @@ pub mod namespace;
 pub use self::namespace::{Namespace, WeakNamespace};
 
 macro_rules! local_name {
     ($s:tt) => {
         atom!($s)
     };
 }
 
-/// A strong reference to a Gecko atom.
+/// A handle to a Gecko atom.
+///
+/// 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)]
-pub struct Atom(*mut WeakAtom);
+pub struct Atom(usize);
 
 /// An atom *without* a strong reference.
 ///
 /// Only usable as `&'a WeakAtom`,
 /// where `'a` is the lifetime of something that holds a strong reference to that atom.
 pub struct WeakAtom(nsAtom);
 
+#[inline]
+fn valid_static_atom_addr(addr: usize) -> bool {
+    unsafe {
+        let start = gGkAtoms.mAtoms.get_unchecked(0) as *const _;
+        let end = gGkAtoms.mAtoms.get_unchecked(gGkAtoms.mAtoms.len()) as *const _;
+        let in_range = addr >= start as usize && addr < end as usize;
+        let aligned = addr % mem::align_of::<nsStaticAtom>() == 0;
+        in_range && aligned
+    }
+}
+
 impl Deref for Atom {
     type Target = WeakAtom;
 
     #[inline]
     fn deref(&self) -> &WeakAtom {
-        unsafe { &*self.0 }
+        unsafe {
+            let addr = if self.is_static() {
+                (&gGkAtoms as *const _ as usize) + (self.0 >> 1)
+            } else {
+                self.0
+            };
+            debug_assert!(!self.is_static() || valid_static_atom_addr(addr));
+            WeakAtom::new(addr as *const nsAtom)
+        }
     }
 }
 
 impl PrecomputedHash for Atom {
     #[inline]
     fn precomputed_hash(&self) -> u32 {
         self.get_hash()
     }
@@ -272,60 +295,83 @@ impl fmt::Display for WeakAtom {
     fn fmt(&self, w: &mut fmt::Formatter) -> fmt::Result {
         for c in self.chars() {
             w.write_char(c.unwrap_or(char::REPLACEMENT_CHARACTER))?
         }
         Ok(())
     }
 }
 
+#[inline]
+unsafe fn make_handle(ptr: *const nsAtom) -> usize {
+    debug_assert!(!ptr.is_null());
+    if !WeakAtom::new(ptr).is_static() {
+        ptr as usize
+    } else {
+        make_static_handle(ptr as *mut nsStaticAtom)
+    }
+}
+
+#[inline]
+unsafe fn make_static_handle(ptr: *const nsStaticAtom) -> usize {
+    // FIXME(heycam): Use offset_from once it's stabilized.
+    // https://github.com/rust-lang/rust/issues/41079
+    debug_assert!(valid_static_atom_addr(ptr as usize));
+    let base = &gGkAtoms as *const _;
+    let offset = ptr as usize - base as usize;
+    (offset << 1) | 1
+}
+
 impl Atom {
+    #[inline]
+    fn is_static(&self) -> bool {
+        self.0 & 1 == 1
+    }
+
     /// Execute a callback with the atom represented by `ptr`.
     pub unsafe fn with<F, R>(ptr: *const nsAtom, callback: F) -> R
     where
         F: FnOnce(&Atom) -> R,
     {
-        let atom = Atom(WeakAtom::new(ptr));
+        let atom = Atom(make_handle(ptr as *mut nsAtom));
         let ret = callback(&atom);
         mem::forget(atom);
         ret
     }
 
-    /// Creates an atom from an static atom pointer without checking in release
-    /// builds.
-    ///
-    /// Right now it's only used by the atom macro, and ideally it should keep
-    /// that way, now we have sugar for is_static, creating atoms using
-    /// Atom::from_raw should involve almost no overhead.
+    /// Creates a static atom from its index in the static atom table, without
+    /// checking in release builds.
     #[inline]
-    pub unsafe fn from_static(ptr: *const nsStaticAtom) -> Self {
-        let atom = Atom(ptr as *mut WeakAtom);
-        debug_assert!(
-            atom.is_static(),
-            "Called from_static for a non-static atom!"
-        );
+    pub unsafe fn from_index(index: u16) -> Self {
+        let ptr = gGkAtoms.mAtoms.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
     }
 
     /// Creates an atom from an atom pointer.
     #[inline(always)]
     pub unsafe fn from_raw(ptr: *mut nsAtom) -> Self {
-        let atom = Atom(ptr as *mut WeakAtom);
+        let atom = Atom(make_handle(ptr));
         if !atom.is_static() {
             Gecko_AddRefAtom(ptr);
         }
         atom
     }
 
-    /// Creates an atom from a dynamic atom pointer that has already had AddRef
-    /// called on it.
+    /// Creates an atom from an atom pointer that has already had AddRef
+    /// called on it. This may be a static or dynamic atom.
     #[inline]
     pub unsafe fn from_addrefed(ptr: *mut nsAtom) -> Self {
         assert!(!ptr.is_null());
-        Atom(WeakAtom::new(ptr))
+        Atom(make_handle(ptr))
     }
 
     /// Convert this atom into an addrefed nsAtom pointer.
     #[inline]
     pub fn into_addrefed(self) -> *mut nsAtom {
         let ptr = self.as_ptr();
         mem::forget(self);
         ptr
@@ -348,17 +394,23 @@ impl Hash for WeakAtom {
     {
         state.write_u32(self.get_hash());
     }
 }
 
 impl Clone for Atom {
     #[inline(always)]
     fn clone(&self) -> Atom {
-        unsafe { Atom::from_raw(self.as_ptr()) }
+        unsafe {
+            let atom = Atom(self.0);
+            if !atom.is_static() {
+                Gecko_AddRefAtom(atom.as_ptr());
+            }
+            atom
+        }
     }
 }
 
 impl Drop for Atom {
     #[inline]
     fn drop(&mut self) {
         if !self.is_static() {
             unsafe {
@@ -372,50 +424,50 @@ impl Default for Atom {
     #[inline]
     fn default() -> Self {
         atom!("")
     }
 }
 
 impl fmt::Debug for Atom {
     fn fmt(&self, w: &mut fmt::Formatter) -> fmt::Result {
-        write!(w, "Gecko Atom({:p}, {})", self.0, self)
+        write!(w, "Atom(0x{:08x}, {})", self.0, self)
     }
 }
 
 impl fmt::Display for Atom {
     fn fmt(&self, w: &mut fmt::Formatter) -> fmt::Result {
-        unsafe { (&*self.0).fmt(w) }
+        self.deref().fmt(w)
     }
 }
 
 impl<'a> From<&'a str> for Atom {
     #[inline]
     fn from(string: &str) -> Atom {
         debug_assert!(string.len() <= u32::max_value() as usize);
         unsafe {
-            Atom(WeakAtom::new(Gecko_Atomize(
+            Atom::from_addrefed(Gecko_Atomize(
                 string.as_ptr() as *const _,
                 string.len() as u32,
-            )))
+            ))
         }
     }
 }
 
 impl<'a> From<&'a [u16]> for Atom {
     #[inline]
     fn from(slice: &[u16]) -> Atom {
         Atom::from(&*nsStr::from(slice))
     }
 }
 
 impl<'a> From<&'a nsAString> for Atom {
     #[inline]
     fn from(string: &nsAString) -> Atom {
-        unsafe { Atom(WeakAtom::new(Gecko_Atomize16(string))) }
+        unsafe { Atom::from_addrefed(Gecko_Atomize16(string)) }
     }
 }
 
 impl<'a> From<Cow<'a, str>> for Atom {
     #[inline]
     fn from(string: Cow<'a, str>) -> Atom {
         Atom::from(&*string)
     }
--- a/servo/components/style/lib.rs
+++ b/servo/components/style/lib.rs
@@ -27,20 +27,16 @@
 
 extern crate app_units;
 extern crate arrayvec;
 extern crate atomic_refcell;
 #[macro_use]
 extern crate bitflags;
 #[allow(unused_extern_crates)]
 extern crate byteorder;
-#[cfg(feature = "gecko")]
-#[macro_use]
-#[no_link]
-extern crate cfg_if;
 #[cfg(feature = "servo")]
 extern crate crossbeam_channel;
 #[macro_use]
 extern crate cssparser;
 #[macro_use]
 extern crate debug_unreachable;
 extern crate euclid;
 extern crate fallible;