[GitHub] thrift pull request: THRIFT-2877 Generate hashCode using primitive...
Github user asfgit closed the pull request at: https://github.com/apache/thrift/pull/448 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request: THRIFT-2877 Generate hashCode using primitive...
Github user roshan commented on the pull request: https://github.com/apache/thrift/pull/448#issuecomment-183718878 Thanks for the comments, @nsuke . I will squash after builds pass. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request: THRIFT-2877 Generate hashCode using primitive...
Github user nsuke commented on a diff in the pull request:
https://github.com/apache/thrift/pull/448#discussion_r52825710
--- Diff: compiler/cpp/src/generate/t_java_generator.cc ---
@@ -1886,24 +1889,57 @@ void
t_java_generator::generate_java_struct_equality(ofstream& out, t_struct* ts
bool can_be_null = type_can_be_null(t);
string name = (*m_iter)->get_name();
-string present = "true";
+if (is_optional || can_be_null) {
+ indent(out) << "hashCode = hashCode * " << MUL << " + ((" <<
generate_isset_check(*m_iter)
+ << ") ? " << B_YES << " : " << B_NO << ");" << endl;
+}
if (is_optional || can_be_null) {
- present += " && (" + generate_isset_check(*m_iter) + ")";
+ indent(out) << "if (" + generate_isset_check(*m_iter) + ")" << endl;
+ indent_up();
}
-indent(out) << "boolean present_" << name << " = " << present << ";"
<< endl;
-indent(out) << "list.add(present_" << name << ");" << endl;
-indent(out) << "if (present_" << name << ")" << endl;
if (t->is_enum()) {
- indent(out) << " list.add(" << name << ".getValue());" << endl;
+ indent(out) << "hashCode = hashCode * " << MUL << " + " << name <<
".getValue();" << endl;
+} else if (t->is_base_type()) {
+ switch(((t_base_type*)t)->get_base()) {
+case t_base_type::TYPE_VOID:
+ break;
+case t_base_type::TYPE_STRING:
+ indent(out) << "hashCode = hashCode * " << MUL << " + " << name
<< ".hashCode();" << endl;
+ break;
+case t_base_type::TYPE_BOOL:
+ indent(out) << "hashCode = hashCode * " << MUL << " + ((" <<
name << ") ? "
+ << B_YES << " : " << B_NO << ");" << endl;
+ break;
+case t_base_type::TYPE_I8:
+ indent(out) << "hashCode = hashCode * " << MUL << " + (int) ("
<< name << ");" << endl;
+ break;
+case t_base_type::TYPE_I16:
+case t_base_type::TYPE_I32:
+ indent(out) << "hashCode = hashCode * " << MUL << " + " << name
<< ";" << endl;
+ break;
+case t_base_type::TYPE_I64:
+ indent(out) << "hashCode = hashCode * " << MUL << " +
org.apache.thrift.TBaseHelper.hashCode(" << name << ");" << endl;
+ break;
+case t_base_type::TYPE_DOUBLE:
+ indent(out) << "hashCode = hashCode * " << MUL << " +
org.apache.thrift.TBaseHelper.hashCode(" << name << ");" << endl;
+ break;
+default:
+ throw "compiler error: the following base type has no hashcode
generator: " +
+ t_base_type::t_base_name(((t_base_type*)t)->get_base());
--- End diff --
Just a style nit, could you indent switch in the same way as the other part
of this file (less indentation) ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-2877 Generate hashCode using primitive...
Github user nsuke commented on a diff in the pull request:
https://github.com/apache/thrift/pull/448#discussion_r52825719
--- Diff: compiler/cpp/src/generate/t_java_generator.cc ---
@@ -1886,24 +1889,57 @@ void
t_java_generator::generate_java_struct_equality(ofstream& out, t_struct* ts
bool can_be_null = type_can_be_null(t);
string name = (*m_iter)->get_name();
-string present = "true";
+if (is_optional || can_be_null) {
+ indent(out) << "hashCode = hashCode * " << MUL << " + ((" <<
generate_isset_check(*m_iter)
+ << ") ? " << B_YES << " : " << B_NO << ");" << endl;
+}
if (is_optional || can_be_null) {
- present += " && (" + generate_isset_check(*m_iter) + ")";
+ indent(out) << "if (" + generate_isset_check(*m_iter) + ")" << endl;
+ indent_up();
}
-indent(out) << "boolean present_" << name << " = " << present << ";"
<< endl;
-indent(out) << "list.add(present_" << name << ");" << endl;
-indent(out) << "if (present_" << name << ")" << endl;
if (t->is_enum()) {
- indent(out) << " list.add(" << name << ".getValue());" << endl;
+ indent(out) << "hashCode = hashCode * " << MUL << " + " << name <<
".getValue();" << endl;
+} else if (t->is_base_type()) {
+ switch(((t_base_type*)t)->get_base()) {
+case t_base_type::TYPE_VOID:
+ break;
+case t_base_type::TYPE_STRING:
+ indent(out) << "hashCode = hashCode * " << MUL << " + " << name
<< ".hashCode();" << endl;
+ break;
+case t_base_type::TYPE_BOOL:
+ indent(out) << "hashCode = hashCode * " << MUL << " + ((" <<
name << ") ? "
+ << B_YES << " : " << B_NO << ");" << endl;
+ break;
+case t_base_type::TYPE_I8:
+ indent(out) << "hashCode = hashCode * " << MUL << " + (int) ("
<< name << ");" << endl;
+ break;
+case t_base_type::TYPE_I16:
+case t_base_type::TYPE_I32:
+ indent(out) << "hashCode = hashCode * " << MUL << " + " << name
<< ";" << endl;
+ break;
+case t_base_type::TYPE_I64:
+ indent(out) << "hashCode = hashCode * " << MUL << " +
org.apache.thrift.TBaseHelper.hashCode(" << name << ");" << endl;
+ break;
--- End diff --
It seems it can be merged to `TYPE_DOUBLE:` below.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-2877 Generate hashCode using primitive...
Github user nsuke commented on the pull request: https://github.com/apache/thrift/pull/448#issuecomment-183632632 @roshan I've added comments on minor points. Other than that I think we're good to proceed :+1: --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request: THRIFT-2877 Generate hashCode using primitive...
Github user nsuke commented on a diff in the pull request:
https://github.com/apache/thrift/pull/448#discussion_r52825674
--- Diff: compiler/cpp/src/generate/t_java_generator.cc ---
@@ -1886,24 +1889,57 @@ void
t_java_generator::generate_java_struct_equality(ofstream& out, t_struct* ts
bool can_be_null = type_can_be_null(t);
string name = (*m_iter)->get_name();
-string present = "true";
+if (is_optional || can_be_null) {
+ indent(out) << "hashCode = hashCode * " << MUL << " + ((" <<
generate_isset_check(*m_iter)
+ << ") ? " << B_YES << " : " << B_NO << ");" << endl;
+}
if (is_optional || can_be_null) {
- present += " && (" + generate_isset_check(*m_iter) + ")";
+ indent(out) << "if (" + generate_isset_check(*m_iter) + ")" << endl;
+ indent_up();
}
-indent(out) << "boolean present_" << name << " = " << present << ";"
<< endl;
-indent(out) << "list.add(present_" << name << ");" << endl;
-indent(out) << "if (present_" << name << ")" << endl;
if (t->is_enum()) {
- indent(out) << " list.add(" << name << ".getValue());" << endl;
+ indent(out) << "hashCode = hashCode * " << MUL << " + " << name <<
".getValue();" << endl;
+} else if (t->is_base_type()) {
+ switch(((t_base_type*)t)->get_base()) {
+case t_base_type::TYPE_VOID:
+ break;
+case t_base_type::TYPE_STRING:
+ indent(out) << "hashCode = hashCode * " << MUL << " + " << name
<< ".hashCode();" << endl;
+ break;
+case t_base_type::TYPE_BOOL:
+ indent(out) << "hashCode = hashCode * " << MUL << " + ((" <<
name << ") ? "
+ << B_YES << " : " << B_NO << ");" << endl;
+ break;
+case t_base_type::TYPE_I8:
+ indent(out) << "hashCode = hashCode * " << MUL << " + (int) ("
<< name << ");" << endl;
+ break;
+case t_base_type::TYPE_I16:
+case t_base_type::TYPE_I32:
+ indent(out) << "hashCode = hashCode * " << MUL << " + " << name
<< ";" << endl;
+ break;
+case t_base_type::TYPE_I64:
+ indent(out) << "hashCode = hashCode * " << MUL << " +
org.apache.thrift.TBaseHelper.hashCode(" << name << ");" << endl;
+ break;
+case t_base_type::TYPE_DOUBLE:
+ indent(out) << "hashCode = hashCode * " << MUL << " +
org.apache.thrift.TBaseHelper.hashCode(" << name << ");" << endl;
+ break;
+default:
+ throw "compiler error: the following base type has no hashcode
generator: " +
+ t_base_type::t_base_name(((t_base_type*)t)->get_base());
--- End diff --
We can take this opportunity to stop throwing string here.
As I believe it should never happen (right ?), std::logic_error would be
appropriate.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-2877 Generate hashCode using primitive...
Github user nsuke commented on a diff in the pull request:
https://github.com/apache/thrift/pull/448#discussion_r52825698
--- Diff: compiler/cpp/src/generate/t_java_generator.cc ---
@@ -1886,24 +1889,57 @@ void
t_java_generator::generate_java_struct_equality(ofstream& out, t_struct* ts
bool can_be_null = type_can_be_null(t);
string name = (*m_iter)->get_name();
-string present = "true";
+if (is_optional || can_be_null) {
+ indent(out) << "hashCode = hashCode * " << MUL << " + ((" <<
generate_isset_check(*m_iter)
+ << ") ? " << B_YES << " : " << B_NO << ");" << endl;
+}
if (is_optional || can_be_null) {
- present += " && (" + generate_isset_check(*m_iter) + ")";
+ indent(out) << "if (" + generate_isset_check(*m_iter) + ")" << endl;
+ indent_up();
}
-indent(out) << "boolean present_" << name << " = " << present << ";"
<< endl;
-indent(out) << "list.add(present_" << name << ");" << endl;
-indent(out) << "if (present_" << name << ")" << endl;
if (t->is_enum()) {
- indent(out) << " list.add(" << name << ".getValue());" << endl;
+ indent(out) << "hashCode = hashCode * " << MUL << " + " << name <<
".getValue();" << endl;
+} else if (t->is_base_type()) {
+ switch(((t_base_type*)t)->get_base()) {
+case t_base_type::TYPE_VOID:
+ break;
--- End diff --
As this is a struct field, there should never be void ?
I you agree, let's put similar exception to the one for `default:` below.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-2877 Generate hashCode using primitive...
Github user nsuke commented on the pull request: https://github.com/apache/thrift/pull/448#issuecomment-183452201 @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 ? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request: THRIFT-2877 Generate hashCode using primitive...
Github user jsirois commented on the pull request: https://github.com/apache/thrift/pull/448#issuecomment-183458933 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](http://openjdk.java.net/projects/code-tools/jmh/). We use this in [Apache Aurora](https://github.com/apache/aurora/tree/master/src/jmh/java/org/apache/aurora/benchmark) for example to ensure we have a shared history of performance improvements and regressions. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request: THRIFT-2877 Generate hashCode using primitive...
Github user roshan commented on the pull request: https://github.com/apache/thrift/pull/448#issuecomment-183181973 Ha, thanks for that @nsuke . I only used the JDK 8 methods so we could keep `hashCode` identical in value to its current value. 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. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request: THRIFT-2877 Generate hashCode using primitive...
Github user nsuke commented on the pull request: https://github.com/apache/thrift/pull/448#issuecomment-182289601 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). --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request: THRIFT-2877 Generate hashCode using primitive...
GitHub user roshan reopened a pull request:
https://github.com/apache/thrift/pull/448
THRIFT-2877 Generate hashCode using primitives, static utility methods
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()](https://gist.github.com/roshan/7b7fff349e3ed06322c3).
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 https://github.com/apache/thrift/pull/447. I
rebased to one commit, but couldn't seem to reuse the old PR (it compiled
locally on gcc but failed on clang).
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/roshan/thrift THRIFT-2877_int_based_hashcode
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/448.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #448
commit b436a7ef56f5e6744d1004b284efa7b1d64b375b
Author: Roshan George ros...@arjie.com
Date: 2015-04-17T07:46:02Z
THRIFT-2877 Generate hashCode using primitives and static utility methods
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-2877 Generate hashCode using primitive...
Github user roshan closed the pull request at: https://github.com/apache/thrift/pull/447 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request: THRIFT-2877 Generate hashCode using primitive...
GitHub user roshan opened a pull request:
https://github.com/apache/thrift/pull/448
THRIFT-2877 Generate hashCode using primitives, static utility methods
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()](https://gist.github.com/roshan/7b7fff349e3ed06322c3).
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 https://github.com/apache/thrift/pull/447. I
rebased to one commit, but couldn't seem to reuse the old PR (it compiled
locally on gcc but failed on clang).
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/roshan/thrift THRIFT-2877_int_based_hashcode
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/448.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #448
commit b436a7ef56f5e6744d1004b284efa7b1d64b375b
Author: Roshan George ros...@arjie.com
Date: 2015-04-17T07:46:02Z
THRIFT-2877 Generate hashCode using primitives and static utility methods
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
