Charles,

Thanks for persisting with me. I'm not deliberately being unreasonable :)
I think there are real issues around this case and others like them,
regarding what we expect from Ruby's standard behaviour.

It turns out that I've worked around the original issue, which occurred
in my forthcoming activefacts-api gem (forthcoming because although it's
old code, its about to get to 1.0 and get publicly announced). The
workaround is mostly successful in hiding the problem(s).

But anyhow...

On 04/20/11 08:41, Charles Oliver Nutter wrote:
> On Thu, Apr 14, 2011 at 12:55 AM, Clifford Heath<no / spam.please.net>  wrote:
>> I presented results from MRI 1.8.7, JRuby 1.6.0, and Rubinius,
>> and showed that they all had different shortcuts, and none
>> reliably kept the Hash contract (of using eql? and hash).
>> I.e. you can't rely on sensible code working the same in MRI
>> and JRuby.
>
> I posted results on JRuby master (1.6.1). You responded to that email with:
>
> "That result is reasonable, using MRI. Interesting, and nice, that it
> figures out it doesn't need to call Fixnum#hash to be able to choose
> a bucket. (BTW, that *uncommon* case means there's a test and branch
> which is superfluous and excessively costly for the more common case,
> according to arguments you've made against detecting monkey patching)

I'm sorry for this comment, it's wrong. I must have been confused.
MRI doesn't call Fixnum#hash in any case, but uses a shortcut.

> If you do the same thing with JRuby however, the first test contains this:
> Looking up using Integer:
> nil"
> I'm not sure you actually looked at my results.
>> I think that's a problem. If you don't, then I'm done...
> It may be a problem, or it may not. When running your example,
> however, it seems to call your monkeypatched code in many places where
> you claim it doesn't. So I'm still confused.

I think that will have to go down to communication error.  Let it ride.

> I know we don't call
> hash/eql? in all cases, but I'm trying to quantify what the correct
> behavior should be and what that behavior would cost.

The correct behavior would result in that last "Looking up using Integer:"
to find the value, as MRI does. MRI doesn't shortcut Fixnum#eql? in the
hash lookup, and my Number hash function is defined in terms of Integer#hash,
so it works. It would be better if MRI knew it shouldn't shortcut Fixnum#hash,
but I can live without that as long it's clear.
  
> I'd be happy to continue discussing this as a JRuby issue. Would you
> file something at http://bugs.jruby.org with expected and actual JRuby
> 1.6.1 results?

Ok, perhaps. I think it's a Ruby issue though, not just a JRuby one.
  
>> Unless you care to point me to the place in the JRuby code
>> where this shortcut occurs (where I could make a change to
>> make it invisible), and a performance benchmark that would
>> show the effect of doing so. Then I'll happily make the
>> experiment to see whether I'm right (and the shortcut can
>> be made invisible without affecting performance measurably,
>> i.e. above the noise level of the benchmark). If I'm not,
>> I'll openly admit I was wrong... but I've done some pretty
>> hardcore optimizing in machine code before, and I think I
>> can win this one.
>
> I think you are underestimating the cost of performing a dynamic call.

I'm not. I'm expecting that JRuby would detect that a core Fixnum
method has been monkey-patched, and set a global variable. If  the
variable is set (an inline check, susceptible to branch-prediction)
then it would default to conservative behaviour by calling dispatch,
otherwise continue with the shortcuts as normal.

I think this is what you mean when you say:

> Now using an even faster check, that only dispatches to "hash" if the
> object is a Fixnum and the Fixnum class has not been reopened. Notice
> it's faster than full dyncall, but still a good bit slower than the
> fast path:

If I read the result correctly, that's 0.871/0.753 or 15.6% slower,
on a test that does nothing but Fixnum hash lookups in a hash with
a single element.

What I'd ask is, what's the percentage of time in actual programs
typically spent doing Fixnum hash lookups? I'd wager it's less than
1%, meaning your 16% is now 0.16% - and it makes the behaviour
incorrect per the Ruby spec. Personally, I'd take the hit.

The cost of the inline check of this variable is what I was implying
would vanish into the dust in performance tests, as the branch
prediction figures out that "we always go this way". Is your
implementation of this check implemented to make best advantage of
branch prediction? Frankly I'm rather amazed that it's as big as 18%
merely to decide to use the shortcut. Does that %age drop when using
a hash with more than one entry? It seems like a benchmark that was
created to magnify the change, not to realistically demonstrate the
effect.

> Bottom line is that *any* additional branching logic will add
> overhead, and full dynamic calling introduces even more overhead on
> just about any implementation. Whether that's a fair trade-off is not
> for me to decide ;)

Well, yes and no. If the effect is 0.16% in typical apps, you'd be
well within your right to maintain it should work like MRI and/or
the documentation.

Do you still think this needs a JRuby bug report?

Clifford Heath.