Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Const data class #51

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

SalomonBrys
Copy link

Proposal

Proposition: const data class would enable immutability guarantee of both structure and values of data classes, thus enabling cached hash code, which divides read time in HashSet / HashMap by 3.

@voddan
Copy link
Contributor

voddan commented Sep 27, 2016

Please redo the benchmarking using JMH.

It is very hard to reason about the correctness of your current benchmark: maybe you forgot a tiny detail which changes everything, maybe you didn't. Only a dev of level "hotspot jvm maintainer" could tell the difference ;)

@voddan
Copy link
Contributor

voddan commented Sep 27, 2016

Another alternative:

Change the algorithm for all data classes to check that:

  • no hashCode implementation is provided
  • all properties in the constructor are val
  • all properties are of
    • basic types: Int, Double, ets,String`,
    • known immutable types from JDK: BigInteger, ets?

Then implements a hashing function with hashing

@voddan
Copy link
Contributor

voddan commented Sep 27, 2016

This proposal seems to be also applicable to toString. Why not include it in the proposal?

@voddan
Copy link
Contributor

voddan commented Sep 27, 2016

About the alternative to implement @CachedHashcode:

A compiler-implemented annotation has an ability to check its applicant in detail. For instance it can check that the class is data and all properties are immutable. That way its functionality is not less than one of a keyword.

@IRus
Copy link

IRus commented Dec 10, 2016

How it will works with serialization? I see possible problems of transferring hashCode between JVMs.

@SalomonBrys
Copy link
Author

@voddan:

  • Concerning the benchmark: sorry, I don't know what JVMH is. Can you be more specific ?
  • I've updated the proposal to also cache toString ;)
  • Whether it's a keyword (such as const) or an annotation (such as @CachedHashcode) doesn't really matter. I've updated the proposal in that sense.
  • I like the auto-detection feature. I've updated the proposal ;) I think a keyword or annotation should still exist, though, to allow the programmer to force himself to enforce the limitations.

@IRus:

You're right. All cached values should be transient. I've updated the proposal.

@voddan
Copy link
Contributor

voddan commented Dec 10, 2016

@SalomonBrys JMH is the recommended way of benchmarking in JVM. http://openjdk.java.net/projects/code-tools/jmh/

@voddan
Copy link
Contributor

voddan commented Dec 10, 2016

@SalomonBrys auto detection will be inconsistent with the rest of Kotlin because it silently adds a field to a class. That may be critical when you optimize for small footprint.

@voddan
Copy link
Contributor

voddan commented Dec 10, 2016

What is the reasoning for making the cache @volatile?

@SalomonBrys
Copy link
Author

@voddan, You're right. I've moved the auto-detection paragraph to the "alternatives" section, and annotated it with your reserve.

The cached values should be volatile because multiple threads may access hashCode at the same time. If the hash code is not computed, this may lead to corruption.

I'll update the benchmark using JVMH next week, when I'll have access to my linux PC ;)

@voddan
Copy link
Contributor

voddan commented Dec 10, 2016

The cached values should be volatile because multiple threads may access hashCode at the same time. If the hash code is not computed, this may lead to corruption.

What corruption may that cause? The hash is an Int which has the same value even if computed on different threads. The worst case scenario is that 2 threads compute the hash simultaneously and override one another with the same value. IMO no synchronisation is needed.

This is important because volatile reads may have a significant overhead http://stackoverflow.com/a/12357342/3144601

@IRus
Copy link

IRus commented Dec 10, 2016

It's corner case, but on my jvm for 4294967297 number hashCode is 0, so your code will compute it every time)
Please add this case to tests too.

@sakno
Copy link

sakno commented Apr 11, 2018

const data class can be good investment in the future when value types will come in Java 10 (probably). At this moment, such class can be converted into correct value-based class as stated here

@sakno sakno mentioned this pull request Apr 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants