tfpt review "/shelveset:critical;REDMOND\sborde" Microsoft.Scripting.dll: Fix to interpreter to get better stack trace even when call-site caching kicks in. The try-catch needs to be added to all code that can throw an exception in the interpreter (or preferably in one function like Interpreter.Interpret), but I have not done that here. IronRuby: Implements Thread.critical= by using Monitor.Enter/Exit. This will work for 90% or more of scenarios IMO. MRI does not schedule other threads while Thread.critical is true (while IronRuby will), but this is more an implementation detail that falls out with the green threads scheduler, than something that should be by spec. In fact, other threads do get scheduled anyway if the critical thread does Thread.pass etc (added tests to show this behavior if you want to play with it). Added tests Added irtests.bat to run all IronRuby tests with a single command, in four processes -------------- next part -------------- A non-text attachment was scrubbed... Name: critical.diff Type: application/octet-stream Size: 3002 bytes Desc: critical.diff URL: <http://rubyforge.org/pipermail/ironruby-core/attachments/20090105/c2f8acd4/attachment-0001.obj>
I''d rather see the functionality in irtest.bat in IronRuby''s run.bat. That way it runs with run 0. The specs will probably need to be cleaned up to meet RubySpec style guidelines, and to get Brian to approve them. Go ahead and commit them, and I can clean them up, or we can work on them together later. For future reference, style guidelines are here: http://rubyspec.org/wiki/rubyspec/Style_Guide JD -----Original Message----- From: Shri Borde Sent: Monday, January 05, 2009 10:18 AM To: IronRuby External Code Reviewers; DLR Code Reviews Cc: ironruby-core at rubyforge.org Subject: Code Review: Thread.critical tfpt review "/shelveset:critical;REDMOND\sborde" Microsoft.Scripting.dll: Fix to interpreter to get better stack trace even when call-site caching kicks in. The try-catch needs to be added to all code that can throw an exception in the interpreter (or preferably in one function like Interpreter.Interpret), but I have not done that here. IronRuby: Implements Thread.critical= by using Monitor.Enter/Exit. This will work for 90% or more of scenarios IMO. MRI does not schedule other threads while Thread.critical is true (while IronRuby will), but this is more an implementation detail that falls out with the green threads scheduler, than something that should be by spec. In fact, other threads do get scheduled anyway if the critical thread does Thread.pass etc (added tests to show this behavior if you want to play with it). Added tests Added irtests.bat to run all IronRuby tests with a single command, in four processes
The implementation of ThreadOps._CriticalMonitor and ThreadOps._IsInCriticalRegion as class-level fields will cause state information to leak between ScriptRuntimes. These should probably be put on the RubyContext, which can either be done directly or by using RubyContext.GetOrCreateLibraryData to create it dynamically. Looks good otherwise. -----Original Message----- From: Shri Borde Sent: Monday, January 05, 2009 10:18 AM To: IronRuby External Code Reviewers; DLR Code Reviews Cc: ironruby-core at rubyforge.org Subject: Code Review: Thread.critical tfpt review "/shelveset:critical;REDMOND\sborde" Microsoft.Scripting.dll: Fix to interpreter to get better stack trace even when call-site caching kicks in. The try-catch needs to be added to all code that can throw an exception in the interpreter (or preferably in one function like Interpreter.Interpret), but I have not done that here. IronRuby: Implements Thread.critical= by using Monitor.Enter/Exit. This will work for 90% or more of scenarios IMO. MRI does not schedule other threads while Thread.critical is true (while IronRuby will), but this is more an implementation detail that falls out with the green threads scheduler, than something that should be by spec. In fact, other threads do get scheduled anyway if the critical thread does Thread.pass etc (added tests to show this behavior if you want to play with it). Added tests Added irtests.bat to run all IronRuby tests with a single command, in four processes
Good catch. I have fixed the issue. Also updated the tests according to Jim''s suggestions. Also, the last test using Thread.kill was wrong but seemed to succeed since "flunk" fails silently when used on the non-main thread. New patch included if you want to take a look. Will go ahead and submit to Snap. -----Original Message----- From: Curt Hagenlocher Sent: Monday, January 05, 2009 1:33 PM To: Shri Borde; IronRuby External Code Reviewers; DLR Code Reviews Cc: ironruby-core at rubyforge.org Subject: RE: Code Review: Thread.critical The implementation of ThreadOps._CriticalMonitor and ThreadOps._IsInCriticalRegion as class-level fields will cause state information to leak between ScriptRuntimes. These should probably be put on the RubyContext, which can either be done directly or by using RubyContext.GetOrCreateLibraryData to create it dynamically. Looks good otherwise. -----Original Message----- From: Shri Borde Sent: Monday, January 05, 2009 10:18 AM To: IronRuby External Code Reviewers; DLR Code Reviews Cc: ironruby-core at rubyforge.org Subject: Code Review: Thread.critical tfpt review "/shelveset:critical;REDMOND\sborde" Microsoft.Scripting.dll: Fix to interpreter to get better stack trace even when call-site caching kicks in. The try-catch needs to be added to all code that can throw an exception in the interpreter (or preferably in one function like Interpreter.Interpret), but I have not done that here. IronRuby: Implements Thread.critical= by using Monitor.Enter/Exit. This will work for 90% or more of scenarios IMO. MRI does not schedule other threads while Thread.critical is true (while IronRuby will), but this is more an implementation detail that falls out with the green threads scheduler, than something that should be by spec. In fact, other threads do get scheduled anyway if the critical thread does Thread.pass etc (added tests to show this behavior if you want to play with it). Added tests Added irtests.bat to run all IronRuby tests with a single command, in four processes -------------- next part -------------- A non-text attachment was scrubbed... Name: critical.diff Type: application/octet-stream Size: 6524 bytes Desc: critical.diff URL: <http://rubyforge.org/pipermail/ironruby-core/attachments/20090105/9f1d4778/attachment.obj>