Details

    • Type: Task Task
    • Status: Resolved Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: JRuby 1.6RC1
    • Fix Version/s: JRuby 1.7.0.pre1
    • Component/s: Core Classes/Modules
    • Labels:
      None
    • Environment:
      jruby 1.6.0.RC1 (ruby 1.8.7 patchlevel 330) (2011-01-12 71f8afd) (OpenJDK 64-Bit Server VM 1.6.0_20) [linux-amd64-java]
    • Number of attachments :
      0

      Description

      CRuby's Random (uses Mersenne Twister) serializes it's state, left bytes and seed for restoring PRNG's state. We don't support PRNG state dump/load at present.

      So Random's marshal format is incompatible. It would affect to dRuby, etc if one want to Marshal PRNG, I think it's a rare case though.

      0% jruby --1.9 -e 'print Marshal.dump(Random.new)' | ruby -e 'p Marshal.load(STDIN)'
      -e:1:in `marshal_load': wrong argument type Random (expected Bignum) (TypeError)
      	from -e:1:in `load'
      	from -e:1:in `<main>'
      
      0% jruby -v
      jruby 1.6.0.RC1 (ruby 1.8.7 patchlevel 330) (2011-01-12 71f8afd) (OpenJDK 64-Bit Server VM 1.6.0_20) [linux-amd64-java]
      
      0% ruby -v
      ruby 1.9.3dev (2011-01-11 trunk 30513) [x86_64-linux]
      

      By definition, we cannot let it be compatible without implementing exactly the same Mersenne Twister logic as CRuby. Take care, it could cause security issues.

      Shall we do this?

        Activity

        Hide
        Hiroshi Nakamura added a comment -

        It sounds OK for me that adopting Mersenne Twister at 1.6.1 or later but we should take care that it's not just a '1.9.2 Random compatibility improvement' but '1.6.X Random Marshal format incompatibility' I think. jruby1.6.0 -e 'print Marshal.dump(Random.new(0))' | jruby1.6.1 -e 'Marshal.load(ARGF)' would raise an exception, except we introduce more trick on marshal_load (dual Random impl and auto detection?)

        Now I remembered we already hit 'RC' after reading Tom's post at EY blog. Isn't it good to skip adopting Mersenne Twister until 1.7? As I wrote above, it's a rare feature and no one would care about it. For example, 1.8 mode rand() behaves differently from CRuby even if we set the same srand() value but no one claims before.

        For the incompatibility of Array#choice (remove), Array#shuffle! (takes PRNG opt) and Array#sample (takes PRNG opt), these are simply issues of '1.9.2 compatibility improvement'. Shall I fix it based on current Random implementation if you don't mind. I read these parts of CRuby and JRuby source and it should be easier to fix for me now. Don't expect it after a few months later.

        Show
        Hiroshi Nakamura added a comment - It sounds OK for me that adopting Mersenne Twister at 1.6.1 or later but we should take care that it's not just a '1.9.2 Random compatibility improvement' but '1.6.X Random Marshal format incompatibility' I think. jruby1.6.0 -e 'print Marshal.dump(Random.new(0))' | jruby1.6.1 -e 'Marshal.load(ARGF)' would raise an exception, except we introduce more trick on marshal_load (dual Random impl and auto detection?) Now I remembered we already hit 'RC' after reading Tom's post at EY blog. Isn't it good to skip adopting Mersenne Twister until 1.7? As I wrote above, it's a rare feature and no one would care about it. For example, 1.8 mode rand() behaves differently from CRuby even if we set the same srand() value but no one claims before. For the incompatibility of Array#choice (remove), Array#shuffle! (takes PRNG opt) and Array#sample (takes PRNG opt), these are simply issues of '1.9.2 compatibility improvement'. Shall I fix it based on current Random implementation if you don't mind. I read these parts of CRuby and JRuby source and it should be easier to fix for me now. Don't expect it after a few months later.
        Hide
        Charles Oliver Nutter added a comment -

        Nahi: Your concern about making the change makes me concerned. I agree, let's just push it to 1.7, since it's noncritical for the majority of users but it has very visible differences.

        Go ahead and fix Array#choice/shuffle/sample using the current random logic and push it to master for 1.6 RC3/final. I've marked this bug for 1.7. If someone complains before we get into 1.6.1 fixes, we can reconsider.

        Show
        Charles Oliver Nutter added a comment - Nahi: Your concern about making the change makes me concerned. I agree, let's just push it to 1.7, since it's noncritical for the majority of users but it has very visible differences. Go ahead and fix Array#choice/shuffle/sample using the current random logic and push it to master for 1.6 RC3/final. I've marked this bug for 1.7. If someone complains before we get into 1.6.1 fixes, we can reconsider.
        Hide
        Hiroshi Nakamura added a comment -

        Updated Array methods at master branch.

        commit f6919844694619c55aff1bc529f947f7afeaa79e
        Author: Hiroshi Nakamura <nahi@ruby-lang.org>
        Date:   Fri Feb 18 01:08:54 2011 +0900
        
            Array method cleanups which uses PRNG
            
             - Remove Array#choice in 1.9 mode. CRuby 1.9.* does not have it.
             - Add RubyRandom.randomReal for rb_random_real impl.
             - Update Array#shuffle and Array#sample to take Random instance as an
               optional keyword argument.
        
        commit b40f03144e97c54de225dc15a257fe15e315c7f1
        Author: Hiroshi Nakamura <nahi@ruby-lang.org>
        Date:   Thu Feb 10 00:27:28 2011 +0900
        
            Add TypeConverter.checkHashType
            
            rb_check_hash_type in 1.9, which is used in OPTHASH_GIVEN_P macro.
        
        Show
        Hiroshi Nakamura added a comment - Updated Array methods at master branch. commit f6919844694619c55aff1bc529f947f7afeaa79e Author: Hiroshi Nakamura <nahi@ruby-lang.org> Date: Fri Feb 18 01:08:54 2011 +0900 Array method cleanups which uses PRNG - Remove Array#choice in 1.9 mode. CRuby 1.9.* does not have it. - Add RubyRandom.randomReal for rb_random_real impl. - Update Array#shuffle and Array#sample to take Random instance as an optional keyword argument. commit b40f03144e97c54de225dc15a257fe15e315c7f1 Author: Hiroshi Nakamura <nahi@ruby-lang.org> Date: Thu Feb 10 00:27:28 2011 +0900 Add TypeConverter.checkHashType rb_check_hash_type in 1.9, which is used in OPTHASH_GIVEN_P macro.
        Hide
        Hiroshi Nakamura added a comment -

        I'll merge it.

        Show
        Hiroshi Nakamura added a comment - I'll merge it.
        Hide
        Hiroshi Nakamura added a comment -

        Merged MersenneTwister branch at [master 8c6dba]

        Show
        Hiroshi Nakamura added a comment - Merged MersenneTwister branch at [master 8c6dba]

          People

          • Assignee:
            Hiroshi Nakamura
            Reporter:
            Hiroshi Nakamura
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: