Use a global static variable for IdNamespace counting. #1882
Conversation
This is currently not the case. With gecko, each window creates a separate render backend thread, so this code is running on multiple thread. |
|
Another possibility could be to have some of the bits of the id namespace representing a unique id per webrender instance, and the other bits representing a unique id within the instance (although a global atomic variable is simple enough as well). |
JerryShih
force-pushed
the
unique-IdNamespace
branch
from
27d99b3 to
10f7974
Compare
|
This version try to use a simple global atomic variable for the id counting. 10f7974 |
| @@ -22,6 +22,7 @@ use resource_cache::ResourceCache; | |||
| use scene::Scene; | |||
| #[cfg(feature = "debugger")] | |||
| use serde_json; | |||
| use std::sync::atomic::{ATOMIC_USIZE_INIT, AtomicUsize, Ordering}; | |||
There was a problem hiding this comment.
The u32is not stable now. Use usize instead.
error: use of unstable library feature 'integer_atomics' (see issue #32976)
--> src\render_backend.rs:25:25
|
25 | use std::sync::atomic::{AtomicU32, Ordering};
|
We want to avoid to use 0 as id name space. But current patch seems to do it. |
|
Thanks. If this approach looks good, I will change the init value to 1. |
webrender/src/render_backend.rs
Outdated
| @@ -124,6 +125,9 @@ enum DocumentOp { | |||
| Rendered(RendererFrame), | |||
| } | |||
|
|
|||
| /// The unique id for WR resource identification. | |||
| static mut NEXT_NAMESPACE_ID: AtomicUsize = ATOMIC_USIZE_INIT; | |||
There was a problem hiding this comment.
This doesn't need the mut for an atomic.
webrender/src/render_backend.rs
Outdated
| let namespace = self.next_namespace_id; | ||
| self.next_namespace_id = IdNamespace(namespace.0 + 1); | ||
| sender.send(namespace).unwrap(); | ||
| unsafe { |
There was a problem hiding this comment.
Once the mut above is removed, the unsafe can also be removed.
JerryShih
force-pushed
the
unique-IdNamespace
branch
from
10f7974 to
c551f5f
Compare
| @@ -163,6 +166,9 @@ impl RenderBackend { | |||
| blob_image_renderer: Option<Box<BlobImageRenderer>>, | |||
| enable_render_on_scroll: bool, | |||
| ) -> RenderBackend { | |||
| // The namespace_id should start from 1. | |||
There was a problem hiding this comment.
I can't set the value to 1 directly, so I just increase the value in RenderBackend::new().
error: std::sync::atomic::AtomicUsize::new is not yet stable as a const fn
--> src\render_backend.rs:129:41
|
129 | static NEXT_NAMESPACE_ID: AtomicUsize = AtomicUsize::new(1);
| ^^^^^^^^^^^^^^^^^^^
|
= help: in Nightly builds, add #![feature(const_atomic_usize_new)] to the crate attributes to enable
|
@glennw |
|
Looks good, thank you! @bors-servo r+ |
|
📌 Commit c551f5f has been approved by |
Use a global static variable for IdNamespace counting. #1794 Try to use a simple global static variable for that id counting. That's not thread safe. All render-backend should be created in the same thread. If we try to handle the thread problem, I will turn to use thread_local. cc @sotaroikeda r? @kvark @glennw <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1882) <!-- Reviewable:end -->
|
☀️ Test successful - status-appveyor, status-travis |
#1794
Try to use a simple global static variable for that id counting. That's not thread safe. All render-backend should be created in the same thread. If we try to handle the thread problem, I will turn to use thread_local.
cc @sotaroikeda
r? @kvark @glennw
This change is