Skip to content

Commit 5a783fb

Browse files
stepanchegfacebook-github-bot
authored andcommitted
Use thread_local! for json/repr stack
Summary: `thread_local!` with `const` initializer generates slightly inefficient but more or or less good code ([issue in rust](rust-lang/rust#104033)) and roughtly equivalent to hack done here before. `#[thread_local]` is no longer needed (earlier this was not possible because `thread_local!` `const` initializer was implemented after this code was implemented.) Now we are down to four features preventing using starlark-rust on stable: ``` #![cfg_attr(rust_nightly, feature(const_type_id))] #![cfg_attr(rust_nightly, feature(core_intrinsics))] #![feature(const_mut_refs)] #![feature(generic_associated_types)] #![feature(maybe_uninit_write_slice)] #![feature(ptr_metadata)] ``` Reviewed By: ndmitchell Differential Revision: D41063194 fbshipit-source-id: 2d2e340c987676887c4f185bd563ac5cb90749f0
1 parent b1fd7f1 commit 5a783fb

File tree

2 files changed

+38
-69
lines changed

2 files changed

+38
-69
lines changed

starlark/src/lib.rs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -356,7 +356,6 @@
356356
#![feature(generic_associated_types)]
357357
#![feature(maybe_uninit_write_slice)]
358358
#![feature(ptr_metadata)]
359-
#![feature(thread_local)]
360359
// Plugins
361360
#![cfg_attr(feature = "gazebo_lint", feature(plugin))]
362361
#![cfg_attr(feature = "gazebo_lint", allow(deprecated))] // :(

starlark/src/values/recursive_repr_or_json_guard.rs

Lines changed: 38 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -32,94 +32,64 @@ pub(crate) struct JsonStackGuard;
3232

3333
impl Drop for ReprStackGuard {
3434
fn drop(&mut self) {
35-
let mut stack = REPR_STACK.take();
36-
let popped = stack.pop();
37-
debug_assert!(popped.is_some());
38-
REPR_STACK.set(stack);
35+
REPR_STACK.with(|repr_stack| {
36+
let mut stack = Cell::take(repr_stack);
37+
let popped = stack.pop();
38+
debug_assert!(popped.is_some());
39+
repr_stack.set(stack);
40+
})
3941
}
4042
}
4143

4244
impl Drop for JsonStackGuard {
4345
fn drop(&mut self) {
44-
let mut stack = JSON_STACK.take();
45-
let popped = stack.pop();
46-
debug_assert!(popped.is_some());
47-
JSON_STACK.set(stack);
46+
JSON_STACK.with(|json_stack| {
47+
let mut stack = Cell::take(json_stack);
48+
let popped = stack.pop();
49+
debug_assert!(popped.is_some());
50+
json_stack.set(stack);
51+
})
4852
}
4953
}
5054

51-
/// Release per-thread memory.
52-
///
53-
/// `#[thread_local]` is fast, but it [does not call destructor on thread exit][1].
54-
/// `thread_local!` is somewhat slower.
55-
///
56-
/// So we use `#[thread_local]` for fast access and `thread_local!` for releasing memory.
57-
///
58-
/// Note this is similar to how `thread_local!` actually implemented,
59-
/// but this is somewhat more efficient.
60-
///
61-
/// [1]: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=874a250ed9642d3e1559a8ba7d26abfb
62-
struct ReleaseMemoryOnThreadExit;
63-
64-
impl Drop for ReleaseMemoryOnThreadExit {
65-
fn drop(&mut self) {
66-
// We replace per-thread data with empty sets.
67-
// So destructors for these fields won't be called,
68-
// but that's fine because we don't need to release memory of empty sets.
69-
REPR_STACK.take();
70-
JSON_STACK.take();
71-
}
72-
}
73-
74-
thread_local! {
75-
static RELEASE_MEMORY_ON_THREAD_EXIT: ReleaseMemoryOnThreadExit = ReleaseMemoryOnThreadExit;
76-
}
77-
78-
/// Register a callback to release memory on thread exit.
79-
#[cold]
80-
#[inline(never)]
81-
fn init_release_memory_on_thread_exit() {
82-
RELEASE_MEMORY_ON_THREAD_EXIT.with(|_| {});
83-
}
84-
8555
/// Returned when `repr` is called recursively and a cycle is detected.
8656
pub(crate) struct ReprCycle;
8757

8858
/// Returned when `to_json` is called recursively and a cycle is detected.
8959
pub(crate) struct JsonCycle;
9060

91-
#[thread_local]
92-
static REPR_STACK: Cell<SmallSet<RawPointer>> = Cell::new(SmallSet::new());
61+
thread_local! {
62+
static REPR_STACK: Cell<SmallSet<RawPointer>> = const { Cell::new(SmallSet::new()) };
63+
}
9364

94-
#[thread_local]
95-
static JSON_STACK: Cell<SmallSet<RawPointer>> = Cell::new(SmallSet::new());
65+
thread_local! {
66+
static JSON_STACK: Cell<SmallSet<RawPointer>> = const { Cell::new(SmallSet::new()) };
67+
}
9668

9769
/// Push a value to the stack, return error if it is already on the stack.
9870
pub(crate) fn repr_stack_push(value: Value) -> Result<ReprStackGuard, ReprCycle> {
99-
let mut stack = REPR_STACK.take();
100-
if unlikely(stack.capacity() == 0) {
101-
init_release_memory_on_thread_exit();
102-
}
103-
if unlikely(!stack.insert(value.ptr_value())) {
104-
REPR_STACK.set(stack);
105-
Err(ReprCycle)
106-
} else {
107-
REPR_STACK.set(stack);
108-
Ok(ReprStackGuard)
109-
}
71+
REPR_STACK.with(|repr_stack| {
72+
let mut stack = Cell::take(repr_stack);
73+
if unlikely(!stack.insert(value.ptr_value())) {
74+
repr_stack.set(stack);
75+
Err(ReprCycle)
76+
} else {
77+
repr_stack.set(stack);
78+
Ok(ReprStackGuard)
79+
}
80+
})
11081
}
11182

11283
/// Push a value to the stack, return error if it is already on the stack.
11384
pub(crate) fn json_stack_push(value: Value) -> Result<JsonStackGuard, JsonCycle> {
114-
let mut stack = JSON_STACK.take();
115-
if unlikely(stack.capacity() == 0) {
116-
init_release_memory_on_thread_exit();
117-
}
118-
if unlikely(!stack.insert(value.ptr_value())) {
119-
JSON_STACK.set(stack);
120-
Err(JsonCycle)
121-
} else {
122-
JSON_STACK.set(stack);
123-
Ok(JsonStackGuard)
124-
}
85+
JSON_STACK.with(|json_stack| {
86+
let mut stack = Cell::take(json_stack);
87+
if unlikely(!stack.insert(value.ptr_value())) {
88+
json_stack.set(stack);
89+
Err(JsonCycle)
90+
} else {
91+
json_stack.set(stack);
92+
Ok(JsonStackGuard)
93+
}
94+
})
12595
}

0 commit comments

Comments
 (0)