Use a phf map for the WebGL extension registry

[nox]

There is no need to build a HashMap at runtime etc, we could just generate it at compile-time.

[InquisitivePenguin]

I would like to take on this issue, if possible. What files or parts of the project handle the WebGL extension registry?

[InquisitivePenguin]

I think I found what parts of the application handle the HashMap of extensions: components/script/dom/webgl_extensions/extensions.rs, but I still can't find the code that initializes the HashMap, or at least not in a way that can be determined at compile time.

[jdm]

@JacksonCoder

servo/components/script/dom/webgl_extensions/extensions.rs

Lines 267 to 276 in 1994712

	fn register_all_extensions(&self) {
	self.register::<ext::exttexturefilteranisotropic::EXTTextureFilterAnisotropic>();
	self.register::<ext::oeselementindexuint::OESElementIndexUint>();
	self.register::<ext::oesstandardderivatives::OESStandardDerivatives>();
	self.register::<ext::oestexturefloat::OESTextureFloat>();
	self.register::<ext::oestexturefloatlinear::OESTextureFloatLinear>();
	self.register::<ext::oestexturehalffloat::OESTextureHalfFloat>();
	self.register::<ext::oestexturehalffloatlinear::OESTextureHalfFloatLinear>();
	self.register::<ext::oesvertexarrayobject::OESVertexArrayObject>();
	}

is the code that fills the hashmap.

[InquisitivePenguin]

@jdm Thanks, I didn't see that. Do you think that the rust-phf crate (https://github.com/sfackler/rust-phf) should be the one we use to generate the compile-time map?

[jdm]

Yes, that's what we use in other places in the codebase too.

[InquisitivePenguin]

Alright, I've been busy the past few days, but I had some time to work on this today. Question: should I define the PHF map inside the WebGLExtensions::new function, and if so, should I contain the definition of the map itself inside another function, then pass that function to the DomRefCell::new function under the features property? Or should I make the map initialize late and create all of it's values on the register_all_extensions function?

[InquisitivePenguin]

I'm almost finished, I'll make a PR either today or tomorrow. I tried to find an idiomatic way to create a list of types so I could could just generate the map in build.rs based on a single, constant list of types. Unforunately, the only crate I was able to find that could do that was typemap, and since I had to impl the Key trait for each type I wanted in the map, I decided it just was easier to just put an entry call manually for each type.

One problem I did encounter was the fact that phf::Map does not implement JSTraceable or MallocSizeOf, which means I'll have to custom-implement those traits for WebGLExtensions.

Should I make tests for the changes I have made?

[jdm]

The changes should be covered by the existing webgl tests.

[InquisitivePenguin]

I'm sorry for the wait.. I've been on vacation recently, and I haven't had the time to work on this. I forgot to mention one other problem I encountered: I need to import the extension types during the build.rs phase so I could generate the PHF map with the names of the extensions... however, I cannot use or import anything from the script crate because that would create a circular dependency. Right now, I'm seeing if I could use the alternative form of PHF map generation (macros).

[InquisitivePenguin]

The macro approach did work, but I still need to implement the traits that cannot be automatically derived due to the missing implementation on the PHF map type. My plan is to create trait implementations for the PHF map inside the crates of the trait implementations, which are within the servo repository, but outside of the script crate. Do you think that would be the best solution?

[jdm]

Are you referring to the WebGLExtension trait, or some other type(s)? In general, I agree that moving types that you require at build time into a separate crate that script can depend upon would be the best solution. Moving WebGLExtension entirely won't work because of the new method which uses types which only exist in script, so that trait would need to be split up somehow if that were the case.

[jdm]

It would definitely be easier to answer your questions if you pushed your code to a branch so it could be examined, and mentioned the error messages that appear.

[InquisitivePenguin]

You're right, I'll push the changes to my fork of the repo. I already fixed the problems with code generation (I just used inline macro generation instead), so I'm not worried about importing types at build time. My question (rephrased) was if I should implement the JSTraceable and MallocSizeOf traits manually on the PHF map type, which would allow me to auto-derive the WebGLExtensions struct.

[jdm]

Those would be useful implementations, yes!

[InquisitivePenguin]

Alright, I'll push to my fork once I finish implementing those.

[InquisitivePenguin]

I'm sorry, I haven't had much time to work on this recently. I did work on this a few days ago, and I found a few things: first of all, you have to use phf-codegen to get the names. What I did was create a map of names that was shared with the extensions and the map of extension names to their values. I also implemented the JSTraceable and MallocSizeOf traits.

[InquisitivePenguin]

Alright, so I did some more work today, and unfortunately I can say that moving this hash map over to a PHF map requires a lot of non-trivial, and some downright impossible things. First of all, I decided to make the map static, so it didn't need to be contained inside the WebGLExtensions struct (and therefore the trait impls were not needed). But then I ran into a problem: since the type we're holding as a value in the map is Box<WebGLExtensionWrapper>, it can't implement Copy, because a Box has a destructor. However, the PHF map stores it values internally as a &'static [V] map, where V is the value type. A static array slice of values can only be generated if the values implement Copy, which Box<WebGLExtensionWrapper> does not. So not only is a PHF map using non-Copyable values not allowed, but generating keys is a huge hassle that will no doubt be confusing for people working with WebGL extensions in the future. For that, the best 'solution' I could find was to generate another PHF map that is both shared with build.rs and the ext module, which contains a mapping of types names to their extension name. That way, the names can be used in the extension PHF map.
Honestly, I think that the mild performance gain we'd get from using a PHF map is not worth it. Instead, I was thinking we could use a lazy_static! HashMap. That way, we don't have to explicitly register the extensions. We wouldn't have to use the PHF map for extension names, either.

[jdm]

Yeah, that sounds much more complex than I anticipated; sorry! A lazy_static sounds like a reasonable compromise.

[InquisitivePenguin]

Alright, I've pushed the code: https://github.com/jacksoncoder/servo. I also added the cascade! macro as a dependency to make HashMap generation look a little bit more clean.

[InquisitivePenguin]

Should I make a PR?

[jdm]

Looking at the code, I see a problem with the current implementation. The WebGLExtensionWrapper type requires JSTraceable so that when the GC encounters a WebGLExtensions object it knows what to do with the extensions member, which contains pointers to other objects that can be GCed. If we move that hashtable out of the WebGLExtensions object, the GC no longer sees it in the full object graph, so we risk storing pointers to extension objects that will be collected without our knowledge.

We could fix this by adding a GC hook that specifically traces the hashtable in

servo/components/script/script_runtime.rs

Line 444 in e7791f9

unsafe extern fn trace_rust_roots(tr: *mut JSTracer, _data: *mut os::raw::c_void) {

. However, the sync requirements of lazy_static also make us lie and implement a trait unsafely. It would be better to use thread-local storage which doesn't require Sync.

[InquisitivePenguin]

So essentially, the problem is that having a static hash map results in the possibility of dangling pointers, where data the extensions are keeping track of are being garbage collected because the GC doesn't know the hash table exists? And in order to fix it, we'll have to make the hash table thread-local and call trace_rust_roots manually for the WebGLExtensions::trace method? If so, the only problem I see is that I don't know if a thread-local lazy_static is possible, but I'll look into it.

[nox]

All I wanted to replace is the correspondance from str to the function that enables the corresponding extension, I was on PTO and now that I'm reading about this again I'm not sure what lazy_static and custom hooks have to do with that. :/

[InquisitivePenguin]

At this point, I think that creating a thread-local lazy_static HashMap would be nearly the same thing as just having the HashMap be part of the WebGLExtensions struct, but the latter would be much easier to maintain.