apply review

youknowone · youknowone · commit e1126fc09fd7 · 2026-01-10T21:09:00.000+09:00
diff --git a/crates/vm/src/stdlib/sys.rs b/crates/vm/src/stdlib/sys.rs
@@ -1109,23 +1109,15 @@ mod sys {
 
     #[pyfunction]
     fn _settraceallthreads(tracefunc: PyObjectRef, vm: &VirtualMachine) {
-        let func = if vm.is_none(&tracefunc) {
-            None
-        } else {
-            Some(tracefunc.clone())
-        };
+        let func = (!vm.is_none(&tracefunc)).then(|| tracefunc.clone());
         *vm.state.global_trace_func.lock() = func;
         vm.trace_func.replace(tracefunc);
         update_use_tracing(vm);
     }
 
     #[pyfunction]
     fn _setprofileallthreads(profilefunc: PyObjectRef, vm: &VirtualMachine) {
-        let func = if vm.is_none(&profilefunc) {
-            None
-        } else {
-            Some(profilefunc.clone())
-        };
+        let func = (!vm.is_none(&profilefunc)).then(|| profilefunc.clone());
         *vm.state.global_profile_func.lock() = func;
         vm.profile_func.replace(profilefunc);
         update_use_tracing(vm);
diff --git a/crates/vm/src/stdlib/thread.rs b/crates/vm/src/stdlib/thread.rs
@@ -617,24 +617,37 @@ pub(crate) mod _thread {
     impl Local {
         fn l_dict(&self, vm: &VirtualMachine) -> PyDictRef {
             let thread_id = std::thread::current().id();
-            let mut data = self.inner.data.lock();
 
-            if let Some(dict) = data.get(&thread_id) {
-                return dict.clone();
+            // Fast path: check if dict exists under lock
+            if let Some(dict) = self.inner.data.lock().get(&thread_id).cloned() {
+                return dict;
             }
 
-            // Create new dict for this thread
-            let dict = vm.ctx.new_dict();
-            data.insert(thread_id, dict.clone());
+            // Slow path: allocate dict outside lock to reduce lock hold time
+            let new_dict = vm.ctx.new_dict();
 
-            // Register cleanup guard for this thread
-            let guard = LocalGuard {
-                local: std::sync::Arc::downgrade(&self.inner),
-                thread_id,
+            // Insert with double-check to handle races
+            let mut data = self.inner.data.lock();
+            use std::collections::hash_map::Entry;
+            let (dict, need_guard) = match data.entry(thread_id) {
+                Entry::Occupied(e) => (e.get().clone(), false),
+                Entry::Vacant(e) => {
+                    e.insert(new_dict.clone());
+                    (new_dict, true)
+                }
             };
-            LOCAL_GUARDS.with(|guards| {
-                guards.borrow_mut().push(guard);
-            });
+            drop(data); // Release lock before TLS access
+
+            // Register cleanup guard only if we inserted a new entry
+            if need_guard {
+                let guard = LocalGuard {
+                    local: std::sync::Arc::downgrade(&self.inner),
+                    thread_id,
+                };
+                LOCAL_GUARDS.with(|guards| {
+                    guards.borrow_mut().push(guard);
+                });
+            }
 
             dict
         }
@@ -760,7 +773,14 @@ pub(crate) mod _thread {
 
         #[pymethod]
         fn join(&self, timeout: OptionalArg<Option<f64>>, vm: &VirtualMachine) -> PyResult<()> {
-            let _timeout_val = timeout.flatten().unwrap_or(-1.0);
+            // Validate timeout: None or negative means wait forever, positive is not supported
+            if let OptionalArg::Present(Some(t)) = timeout
+                && t > 0.0
+            {
+                return Err(vm.new_value_error(
+                    "timeout parameter is not supported for _ThreadHandle.join()".to_owned(),
+                ));
+            }
 
             // Check for self-join and if already joined
             let join_handle = {
