Lookups into HashMap<Atom, ...> have to create needless Atoms

[pshaughn]

There's a comment pointing out this matter in:

servo/components/script/dom/stylepropertymapreadonly.rs

Lines 64 to 77 in 5f55cd5

	impl StylePropertyMapReadOnlyMethods for StylePropertyMapReadOnly {
	/// <https://drafts.css-houdini.org/css-typed-om-1/#dom-stylepropertymapreadonly-get>
	fn Get(&self, property: DOMString) -> Option<DomRoot<CSSStyleValue>> {
	// TODO: avoid constructing an Atom
	self.entries
	.get(&Atom::from(property))
	.map(|value| DomRoot::from_ref(&**value))
	}
	/// <https://drafts.css-houdini.org/css-typed-om-1/#dom-stylepropertymapreadonly-has>
	fn Has(&self, property: DOMString) -> bool {
	// TODO: avoid constructing an Atom
	self.entries.contains_key(&Atom::from(property))
	}

but it's true in most or all places we have a HashMap with Atom keys.

What we should do instead is first ask "Is this string in the atom cache?", since if it's not then there's obviously no way for it to be a key of the HashMap.

Does string_cache already support asking that question?

[pshaughn]

If there's not already a way to do it, I'd suggest something like Atom::from_if_exists(&str) -> Option<Atom>.

[jdm]

There is no way to determine if there's a string in the atom cache, as far as I can tell. The question is also not quite as clear to answer because of the presence of inline atoms, as shown in https://github.com/servo/string-cache/blob/af2c7707e797768660b3db90066b80218dbca6f7/src/atom.rs#L171-L205.

[pshaughn]

I see. Very short strings get packed verbatim instead of converted to hashes, so there's no actual cache data structure including them. There's no static allocation there, so always creating those short atoms isn't a problem the way creating long ones could be. I could see optimizing with a method that returns Some(atom) if it's either inline or in-cache and None if it's neither, but I can't think of what to name that method or how to document its semantics in a way that isn't exposing too much internal state.

[pshaughn]

A way to encapsulate this idea without exposing which strings are inlined could be to make string-cache itself define a struct like AtomHashMap<T> that wraps an internal HashMap<Atom, T> but also has the traits for HashMap<String, T> and does this existence check in that getter.

[SimonSapin]

We could add Atom::something(&str) -> Result<Atom, ()> whose semantics would be: if Err is returned, there was at least one point in time during the duration of the call where there was no other Atom alive in the process with that string value. It may have been created since.

(The implementation would be the same as Atom::from(&str) for inline and static atoms, and for dynamic atoms it would do a hash map lookup without inserting. This still involves acquiring the global Mutex, though in principle we could make it a RwLock.)

These semantics are weak, but sufficient to skip a lookup in HashMap<Atom, _> if the map is known not to have concurrent mutation from other threads. (std::collections::HashMap does not have interior mutability, so holding either &mut HashMap or &HashMap is enough to ensure this.)

However this feels very much like a micro-optimization. Do the creation and destruction of dynamic atoms show up while profiling?

[pshaughn]

I doubt it; the thing that I'm thinking about is that someone might be running some script in a loop that does a large number of strings and then we're carrying those interned atoms around in RAM forever.

I don't disagree with the papercut label.

[SimonSapin]

Dynamic atoms are reference counted, so they won’t stay in RAM forever unless something keeps using them.