[1.9] Random instance marshal data incompatible with CRuby 1.9

It looks like Marcin did port the Mersenne Twister logic to org.jruby.util.Random at one point, but never hooked it up. I asked him about it, and he said 1.9's logic was still evolving at that point.

So in theory, if we can confirm that our Twister logic is sound, it's a matter of porting the random.c logic into RubyRandom.

Nahi: Since you filed, this, I'm putting the ball in your court to decide what we should do. I personally don't have a problem with Random marshaling differently than 1.9, since it seems like a very peculiar thing to marshal in the first place. But you may have a different opinion...so I leave it up to you.

Bumping to 1.6.1 since it's still ongoing and in Nahi's court. Nahi: Are we close on this or do you want someone to take over?

Sorry for slow progress. Now MT PRNG features are implemented and tested. RubyRandom gets compilable last night. I'll create a topic branch today (in 8 hours).

I hope it gets included in 1.6 since it changes Random sequence of 1.8, too. But it could not be in time... I'll write again after creating the topic branch.

I agree with you...we will include it in 1.6 for final.

I see the topic branch; is it ready to merge?

I thought it needed more effort, but it gets green. spec:ci_18 passes, CRuby 1.8/1.9 tests passes except 1. https://gist.github.com/816817

2 points to get ready for merge.

• Marshal format incompatibility between CRuby 1.9; simply a bug in my current implementation. Must be fixed in a few hours.
• Do we change Array#shuffle, choice, etc. as well?

I'll try to implement Array#shuffle today to see how big changes needed or not.

Wow, many thanks for your help with this Nahi!

On your two points:

• Marshal format incompatibility: Ok. Your tweet a few minutes ago may mean this is now finished too
• Array#shuffle: We certainly could change Array#shuffle to do the 1.9 logic. I have no preference, and I doubt anyone in the world will care either way. If it's not too difficult, go ahead and do it.

I'd say go ahead and get the marshal+random changes in now and resolve this bug. You can either open a separate bug for the Array#shuffle work...or just do it, if it won't be big enough to warrant another bug.

Thanks again I think this is our last Marshal failure too!

Yes, I fixed the first one. Simply a bug around byte order.

I gave a look to RubyArray.java.

• choice, shuffle, and sample uses Random.
• shuffle gets Hash argument 'random' like 'Array#shuffle(random: Random.new(0))'. We should implement it (different topic)
• do we have common functions like OPTHASH_GIVEN_P in CRuby 1.9? RubyString#getEncoder() looks parsing it by itself... (different topic)
• choice is removed from 1.9. We should remove RubyArray#choice (different topic)

So I think we can merge MersenneTwister branch with keeping Array#methods keep as is for this issue since spec:ci_19 passes as well as spec:ci_18 now.

I updated Array#shuffle and Array#sample with Mersenne Twister PRNG.
https://github.com/jruby/jruby/commit/fd957a1956a032be89bd4b309f3c0f2d7f0b6272

This commit might be too aggressive to merge, but when it slips 1.6 it won't be implemented in the future. When this compatibility is needed, it should be merged till 1.6 GA.

The most interesting (and frustrated for me) thing is that no one uses / depends this compatibility. It's safe to skip this feature.

Nahi: I'm comfortable with your changes, but if you think it better we can merge them in 1.6.1. We expect 1.6.x releases to continue to improve compatibility with 1.9, which includes work like yours on Random.

So how about we plan to include this in 1.6.1, but merge it to master (continued target for 1.6.x development) after we release 1.6 final?

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.

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.

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.

I'll merge it.

Merged MersenneTwister branch at [master 8c6dba]