THRIFT-2877 Generate hashCode using primitives, static utility methods

[roshan]

This is pretty much the List.hashCode() except without any list. It takes about a third the time on some rudimentary benchmarks. I tried it out with

typedef i32 SomeId
typedef binary BinId
typedef string StringId

struct NonTrue {}

union MaybeAThing {
    1: NonTrue nt
    2: bool bl
}

enum Nomnom {
    EAT=31
    LIVE=515
}

struct AllPrims {
    1: bool boole
    2: byte single_byte
    3: i16 shrt
    4: i32 integ
    5: i64 longue
    6: double f64
    7: string str
    8: binary bin
    9: Nomnom en
    10: NonTrue stru
    11: MaybeAThing un
    12: SomeId intid
    13: BinId binid
    14: StringId strid
}

generating this hashCode().

Populating these structs with some values has it match the AbstractList.hashCode() result but maybe we could try a different multiplicative factor.

Sorry about the other one #447. I rebased to one commit, but couldn't seem to reuse the old PR (it compiled locally on gcc but failed on clang).

[roshan]

Ah, I used Java 8 only methods. I'll fix this and reopen later.

[nsuke]

Hi @roshan, sorry for the delay, I've just discovered this.
While this change makes sense in general, I have one concern.

The TBaseHelper.hashCode methods are the Java 8 implementations of hashCode for
those types.

Does this involve any code-level transplant, like copy-paste ?
In that case I'm afraid we have no way to incorporate this, as OpenJDK is GPL (let alone Oracle JDK disassemble).

[roshan]

Ha, thanks for that @nsuke . I only used the JDK 8 methods so we could keep hashCode identical in value to its current value for people's existing structs. Is that not a concern? If so, it should be easy to write a trivial implementation that does not resemble the JDK 8 methods in functionality or value and I'll be happy to do that.

[nsuke]

@roshan great, thanks for working on it after such a long break.
I would suggest rewriting merely targeting "boxing value not involved" and "reasonably good" hash functions "from scratch."

If the JDK-identical return values are important, we may probably ask https://issues.apache.org/jira/browse/LEGAL if it's feasible first, but it's not the case, I suppose ?

[jsirois]

My 2 nonbinding cents: No reasonable client should care about hashCode values, only that the values meet the hashCode contract (this is true of any language). I don't think preserving them should ever be a goal.

A second comment, not as helpful since it requires work, is that we should enshrine perf improvements using a tool like jmh. We use this in Apache Aurora for example to ensure we have a shared history of performance improvements and regressions.

[roshan]

@nsuke, I switched it so we now consider a long like two ints (the high bits and low bits) and we combine their values like we combine hashCodes (just with a different multiplicative factor). For doubles we get the long represented by their bytes and treat as before. Since we're changing the values, I also took the chance to pick arbitrary constants to combine with.

For what it's worth, though, the previous hashCode helper functions are pretty much what are described in Effective Java. Anyway, I'll rebase and squash after you have a look.

@jsirois That's a good idea. Unfortunately, I don't have the time at the moment to do that :(

[nsuke]

We can take this opportunity to stop throwing string here.
As I believe it should never happen (right ?), std::logic_error would be appropriate.

[nsuke]

Just a style nit, could you indent switch block in the same way as the other part of this file (less indentation) ?

[nsuke]

@roshan I've added comments on minor points. Other than that I think we're good to proceed 👍

[roshan]

Thanks for the comments, @nsuke . I will squash after builds pass.