KEEP icon indicating copy to clipboard operation
KEEP copied to clipboard

Const data class

Open SalomonBrys opened this issue 9 years ago • 13 comments

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.

SalomonBrys avatar Sep 22 '16 15:09 SalomonBrys

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 avatar Sep 27 '16 11:09 voddan

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 avatar Sep 27 '16 11:09 voddan

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

voddan avatar Sep 27 '16 11:09 voddan

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.

voddan avatar Sep 27 '16 11:09 voddan

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

IRus avatar Dec 10 '16 06:12 IRus

@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.

SalomonBrys avatar Dec 10 '16 08:12 SalomonBrys

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

voddan avatar Dec 10 '16 08:12 voddan

@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 avatar Dec 10 '16 08:12 voddan

What is the reasoning for making the cache @volatile?

voddan avatar Dec 10 '16 08:12 voddan

@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 ;)

SalomonBrys avatar Dec 10 '16 09:12 SalomonBrys

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

voddan avatar Dec 10 '16 12:12 voddan

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.

IRus avatar Dec 10 '16 12:12 IRus

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 avatar Apr 11 '18 14:04 sakno