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

Automatic, internal primitive storage support for `Vector` #1557

Merged
merged 6 commits into from Oct 30, 2016

Conversation

Projects
None yet
4 participants
@paplorinc
Contributor

paplorinc commented Sep 4, 2016

This PR modifies the optimized Vector to store its data internally in the format it was created from, i.e.

Vector.ofAll(3, 1, 4, 1, 5); // stored internally as `int[]`

will create a Vector that has an int[] internally and

Vector.ofAll(Arrays.asList(3, 1, 4, 1, 5)); // boxed to `Integer[]` by `asList`

will have an Object[] inside.

The user doesn't have to know how it's stored internally (e.g. adding a null will automatically box everything in amortized constant time).

Internally many operations can work with primitive arrays without boxing (e.g. slice, iterate, filter, distinct), but others have to wrap them to make sense (e.g. flatMap).

Memory-wise storing primitives can be up to ~12× more memory efficient (e.g. 53,968 bytes for Vector compared to 653,672 bytes for ArrayList<Byte> (or 3,408,624 bytes for Functional Java's Seq<Byte>)).
Interestingly many operations are faster also (e.g. create and iterate), sometimes even faster than the Java counterpart - e.g. filter or map.

The benchmarks are as follows:

Operation Ratio                                                32     1024    32768
Create    slang_persistent/java_mutable                     1.51×    0.92×    1.17×
Create    slang_persistent/java_mutable_boxed              11.10×    8.64×    6.17×
Create    slang_persistent/java_mutable_boxed_stream       16.68×   10.77×    9.57×
Create    slang_persistent/fjava_persistent               229.91×  216.16×  192.30×
Create    slang_persistent/pcollections_persistent        126.90×  253.86×  375.34×
Create    slang_persistent/ecollections_persistent          2.05×    0.95×    1.18×
Create    slang_persistent/clojure_persistent               0.95×   15.40×   12.79×
Create    slang_persistent/scala_persistent                13.96×    9.32×    6.43×

Head      slang_persistent/java_mutable                     0.71×    0.53×    0.40×
Head      slang_persistent/fjava_persistent                 0.85×    0.66×    0.51×
Head      slang_persistent/pcollections_persistent          2.18×    3.75×    4.68×
Head      slang_persistent/ecollections_persistent          0.77×    0.61×    0.47×
Head      slang_persistent/clojure_persistent               0.70×    0.83×    0.90×
Head      slang_persistent/scala_persistent                 0.78×    0.63×    0.49×

Tail      slang_persistent/java_mutable                     0.71×    0.56×    0.39×
Tail      slang_persistent/fjava_persistent                52.84×   50.26×   52.36×
Tail      slang_persistent/pcollections_persistent          5.85×    9.15×   12.06×
Tail      slang_persistent/ecollections_persistent         16.88×  156.82× 4301.60×
Tail      slang_persistent/clojure_persistent               0.93×    0.69×    0.60×
Tail      slang_persistent/scala_persistent                 2.98×    4.87×    5.97×

Get       slang_persistent/java_mutable                     0.71×    0.12×    0.36×
Get       slang_persistent/fjava_persistent                55.92×   28.12×   79.81×
Get       slang_persistent/pcollections_persistent          8.32×    4.20×   17.46×
Get       slang_persistent/ecollections_persistent          0.71×    0.10×    0.34×
Get       slang_persistent/clojure_persistent               0.93×    0.37×    1.45×
Get       slang_persistent/scala_persistent                 1.13×    0.30×    0.95×

Update    slang_persistent/java_mutable                     0.11×    0.05×    0.03×
Update    slang_persistent/fjava_persistent               133.11×  262.66×  222.09×
Update    slang_persistent/pcollections_persistent          2.45×    4.30×    4.97×
Update    slang_persistent/ecollections_persistent         13.88×  162.43× 2589.05×
Update    slang_persistent/clojure_persistent               0.85×    1.23×    1.10×
Update    slang_persistent/scala_persistent                 1.03×    1.07×    1.14×

Map       slang_persistent/java_mutable                     2.19×    2.27×    2.00×
Map       slang_persistent/java_mutable_loop                0.53×    0.67×    0.54×
Map       slang_persistent/ecollections_persistent          1.16×    1.15×    1.06×
Map       slang_persistent/scala_persistent                 1.03×    1.43×    1.73×

Filter    slang_persistent/java_mutable                     1.97×    1.64×    1.15×
Filter    slang_persistent/ecollections_persistent          1.67×    1.14×    0.92×
Filter    slang_persistent/scala_persistent                 1.45×    1.44×    1.42×

Prepend   slang_persistent/java_mutable                     0.38×    0.71×   11.80×
Prepend   slang_persistent/fjava_persistent                 1.43×    1.62×    1.17×
Prepend   slang_persistent/pcollections_persistent          0.94×    2.47×    2.95×
Prepend   slang_persistent/ecollections_persistent          2.53×   27.04×  569.17×
Prepend   slang_persistent/clojure_persistent               5.90×  124.95× 3177.65×
Prepend   slang_persistent/scala_persistent                 0.22×    0.23×    0.15×

Append    slang_persistent/java_mutable                     0.25×    0.05×    0.03×
Append    slang_persistent/fjava_persistent                 5.18×    1.54×    1.02×
Append    slang_persistent/pcollections_persistent          2.71×    1.87×    2.22×
Append    slang_persistent/ecollections_persistent          7.93×   24.56×  560.25×
Append    slang_persistent/clojure_persistent               0.99×    0.23×    0.18×
Append    slang_persistent/scala_persistent                 0.81×    0.22×    0.16×

GroupBy   slang_persistent/java_mutable                     0.43×    0.65×    0.83×
GroupBy   slang_persistent/scala_persistent                 1.44×    1.05×    1.21×

Slice     slang_persistent/java_mutable                     0.54×    0.49×    0.45×
Slice     slang_persistent/pcollections_persistent          0.51×    0.42×    0.40×
Slice     slang_persistent/clojure_persistent               0.54×    0.45×    0.43×
Slice     slang_persistent/scala_persistent                 2.43×    5.61×    8.61×

Iterate   slang_persistent/java_mutable                     0.88×    0.45×    0.39×
Iterate   slang_persistent/fjava_persistent               368.25×  442.89×  289.54×
Iterate   slang_persistent/pcollections_persistent         14.61×   13.76×    9.42×
Iterate   slang_persistent/ecollections_persistent          2.35×    1.72×    1.52×
Iterate   slang_persistent/clojure_persistent               0.79×    1.03×    0.85×
Iterate   slang_persistent/scala_persistent                 1.53×    1.49×    0.90×

Note: Apparently Java 9 made some array copying optimizations (that made append slower than prepend in the first place), making update, append and groupBy slightly faster.

primitive:

Operation Ratio                                                32     1024    32768
Create    slang_persistent_int/java_mutable                 3.55×    1.71×    1.35×
Create    slang_persistent_int/java_mutable_boxed          26.16×   16.01×    7.09×
Create    slang_persistent_int/java_mutable_boxed_stream   39.31×   19.96×   11.01×
Create    slang_persistent_int/fjava_persistent           542.00×  400.65×  221.11×
Create    slang_persistent_int/pcollections_persistent    299.17×  470.54×  431.57×
Create    slang_persistent_int/ecollections_persistent      4.84×    1.76×    1.36×
Create    slang_persistent_int/clojure_persistent           2.23×   28.55×   14.71×
Create    slang_persistent_int/scala_persistent            32.91×   17.27×    7.39×

Tail      slang_persistent_int/java_mutable                 0.71×    0.56×    0.45×
Tail      slang_persistent_int/fjava_persistent            52.55×   50.20×   59.62×
Tail      slang_persistent_int/pcollections_persistent      5.82×    9.14×   13.73×
Tail      slang_persistent_int/ecollections_persistent     16.79×  156.62× 4898.01×
Tail      slang_persistent_int/clojure_persistent           0.92×    0.69×    0.68×
Tail      slang_persistent_int/scala_persistent             2.97×    4.87×    6.80×

Update    slang_persistent_int/java_mutable                 0.11×    0.05×    0.03×
Update    slang_persistent_int/fjava_persistent           124.86×  254.21×  215.66×
Update    slang_persistent_int/pcollections_persistent      2.30×    4.16×    4.83×
Update    slang_persistent_int/ecollections_persistent     13.02×  157.21× 2514.05×
Update    slang_persistent_int/clojure_persistent           0.80×    1.19×    1.07×
Update    slang_persistent_int/scala_persistent             0.97×    1.04×    1.11×

Map       slang_persistent_int/java_mutable                 2.01×    1.85×    1.58×
Map       slang_persistent_int/java_mutable_loop            0.48×    0.54×    0.43×
Map       slang_persistent_int/ecollections_persistent      1.07×    0.93×    0.84×
Map       slang_persistent_int/scala_persistent             0.94×    1.16×    1.37×

Filter    slang_persistent_int/java_mutable                 2.35×    1.63×    0.87×
Filter    slang_persistent_int/ecollections_persistent      2.00×    1.13×    0.70×
Filter    slang_persistent_int/scala_persistent             1.73×    1.43×    1.07×

Prepend   slang_persistent_int/java_mutable                 0.84×    0.78×   12.69×
Prepend   slang_persistent_int/fjava_persistent             3.12×    1.78×    1.25×
Prepend   slang_persistent_int/pcollections_persistent      2.05×    2.72×    3.18×
Prepend   slang_persistent_int/ecollections_persistent      5.50×   29.76×  612.04×
Prepend   slang_persistent_int/clojure_persistent          12.83×  137.53× 3416.94×
Prepend   slang_persistent_int/scala_persistent             0.49×    0.26×    0.17×

Append    slang_persistent_int/java_mutable                 0.23×    0.11×    0.03×
Append    slang_persistent_int/fjava_persistent             4.66×    3.00×    1.15×
Append    slang_persistent_int/pcollections_persistent      2.43×    3.64×    2.50×
Append    slang_persistent_int/ecollections_persistent      7.13×   47.83×  631.27×
Append    slang_persistent_int/clojure_persistent           0.89×    0.46×    0.20×
Append    slang_persistent_int/scala_persistent             0.73×    0.42×    0.18×

Slice     slang_persistent_int/java_mutable                 0.56×    0.47×    0.38×
Slice     slang_persistent_int/pcollections_persistent      0.52×    0.40×    0.34×
Slice     slang_persistent_int/clojure_persistent           0.55×    0.44×    0.37×
Slice     slang_persistent_int/scala_persistent             2.50×    5.42×    7.28×

Iterate   slang_persistent_int/java_mutable                 1.76×    0.88×    1.38×
Iterate   slang_persistent_int/fjava_persistent           737.64×  858.07× 1036.08×
Iterate   slang_persistent_int/pcollections_persistent     29.26×   26.65×   33.69×
Iterate   slang_persistent_int/ecollections_persistent      4.71×    3.33×    5.44×
Iterate   slang_persistent_int/clojure_persistent           1.58×    2.01×    3.05×
Iterate   slang_persistent_int/scala_persistent             3.06×    2.89×    3.23×

screenshot from 2016-09-10 14 20 50

and memory usages:

for `32` elements
  Java mutable @ ArrayList504 bytes
  Functional Java persistent @ Seq3,064 bytes
  PCollections persistent @ TreePVector1,712 bytes
  Eclipse Collections persistent @ ImmutableArrayList496 bytes
  Clojure persistent @ PersistentVector704 bytes
  Scala persistent @ Vector536 bytes
  Javaslang persistent @ Vector (Integer[])           →   544 bytes
  Javaslang persistent @ Vector (int[])               →   208 bytes
  Javaslang persistent @ Vector (byte[])              →   112 bytes

for `1024` elements
  Java mutable @ ArrayList19,064 bytes
  Functional Java persistent @ Seq104,760 bytes
  PCollections persistent @ TreePVector55,984 bytes
  Eclipse Collections persistent @ ImmutableArrayList19,056 bytes
  Clojure persistent @ PersistentVector20,504 bytes
  Scala persistent @ Vector19,736 bytes
  Javaslang persistent @ Vector (Integer[])           →  19,744 bytes
  Javaslang persistent @ Vector (int[])               →   4,816 bytes
  Javaslang persistent @ Vector (byte[])              →   1,744 bytes

for `32768` elements
  Java mutable @ ArrayList653,672 bytes
  Functional Java persistent @ Seq3,408,624 bytes
  PCollections persistent @ TreePVector1,833,376 bytes
  Eclipse Collections persistent @ ImmutableArrayList653,664 bytes
  Clojure persistent @ PersistentVector700,168 bytes
  Scala persistent @ Vector674,824 bytes
  Javaslang persistent @ Vector (Integer[])           →   674,832 bytes
  Javaslang persistent @ Vector (int[])               →   152,272 bytes
  Javaslang persistent @ Vector (byte[])              →    53,968 bytes

i.e. storing bytes in functional java would require ~63× more storage.

@paplorinc paplorinc changed the title from Added automatic, internal primitive storage support for `Vector` to Automatic, internal primitive storage support for `Vector` Sep 4, 2016

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Sep 4, 2016

Current coverage is 96.44% (diff: 95.22%)

Merging #1557 into master will decrease coverage by 0.04%

@@             master      #1557   diff @@
==========================================
  Files            89         98     +9   
  Lines         10940      11100   +160   
  Methods           0          0          
  Messages          0          0          
  Branches       1850       1884    +34   
==========================================
+ Hits          10556      10705   +149   
- Misses          240        242     +2   
- Partials        144        153     +9   

Powered by Codecov. Last update 120f85f...6b4dc86

codecov-io commented Sep 4, 2016

Current coverage is 96.44% (diff: 95.22%)

Merging #1557 into master will decrease coverage by 0.04%

@@             master      #1557   diff @@
==========================================
  Files            89         98     +9   
  Lines         10940      11100   +160   
  Methods           0          0          
  Messages          0          0          
  Branches       1850       1884    +34   
==========================================
+ Hits          10556      10705   +149   
- Misses          240        242     +2   
- Partials        144        153     +9   

Powered by Codecov. Last update 120f85f...6b4dc86

@danieldietrich danieldietrich added this to the 2.1.0 milestone Sep 4, 2016

@danieldietrich

This comment has been minimized.

Show comment
Hide comment
@danieldietrich

danieldietrich Sep 4, 2016

Member

Wow - this is a really interesting change - The Javaslang way. Automatically switching to primitive representations, I think the Java world hasn't seen that to date, right?

Will review all open Vector PRs today...

Member

danieldietrich commented Sep 4, 2016

Wow - this is a really interesting change - The Javaslang way. Automatically switching to primitive representations, I think the Java world hasn't seen that to date, right?

Will review all open Vector PRs today...

Show outdated Hide outdated javaslang-benchmark/src/test/java/javaslang/collection/VectorBenchmark.java
// Append.class,
GroupBy.class
// Slice.class,
// Iterate.class

This comment has been minimized.

@danieldietrich

danieldietrich Sep 4, 2016

Member

Were these accidentally committed?

@danieldietrich

danieldietrich Sep 4, 2016

Member

Were these accidentally committed?

This comment has been minimized.

@paplorinc

paplorinc Sep 4, 2016

Contributor

fixed, thanks

@paplorinc

paplorinc Sep 4, 2016

Contributor

fixed, thanks

Show outdated Hide outdated javaslang/src/main/java/javaslang/collection/BitMappedTrie.java
return (T[]) array;
} else if (depthShift == BRANCHING_BASE) {
return getAt(array, firstDigit(offset + index, depthShift));
} else {

This comment has been minimized.

@danieldietrich

danieldietrich Sep 4, 2016

Member

The else branch covers depthShift > 0 && (depthShift < BRANCHING_BASE || depthShift > BRANCHING_BASE), right?

Just asking to get sure that depthShift can be less than BRANCHING_BASE...

@danieldietrich

danieldietrich Sep 4, 2016

Member

The else branch covers depthShift > 0 && (depthShift < BRANCHING_BASE || depthShift > BRANCHING_BASE), right?

Just asking to get sure that depthShift can be less than BRANCHING_BASE...

Show outdated Hide outdated javaslang/src/main/java/javaslang/collection/LeafType.java
Class<T> type() { return type; }
@Override
Object empty() { return java.lang.reflect.Array.newInstance(type, 0); }

This comment has been minimized.

@ruslansennov

ruslansennov Sep 6, 2016

Member

java.lang.reflect.Array is GWT-incompatible

@ruslansennov

ruslansennov Sep 6, 2016

Member

java.lang.reflect.Array is GWT-incompatible

This comment has been minimized.

@paplorinc

paplorinc Sep 6, 2016

Contributor

This is a backup plan (i.e. we could spell out every primitive), though I'm not even sure whether this would work/is necessary in Javascipt.

@ruslansennov, what are your thoughts on the rest of the PRs?

@paplorinc

paplorinc Sep 6, 2016

Contributor

This is a backup plan (i.e. we could spell out every primitive), though I'm not even sure whether this would work/is necessary in Javascipt.

@ruslansennov, what are your thoughts on the rest of the PRs?

This comment has been minimized.

@ruslansennov

ruslansennov Sep 6, 2016

Member

This is a backup plan (i.e. we could spell out every primitive), though I'm not even sure whether this would work/is necessary in Javascipt.

It seems you should create a lot of implementations like IntArray?

what are your thoughts on the rest of the PRs?

I'm very sorry, I can't find a few consecutive hours for review your previous PR :(
I still doubt in the effectiveness of tail and head arrays in the Vector (because of significantly code complication), but benchmarks are more than convincing :)

@ruslansennov

ruslansennov Sep 6, 2016

Member

This is a backup plan (i.e. we could spell out every primitive), though I'm not even sure whether this would work/is necessary in Javascipt.

It seems you should create a lot of implementations like IntArray?

what are your thoughts on the rest of the PRs?

I'm very sorry, I can't find a few consecutive hours for review your previous PR :(
I still doubt in the effectiveness of tail and head arrays in the Vector (because of significantly code complication), but benchmarks are more than convincing :)

Show outdated Hide outdated javaslang/src/main/java/javaslang/collection/Vector.java
@@ -914,7 +914,7 @@ public int lastIndexOf(T element, int end) {
if (ordinal == 0) {
newLeading.val = copy;
} else if (ordinal <= newMiddle.length) {
newMiddle[ordinal - 1] = copy;
newMiddle[ordinal - 1] = copy);

This comment has been minimized.

@danieldietrich

danieldietrich Oct 20, 2016

Member

Huh, where does this ')' come from? Does it really compile!?

@danieldietrich

danieldietrich Oct 20, 2016

Member

Huh, where does this ')' come from? Does it really compile!?

@danieldietrich

This comment has been minimized.

Show comment
Hide comment
@danieldietrich

danieldietrich Oct 20, 2016

Member

@paplorinc

I see your effort and the improvements, thank you! The implementation is very low level and got more complicated because of the heading and trailing arrays. But Vector will be very useful as general purpose collection.

Therefore I will merge this change. Please resolve the conflicts and I will pull it in.

Do I also have to pull in "Vector optimizations #1522" or does this PR also contain all changes of #1522?

Member

danieldietrich commented Oct 20, 2016

@paplorinc

I see your effort and the improvements, thank you! The implementation is very low level and got more complicated because of the heading and trailing arrays. But Vector will be very useful as general purpose collection.

Therefore I will merge this change. Please resolve the conflicts and I will pull it in.

Do I also have to pull in "Vector optimizations #1522" or does this PR also contain all changes of #1522?

@danieldietrich

This comment has been minimized.

Show comment
Hide comment
@danieldietrich

danieldietrich Oct 20, 2016

Member

What about the reflection stuff? Will Vector work with GWT?
If not, we should discuss how to proceed further.

Member

danieldietrich commented Oct 20, 2016

What about the reflection stuff? Will Vector work with GWT?
If not, we should discuss how to proceed further.

@paplorinc

This comment has been minimized.

Show comment
Hide comment
@paplorinc

paplorinc Oct 21, 2016

Contributor

Will do my best to make it GWT compliant also, but I might need help from @ruslansennov, as currently the GWT tests are failing and I'm not sure how to decipher the error message :/.

Contributor

paplorinc commented Oct 21, 2016

Will do my best to make it GWT compliant also, but I might need help from @ruslansennov, as currently the GWT tests are failing and I'm not sure how to decipher the error message :/.

@danieldietrich

This comment has been minimized.

Show comment
Hide comment
@danieldietrich

danieldietrich Oct 21, 2016

Member

@paplorinc it is important that GWT follows Javaslang core, not the other direction.
We write efficient code. If we can omit GWT incompatible API, we will to it. But we do not write slower/worse code in order to make GWT work. Just saying.
Please make the GWT-dpecific changes in a separate commit for better review-experience.

Member

danieldietrich commented Oct 21, 2016

@paplorinc it is important that GWT follows Javaslang core, not the other direction.
We write efficient code. If we can omit GWT incompatible API, we will to it. But we do not write slower/worse code in order to make GWT work. Just saying.
Please make the GWT-dpecific changes in a separate commit for better review-experience.

@paplorinc

This comment has been minimized.

Show comment
Hide comment
@paplorinc

paplorinc Oct 22, 2016

Contributor

@danieldietrich, will rebase, once #1522 is merged :)

Contributor

paplorinc commented Oct 22, 2016

@danieldietrich, will rebase, once #1522 is merged :)

@paplorinc

This comment has been minimized.

Show comment
Hide comment
@paplorinc

paplorinc Oct 22, 2016

Contributor

Pushed, but GWT complains again. I'm not sure how to fix those errors, any help?

Contributor

paplorinc commented Oct 22, 2016

Pushed, but GWT complains again. I'm not sure how to fix those errors, any help?

@danieldietrich

Hi,

I also can't get the tests get to work on my machine due to GWT errors... :-/ Will try harder tomorrow...

I cloned your fork and applied some minor changes. Because I don't know how to do a PR to your fork, I created a Gist with the changes...

Please replace your Vector with this: https://gist.github.com/danieldietrich/fb3ab33c4db54cd67e272bc95befd37b

Please delete javaslang.collection.LeafType. I moved it to VectorModule because it is currently only used by Vector. If it used later by other types we can make it package-private available.

Also I removed dead code: LeafType.getAt(...) and LeafType.isPrimitive() weren't used.

Thanks,

Daniel

@paplorinc

This comment has been minimized.

Show comment
Hide comment
@paplorinc

paplorinc Oct 23, 2016

Contributor

Also I removed dead code: LeafType.getAt(...) and LeafType.isPrimitive() weren't used.

Not sure what you mean by dead code, every method is used and almost every line is tested (except for some redundant boundary checks in BMT, that were already checked in Vector):

Element         Class       Method          Line
BitMappedTrie   100% (2/2)  100% (33/33)    97%  (152/156)
LeafType        100% (1/1)  100% (17/17)    100% (52/52)
Vector          100% (1/1)  100% (130/130)  100% (329/329)

I moved it to VectorModule because it is currently only used by Vector.

It's actually also used by BitMappedTrie and VectorBenchmark.
Should I really merge Vector with BitMappedTrie and LeafType (they're only meant to complement Vector)?
It would result in a huge, ~1900 line behemoth, that would contain multiple, separate responsibilities.

If you think this change is too complex, I don't mind separating it into more commits/files, but I need to know what is hard to understand. :)
I can also add more documentation or explain it here, just let me know where the difficulty is.

Contributor

paplorinc commented Oct 23, 2016

Also I removed dead code: LeafType.getAt(...) and LeafType.isPrimitive() weren't used.

Not sure what you mean by dead code, every method is used and almost every line is tested (except for some redundant boundary checks in BMT, that were already checked in Vector):

Element         Class       Method          Line
BitMappedTrie   100% (2/2)  100% (33/33)    97%  (152/156)
LeafType        100% (1/1)  100% (17/17)    100% (52/52)
Vector          100% (1/1)  100% (130/130)  100% (329/329)

I moved it to VectorModule because it is currently only used by Vector.

It's actually also used by BitMappedTrie and VectorBenchmark.
Should I really merge Vector with BitMappedTrie and LeafType (they're only meant to complement Vector)?
It would result in a huge, ~1900 line behemoth, that would contain multiple, separate responsibilities.

If you think this change is too complex, I don't mind separating it into more commits/files, but I need to know what is hard to understand. :)
I can also add more documentation or explain it here, just let me know where the difficulty is.

Show outdated Hide outdated javaslang/src/main/java/javaslang/collection/LeafType.java
@@ -12,7 +12,7 @@
* @author Pap Lőrinc
* @since 2.1.0
*/
final class Arrays {
final class LeafType {

This comment has been minimized.

@paplorinc

paplorinc Oct 23, 2016

Contributor

Until now this was a stateless helper for manipulating Object[] arrays.
Now every primitive type will get a different LeafType instance, that helps in manipulating the given array in a generic way (getting/setting elements, creating new primitive arrays, etc).

@paplorinc

paplorinc Oct 23, 2016

Contributor

Until now this was a stateless helper for manipulating Object[] arrays.
Now every primitive type will get a different LeafType instance, that helps in manipulating the given array in a generic way (getting/setting elements, creating new primitive arrays, etc).

@@ -41,32 +41,32 @@
@SuppressWarnings("unchecked")
static <T> BitMappedTrie<T> empty() { return (BitMappedTrie<T>) EMPTY; }
private final Object[] array;

This comment has been minimized.

@paplorinc

paplorinc Oct 23, 2016

Contributor

intermediary commit, that removes all Object[] references, changing them to Objects and accessing them in a single place using getAt, setAt and lengthOf, so that these methods can be specialized in the next commit to every primitive.

@paplorinc

paplorinc Oct 23, 2016

Contributor

intermediary commit, that removes all Object[] references, changing them to Objects and accessing them in a single place using getAt, setAt and lengthOf, so that these methods can be specialized in the next commit to every primitive.

Show outdated Hide outdated javaslang/src/main/java/javaslang/collection/BitMappedTrie.java
array[previousIndex] = newNode;
final Object previous = getAt(array, previousIndex);
final Object newNode = node.apply(previous, offset);
setAt(array, previousIndex, newNode);

This comment has been minimized.

@paplorinc

paplorinc Oct 23, 2016

Contributor

since we have Objects everywhere, array notation cannot be used anymore, since the Object[] type cannot refer to primitive arrays, but Object can

@paplorinc

paplorinc Oct 23, 2016

Contributor

since we have Objects everywhere, array notation cannot be used anymore, since the Object[] type cannot refer to primitive arrays, but Object can

@@ -64,11 +65,14 @@ public static void main(String... args) {
clojure.lang.PersistentVector clojurePersistent;
scala.collection.immutable.Vector<Integer> scalaPersistent;
javaslang.collection.Vector<Integer> slangPersistent;
javaslang.collection.Vector<Integer> slangPersistentInt;
javaslang.collection.Vector<Byte> slangPersistentByte;

This comment has been minimized.

@paplorinc

paplorinc Oct 23, 2016

Contributor

Benchmark primitive storage, too.
Speed is only compared against int[], but memory is measured for byte[] also.

@paplorinc

paplorinc Oct 23, 2016

Contributor

Benchmark primitive storage, too.
Speed is only compared against int[], but memory is measured for byte[] also.

@Benchmark
public Object slang_persistent_int() {
final javaslang.collection.Vector<Integer> values = javaslang.collection.Vector.ofAll(INT_ELEMENTS);
assert (values.trie.type.type() == int.class) && areEqual(values, javaMutable);

This comment has been minimized.

@paplorinc

paplorinc Oct 23, 2016

Contributor

these asserts also make sure that we actually have int[] inside

@paplorinc

paplorinc Oct 23, 2016

Contributor

these asserts also make sure that we actually have int[] inside

final int newSize = length() + 1;
if (length() == 0) {
return new BitMappedTrie<>(asArray(leading), offset, newSize, depthShift);
if (cannotAdd(leading)) {

This comment has been minimized.

@paplorinc

paplorinc Oct 23, 2016

Contributor

if we can't add an element to the Vector (i.e. null or String to a primitive collection), copy everything once, and add it to that instead.

@paplorinc

paplorinc Oct 23, 2016

Contributor

if we can't add an element to the Vector (i.e. null or String to a primitive collection), copy everything once, and add it to that instead.

Show outdated Hide outdated javaslang/src/test/java/javaslang/collection/VectorPropertyTest.java
@@ -29,6 +29,20 @@ public void shouldCreateAndGet() {
for (int j = 0; j < actual.size(); j++) {
assertThat(expected.get(j)).isEqualTo(actual.get(j));
}
final Vector<Integer> actualInt = Vector.ofAll(LeafType.<int[]> asPrimitives(int.class, expected));

This comment has been minimized.

@paplorinc

paplorinc Oct 23, 2016

Contributor

create primitive int[] array and test that it contains the correct elements and that it's stored as int[], not Object[]

@paplorinc

paplorinc Oct 23, 2016

Contributor

create primitive int[] array and test that it contains the correct elements and that it's stored as int[], not Object[]

Show outdated Hide outdated javaslang/src/test/java/javaslang/collection/VectorPropertyTest.java
assert (i == 0) || (actualInt.trie.type.type() == int.class);
assertAreEqual(expected, actualInt);
final Seq<Character> expectedChar = expected.map(v -> (char) v.intValue());

This comment has been minimized.

@paplorinc

paplorinc Oct 23, 2016

Contributor

same for char[], which doesn't have an optimized LeafType yet, just a generic one via reflection (currently only int[] arrays are optimized speed-wise, the rest are all delegated to java.lang.reflect.Array).
Each primitive can be optimized on demand, as done in IntArray.
@danieldietrich, would you like me to add other primitive speed optimizations in this commit also, or is int[] enough for now (the rest are also stored as primitives, but their usage is slower because of the reflective call)

@paplorinc

paplorinc Oct 23, 2016

Contributor

same for char[], which doesn't have an optimized LeafType yet, just a generic one via reflection (currently only int[] arrays are optimized speed-wise, the rest are all delegated to java.lang.reflect.Array).
Each primitive can be optimized on demand, as done in IntArray.
@danieldietrich, would you like me to add other primitive speed optimizations in this commit also, or is int[] enough for now (the rest are also stored as primitives, but their usage is slower because of the reflective call)

Show outdated Hide outdated javaslang/src/test/java/javaslang/collection/VectorPropertyTest.java
for (int drop = 0; drop <= (BRANCHING_FACTOR + 1); drop++) {
final Iterator<Integer> expectedIterator = expected.iterator();
actual.trie.<Object[]> visit((index, leaf, start, end) -> {
actual.trie.<int[]> visit((index, leaf, start, end) -> {
for (int i = start; i < end; i++) {
assertThat(leaf[i]).isEqualTo(expectedIterator.next());

This comment has been minimized.

@paplorinc

paplorinc Oct 23, 2016

Contributor

this is the way to iterate over a primitive-enabled Vector, without any boxing/unboxing

@paplorinc

paplorinc Oct 23, 2016

Contributor

this is the way to iterate over a primitive-enabled Vector, without any boxing/unboxing

Show outdated Hide outdated javaslang/src/main/java/javaslang/collection/Vector.java
}
public static Vector<Character> range(char from, char toExclusive) {
return ofAll(Iterator.range(from, toExclusive));
return ofAll(LeafType.<char[]> asPrimitives(char.class, Iterator.range(from, toExclusive)));

This comment has been minimized.

@paplorinc

paplorinc Oct 23, 2016

Contributor

ranges should be stored in space-and-iteration-friendly primitive arrays <3

@paplorinc

paplorinc Oct 23, 2016

Contributor

ranges should be stored in space-and-iteration-friendly primitive arrays <3

Show outdated Hide outdated javaslang/src/main/java/javaslang/collection/LeafType.java
@SuppressWarnings("unchecked")
@GwtIncompatible
final class PrimitiveArray<T> extends LeafType<T> {

This comment has been minimized.

@paplorinc

paplorinc Oct 23, 2016

Contributor

generic converter for non-optimized array access (only needed if we don't privide converters for every primitive)

@paplorinc

paplorinc Oct 23, 2016

Contributor

generic converter for non-optimized array access (only needed if we don't privide converters for every primitive)

This comment has been minimized.

@danieldietrich

danieldietrich Oct 23, 2016

Member

@paplorinc I've slept over the topic. What about generating the converters for every primitive type?
I would rename LeafType to PrimitiveArray and let IntArray, CharArray, ... extend it. All these types reside in a package-private final class Arrays.
I think we could reuse these types also in other collections, starting with Array :)

@danieldietrich

danieldietrich Oct 23, 2016

Member

@paplorinc I've slept over the topic. What about generating the converters for every primitive type?
I would rename LeafType to PrimitiveArray and let IntArray, CharArray, ... extend it. All these types reside in a package-private final class Arrays.
I think we could reuse these types also in other collections, starting with Array :)

This comment has been minimized.

@paplorinc

paplorinc Oct 23, 2016

Contributor

Interesting idea, I like it, I will try to come up with such a construct :)

@paplorinc

paplorinc Oct 23, 2016

Contributor

Interesting idea, I like it, I will try to come up with such a construct :)

This comment has been minimized.

@paplorinc

paplorinc Oct 23, 2016

Contributor

@danieldietrich, should I put these in a separate module, i.e. javaslang-primitive?

@paplorinc

paplorinc Oct 23, 2016

Contributor

@danieldietrich, should I put these in a separate module, i.e. javaslang-primitive?

This comment has been minimized.

@danieldietrich

danieldietrich Oct 23, 2016

Member

@paplorinc for now I see no reason to do that. the types should be package-private available for collection for now. Arrays are somehow collections. It is sufficient if our Javaslang collections use the primitive arrays. Our Array will abstract over all of the primitve + object arrays. It is a sell-point for our Array to not give them to the outside.

@danieldietrich

danieldietrich Oct 23, 2016

Member

@paplorinc for now I see no reason to do that. the types should be package-private available for collection for now. Arrays are somehow collections. It is sufficient if our Javaslang collections use the primitive arrays. Our Array will abstract over all of the primitve + object arrays. It is a sell-point for our Array to not give them to the outside.

This comment has been minimized.

@danieldietrich

danieldietrich Oct 23, 2016

Member

Btw - I plan to re-engineer/split the code generator in a near future. It will split into:

  • GeneratorCore.scala
  • GeneratorJava.scala (depends on GeneratorCore)

And a separate generator for each task (which depend on GeneratorJava), e.g.

  • TupleGenerator.scala
  • FunctionGenerator.scala
  • ...

Our Generator currently gets bigger and bigger. Maven can only run Scala scripts (without imports) before the compile-phase. I hope that Gradle will make it possible to split our Generator.scala.

@danieldietrich

danieldietrich Oct 23, 2016

Member

Btw - I plan to re-engineer/split the code generator in a near future. It will split into:

  • GeneratorCore.scala
  • GeneratorJava.scala (depends on GeneratorCore)

And a separate generator for each task (which depend on GeneratorJava), e.g.

  • TupleGenerator.scala
  • FunctionGenerator.scala
  • ...

Our Generator currently gets bigger and bigger. Maven can only run Scala scripts (without imports) before the compile-phase. I hope that Gradle will make it possible to split our Generator.scala.

Show outdated Hide outdated javaslang/src/main/java/javaslang/collection/LeafType.java
static void setAt(Object array, int index, Object value) { ((Object[]) array)[index] = value; }
static int lengthOf(Object array) {
return ((Object[]) array).length;
final class IntArray extends LeafType<Integer> {

This comment has been minimized.

@paplorinc

paplorinc Oct 23, 2016

Contributor

ints are used very often, let's optimize their access (storage is the same in all cases)

@paplorinc

paplorinc Oct 23, 2016

Contributor

ints are used very often, let's optimize their access (storage is the same in all cases)

Show outdated Hide outdated javaslang/src/main/java/javaslang/collection/LeafType.java
}
}
final class ObjectArray extends LeafType<Object> {

This comment has been minimized.

@paplorinc

paplorinc Oct 23, 2016

Contributor

general Object[] array access (including e.g. Integer[])

@paplorinc

paplorinc Oct 23, 2016

Contributor

general Object[] array access (including e.g. Integer[])

@paplorinc

This comment has been minimized.

Show comment
Hide comment
@paplorinc

paplorinc Oct 23, 2016

Contributor

@danieldietrich, @zsolt-donca, added comments throughout the codebase to help in reviewing.

Contributor

paplorinc commented Oct 23, 2016

@danieldietrich, @zsolt-donca, added comments throughout the codebase to help in reviewing.

@danieldietrich

This comment has been minimized.

Show comment
Hide comment
@danieldietrich

danieldietrich Oct 23, 2016

Member

No, please do not change the implementation. IntelliJ showed a 'false positive'. The editor stated that these methods are not used.

I need some more time to review.

I see that from the performance perspective the code is fast. But on the abstraction level we did not find the right way yet I think. Future and Promise are a good example. I looked at the Scala code of Viktor Klang. There is a very small core where synchronization logic takes place (in Javaslang it is FutureImpl). It is like an onion. This core is very small. The outer layers don't know anything about the core other than it's public interface (take a look at Future). So it is ensured that no synchronization logic leaks to the outside. It is all very elegant and compositional. Some of the best code I've ever seen.

Our Vector works different. It is so highly optimized that the logic of the BMT leaks to the outside. I'm also not sure if Vector should know about the Leaf types. For reasons of stability and maintainability of the library this is really evil. The complexity increases (more than linear). I'm not sure if I'm able to maintain it or to add new functionality.

I need some additional time to look over the code. But for now I see no reason to change anything.

Member

danieldietrich commented Oct 23, 2016

No, please do not change the implementation. IntelliJ showed a 'false positive'. The editor stated that these methods are not used.

I need some more time to review.

I see that from the performance perspective the code is fast. But on the abstraction level we did not find the right way yet I think. Future and Promise are a good example. I looked at the Scala code of Viktor Klang. There is a very small core where synchronization logic takes place (in Javaslang it is FutureImpl). It is like an onion. This core is very small. The outer layers don't know anything about the core other than it's public interface (take a look at Future). So it is ensured that no synchronization logic leaks to the outside. It is all very elegant and compositional. Some of the best code I've ever seen.

Our Vector works different. It is so highly optimized that the logic of the BMT leaks to the outside. I'm also not sure if Vector should know about the Leaf types. For reasons of stability and maintainability of the library this is really evil. The complexity increases (more than linear). I'm not sure if I'm able to maintain it or to add new functionality.

I need some additional time to look over the code. But for now I see no reason to change anything.

@danieldietrich

This comment has been minimized.

Show comment
Hide comment
@danieldietrich

danieldietrich Oct 23, 2016

Member

Btw - I intentionally did no special handling of primitives throughout Javaslang. It was a design decision. I did not want to fix the 'errors' of the Java language. With Project Valhalla we probably get primitive generics. No boxing will take place any more. Then the 'problem' will disappear automatically.

Member

danieldietrich commented Oct 23, 2016

Btw - I intentionally did no special handling of primitives throughout Javaslang. It was a design decision. I did not want to fix the 'errors' of the Java language. With Project Valhalla we probably get primitive generics. No boxing will take place any more. Then the 'problem' will disappear automatically.

@paplorinc

This comment has been minimized.

Show comment
Hide comment
@paplorinc

paplorinc Oct 23, 2016

Contributor

It is so highly optimized that the logic of the BMT leaks to the outside.

I don't think it's fair to compare it against something conceptually as simple as a Future, but if you think the impl is still leaking, please advise on how to make it better, simpler!
Vector basically just delegates now, there is no BMT logic there.

I did not want to fix the 'errors' of the Java language.

I think this whole library's sole purpose is exactly that, to fix some of Java's shortcomings.

With Project Valhalla we probably get primitive generics.

JDK 9 just got delayed again to mid 2017, version 10 probably won't be widespread enough for another ~5 years. And even when it will be, will Javalang simply deprecate Java 8 & 9 support (it probably won't be fully backwards compatible)?
I think people need a memory efficient, fast, random access, persistent collection now.

Contributor

paplorinc commented Oct 23, 2016

It is so highly optimized that the logic of the BMT leaks to the outside.

I don't think it's fair to compare it against something conceptually as simple as a Future, but if you think the impl is still leaking, please advise on how to make it better, simpler!
Vector basically just delegates now, there is no BMT logic there.

I did not want to fix the 'errors' of the Java language.

I think this whole library's sole purpose is exactly that, to fix some of Java's shortcomings.

With Project Valhalla we probably get primitive generics.

JDK 9 just got delayed again to mid 2017, version 10 probably won't be widespread enough for another ~5 years. And even when it will be, will Javalang simply deprecate Java 8 & 9 support (it probably won't be fully backwards compatible)?
I think people need a memory efficient, fast, random access, persistent collection now.

@danieldietrich

This comment has been minimized.

Show comment
Hide comment
@danieldietrich

danieldietrich Oct 23, 2016

Member

@paplorinc you are right. If I think there is sth leaking, I will provide a better solution. (Will be offline today most of the time...)

Member

danieldietrich commented Oct 23, 2016

@paplorinc you are right. If I think there is sth leaking, I will provide a better solution. (Will be offline today most of the time...)

"int" -> "Integer",
"long" -> "Long",
"short" -> "Short"
) // note: there is no void[] in Java

This comment has been minimized.

@paplorinc

paplorinc Oct 23, 2016

Contributor

funny, I didn't know that :)

@paplorinc

paplorinc Oct 23, 2016

Contributor

funny, I didn't know that :)

@paplorinc

This comment has been minimized.

Show comment
Hide comment
@paplorinc

paplorinc Oct 23, 2016

Contributor

@danieldietrich, pushed version with exhaustive primitive array helper generation - making it GWT compatible also, as it doesn't need reflection anymore.

Contributor

paplorinc commented Oct 23, 2016

@danieldietrich, pushed version with exhaustive primitive array helper generation - making it GWT compatible also, as it doesn't need reflection anymore.

@danieldietrich

This comment has been minimized.

Show comment
Hide comment
@danieldietrich

danieldietrich Oct 23, 2016

Member

Will check the code tomorrow!

Member

danieldietrich commented Oct 23, 2016

Will check the code tomorrow!

@paplorinc

This comment has been minimized.

Show comment
Hide comment
@paplorinc

paplorinc Oct 29, 2016

Contributor

@danieldietrich, would you like me to change anything in this commit? :)

Contributor

paplorinc commented Oct 29, 2016

@danieldietrich, would you like me to change anything in this commit? :)

@danieldietrich

This comment has been minimized.

Show comment
Hide comment
@danieldietrich

danieldietrich Oct 29, 2016

Member

@paplorinc Will get my hands on the new version this evening. Sorry for the delay - so many things to do in parallel. The new Vector will be awesome!

Member

danieldietrich commented Oct 29, 2016

@paplorinc Will get my hands on the new version this evening. Sorry for the delay - so many things to do in parallel. The new Vector will be awesome!

@danieldietrich

Hi Lorinc, many thanks for your changes. I think we will still need one or more iterations before pulling it. I'm not sure about the ArrayType impls yet. But before suggesting a little redesign or refactoring I will provide you with my current comments. Please give me additional time to look over ArrayType and the usages...
Thank you for your great work on Vector.
Daniel

/**
* Generator of javaslang.collection.*ArrayType
*/

This comment has been minimized.

@danieldietrich

danieldietrich Oct 29, 2016

Member

minor: indentation (one space too much before '*')

@danieldietrich

danieldietrich Oct 29, 2016

Member

minor: indentation (one space too much before '*')

This comment has been minimized.

@paplorinc

paplorinc Oct 30, 2016

Contributor

I find indentations important also (though I aligned it this way on purpose).
Will change it.

@paplorinc

paplorinc Oct 30, 2016

Contributor

I find indentations important also (though I aligned it this way on purpose).
Will change it.

Show outdated Hide outdated javaslang/generator/Generator.scala
val cast = if (wrapperType != "Object") s"($wrapperType)" else ""
xs"""
/**

This comment has been minimized.

@danieldietrich

danieldietrich Oct 29, 2016

Member

minor: indentation

@danieldietrich

danieldietrich Oct 29, 2016

Member

minor: indentation

This comment has been minimized.

@paplorinc

paplorinc Oct 30, 2016

Contributor

indeed, thanks

@paplorinc

paplorinc Oct 30, 2016

Contributor

indeed, thanks

Show outdated Hide outdated javaslang/src/main/java/javaslang/collection/BitMappedTrie.java
@@ -10,7 +10,9 @@
import java.util.function.Function;
import java.util.function.Predicate;
import static javaslang.collection.Arrays.*;
import static javaslang.Function1.identity;

This comment has been minimized.

@danieldietrich

danieldietrich Oct 29, 2016

Member

minor: please remove this import and use Function.identity() instead (Function is already imported and we do not need special Function1 features)

@danieldietrich

danieldietrich Oct 29, 2016

Member

minor: please remove this import and use Function.identity() instead (Function is already imported and we do not need special Function1 features)

Show outdated Hide outdated javaslang/src/main/java/javaslang/collection/BitMappedTrie.java
if (length() == 0) {
return new BitMappedTrie<>(asArray(leading), offset, newSize, depthShift);
if (cannotAdd(leading)) {
return map(identity()).prepend(leading);

This comment has been minimized.

@danieldietrich

danieldietrich Oct 29, 2016

Member

Could we create a method to replace this idiom of copying the BMT?

private BitMappedTrie<T> boxed() { return map(Function.identity()); }
@danieldietrich

danieldietrich Oct 29, 2016

Member

Could we create a method to replace this idiom of copying the BMT?

private BitMappedTrie<T> boxed() { return map(Function.identity()); }

This comment has been minimized.

@paplorinc

paplorinc Oct 30, 2016

Contributor

I thought about it, forgot to add it, thanks :)

@paplorinc

paplorinc Oct 30, 2016

Contributor

I thought about it, forgot to add it, thanks :)

final Object previous = array[previousIndex];
final Object[] newNode = node.apply(previous, offset);
array[previousIndex] = newNode;
final Object previous = obj().getAt(array, previousIndex);

This comment has been minimized.

@danieldietrich

danieldietrich Oct 29, 2016

Member

The usages of obj() / the casts are unnecessary here because the target type is Object.
We should use ObjectArrayType.INSTANCE instead.


Note: When reading the code I wondered why we use obj() here directly. Can't the values be of primitive type?

@danieldietrich

danieldietrich Oct 29, 2016

Member

The usages of obj() / the casts are unnecessary here because the target type is Object.
We should use ObjectArrayType.INSTANCE instead.


Note: When reading the code I wondered why we use obj() here directly. Can't the values be of primitive type?

This comment has been minimized.

@danieldietrich

danieldietrich Oct 30, 2016

Member

The usages of obj() are OK!

@danieldietrich

danieldietrich Oct 30, 2016

Member

The usages of obj() are OK!

Show outdated Hide outdated javaslang/src/main/java/javaslang/collection/ArrayType.java
: PRIMITIVES[i];
}
private static int primitiveIndex(Class<?> wrapper) { /* linear search is faster than binary search here */

This comment has been minimized.

@danieldietrich

danieldietrich Oct 29, 2016

Member

Only use in asPrimitive(Class). Can be removed too.

@danieldietrich

danieldietrich Oct 29, 2016

Member

Only use in asPrimitive(Class). Can be removed too.

Show outdated Hide outdated javaslang/src/main/java/javaslang/collection/ArrayType.java
@SuppressWarnings("unchecked")
static <T> T asPrimitives(Class<?> primitiveClass, Iterable<?> values) {
final Object[] array = Array.ofAll(values).toJavaArray();
assert (array.length == 0) || (primitiveClass == asPrimitive(array[0].getClass())) && !primitiveClass.isArray();

This comment has been minimized.

@danieldietrich

danieldietrich Oct 29, 2016

Member

we need to remove this assert please - it is the only remaing line that uses the code below that should be removed

@danieldietrich

danieldietrich Oct 29, 2016

Member

we need to remove this assert please - it is the only remaing line that uses the code below that should be removed

This comment has been minimized.

@paplorinc

paplorinc Oct 30, 2016

Contributor

If you still think I should, I will, just let me know if it's still the case (see my other responses)

@paplorinc

paplorinc Oct 30, 2016

Contributor

If you still think I should, I will, just let me know if it's still the case (see my other responses)

Show outdated Hide outdated javaslang/src/main/java/javaslang/collection/ArrayType.java
/* convert to primitive */
private static final Class<?>[] WRAPPERS = { Boolean.class, Byte.class, Character.class, Double.class, Float.class, Integer.class, Long.class, Short.class, Void.class };
private static final Class<?>[] PRIMITIVES = { boolean.class, byte.class, char.class, double.class, float.class, int.class, long.class, short.class, void.class };

This comment has been minimized.

@danieldietrich

danieldietrich Oct 29, 2016

Member

both constants can be removed

@danieldietrich

danieldietrich Oct 29, 2016

Member

both constants can be removed

This comment has been minimized.

@paplorinc

paplorinc Oct 30, 2016

Contributor

JMH shows that the if/else version is ~5% faster, thanks :)

@paplorinc

paplorinc Oct 30, 2016

Contributor

JMH shows that the if/else version is ~5% faster, thanks :)

Show outdated Hide outdated javaslang/src/main/java/javaslang/collection/BitMappedTrie.java
}
return index;
});
return BitMappedTrie.ofAll(results);
}
int length() { return length; }
int length() { return length; }
private boolean cannotAdd(T element) { return type != obj() && ((element == null) || (type.type() != asPrimitive(element.getClass()))); }

This comment has been minimized.

@danieldietrich

danieldietrich Oct 29, 2016

Member

We don't need to check the types. Because we already use T we only need to check if we want to insert null into a primitive array.

private boolean addNullToPrimitiveArray(T element) { return !arrayType.isPrimitive() && element == null; }

I don't think we need an additional check if the element is an Object and we want to insert it into an primitive array. If the element is not null it should be unboxed, right?

If it is not unboxed we have to:

private boolean addNullOrObjectToPrimitiveArray(T element) {
    return arrayType.isPrimitive() && (element == null || !arrayType.isPrimitive());
}
@danieldietrich

danieldietrich Oct 29, 2016

Member

We don't need to check the types. Because we already use T we only need to check if we want to insert null into a primitive array.

private boolean addNullToPrimitiveArray(T element) { return !arrayType.isPrimitive() && element == null; }

I don't think we need an additional check if the element is an Object and we want to insert it into an primitive array. If the element is not null it should be unboxed, right?

If it is not unboxed we have to:

private boolean addNullOrObjectToPrimitiveArray(T element) {
    return arrayType.isPrimitive() && (element == null || !arrayType.isPrimitive());
}

This comment has been minimized.

@paplorinc

paplorinc Oct 30, 2016

Contributor

We don't need to check the types. Because we already use T

you can add a String to a primitive array via narrow, e.g.

@Test
public void shouldNarrowPrimitives() {
    final Vector<Object> object = Vector.narrow(range(0, 2));
    Vector<Object> actual = object.append("String");
    assertThat(actual).isEqualTo(of(0, 1, "String"));
}

(added this test explicitly now. shouldBehaveLikeArray already tested this implicitly)

arrayType.isPrimitive() && (element == null || !arrayType.isPrimitive());

A & (B | !A) == A && B, since !A is always false (typo?)


let me know if you still prefer to change this

@paplorinc

paplorinc Oct 30, 2016

Contributor

We don't need to check the types. Because we already use T

you can add a String to a primitive array via narrow, e.g.

@Test
public void shouldNarrowPrimitives() {
    final Vector<Object> object = Vector.narrow(range(0, 2));
    Vector<Object> actual = object.append("String");
    assertThat(actual).isEqualTo(of(0, 1, "String"));
}

(added this test explicitly now. shouldBehaveLikeArray already tested this implicitly)

arrayType.isPrimitive() && (element == null || !arrayType.isPrimitive());

A & (B | !A) == A && B, since !A is always false (typo?)


let me know if you still prefer to change this

Show outdated Hide outdated javaslang/src/main/java/javaslang/collection/ArrayType.java
import java.io.Serializable;
abstract class ArrayType<T> implements Serializable {

This comment has been minimized.

@danieldietrich

danieldietrich Oct 29, 2016

Member

I would love to see one Array abstraction on the file system, e.g. ArrayType and have the implementations as auxiliary classes. The ArrayType then needs to be added to the generator.

But I suggested below to call ObjectArrayType.xxx directly instead of using obj(). Maybe we need to call obj() then. Or do you have another suggestion?

@danieldietrich

danieldietrich Oct 29, 2016

Member

I would love to see one Array abstraction on the file system, e.g. ArrayType and have the implementations as auxiliary classes. The ArrayType then needs to be added to the generator.

But I suggested below to call ObjectArrayType.xxx directly instead of using obj(). Maybe we need to call obj() then. Or do you have another suggestion?

This comment has been minimized.

@paplorinc

paplorinc Oct 30, 2016

Contributor

Uff, I don't understand what you mean by file system in this context.
Also, I can't easily make the methods in ObjectArrayType static :/
Suggestions?

@paplorinc

paplorinc Oct 30, 2016

Contributor

Uff, I don't understand what you mean by file system in this context.
Also, I can't easily make the methods in ObjectArrayType static :/
Suggestions?

This comment has been minimized.

@danieldietrich

danieldietrich Oct 30, 2016

Member

Please change the ArrayType like follows (example for int only). We make the following changes:

  • also generate ArrayType
  • make ArrayType an interface
  • ArrayType should not extends Serializable. We've done this for other Javaslang types also - only impls implement Serializable (exception: some collections interfaces extends Function1 and are therefore Serializable)
  • make the ArrayType impls static nested classes.

Also I suggest to rename type() to componentType() (see TODO below).

/*     / \____  _    _  ____   ______  / \ ____  __    _______
 *    /  /    \/ \  / \/    \ /  /\__\/  //    \/  \  //  /\__\   JΛVΛSLΛNG
 *  _/  /  /\  \  \/  /  /\  \\__\\  \  //  /\  \ /\\/ \ /__\ \   Copyright 2014-2016 Javaslang, http://javaslang.io
 * /___/\_/  \_/\____/\_/  \_/\__\/__/\__\_/  \_//  \__/\_____/   Licensed under the Apache License, Version 2.0
 */
package javaslang.collection;

import javaslang.collection.*;

import java.io.Serializable;

interface ArrayType<T> {

    @SuppressWarnings("unchecked")
    static <T> ArrayType<T> obj() { return (ArrayType<T>) ObjectArrayType.INSTANCE; }

    @SuppressWarnings("unchecked")
    static <T> ArrayType<T> of(Object array) {
        final Class<?> type = array.getClass().getComponentType();
        if (!type.isPrimitive()) {
            return obj();
        } else if (boolean.class == type) {
            return (ArrayType<T>) BooleanArrayType.INSTANCE;
        } else if (byte.class == type) {
            return (ArrayType<T>) ByteArrayType.INSTANCE;
        } else if (char.class == type) {
            return (ArrayType<T>) CharArrayType.INSTANCE;
        } else if (double.class == type) {
            return (ArrayType<T>) DoubleArrayType.INSTANCE;
        } else if (float.class == type) {
            return (ArrayType<T>) FloatArrayType.INSTANCE;
        } else if (int.class == type) {
            return (ArrayType<T>) IntArrayType.INSTANCE;
        } else if (long.class == type) {
            return (ArrayType<T>) LongArrayType.INSTANCE;
        } else if (short.class == type) {
            return (ArrayType<T>) ShortArrayType.INSTANCE;
        } else {
            throw new IllegalArgumentException("Unknown type: " + type);
        }
    }

    Class<T> type(); // TODO: rename to componentType
    int lengthOf(Object array);
    T getAt(Object array, int index);

    Object empty();
    void setAt(Object array, int index, Object value);
    Object copy(Object array, int arraySize, int sourceFrom, int destinationFrom, int size);

    default Object newInstance(int length) { return copy(empty(), length); }

    /** System.arrayCopy with same source and destination */
    default Object copyRange(Object array, int from, int to) {
        final int length = to - from;
        return copy(array, length, from, 0, length);
    }

    /** Repeatedly group an array into equal sized sub-trees */
    default Object grouped(Object array, int groupSize) {
        final int arrayLength = lengthOf(array);
        assert arrayLength > groupSize;
        final Object results = obj().newInstance(1 + ((arrayLength - 1) / groupSize));
        obj().setAt(results, 0, copyRange(array, 0, groupSize));

        for (int start = groupSize, i = 1; start < arrayLength; i++) {
            final int nextLength = Math.min(groupSize, arrayLength - (i * groupSize));
            obj().setAt(results, i, copyRange(array, start, start + nextLength));
            start += nextLength;
        }

        return results;
    }

    /** clone the source and set the value at the given position */
    default Object copyUpdate(Object array, int index, T element) {
        final Object copy = copy(array, index + 1);
        setAt(copy, index, element);
        return copy;
    }

    default Object copy(Object array, int minLength) {
        final int arrayLength = lengthOf(array);
        final int length = Math.max(arrayLength, minLength);
        return copy(array, length, 0, 0, arrayLength);
    }

    /** clone the source and keep everything after the index (pre-padding the values with null) */
    default Object copyDrop(Object array, int index) {
        final int length = lengthOf(array);
        return copy(array, length, index, index, length - index);
    }

    /** clone the source and keep everything before and including the index */
    default Object copyTake(Object array, int lastIndex) {
        return copyRange(array, 0, lastIndex + 1);
    }

    /** Create a single element array */
    default Object asArray(T element) {
        final Object result = newInstance(1);
        setAt(result, 0, element);
        return result;
    }

    /** Store the content of an iterable in an array */
    static Object[] asArray(java.util.Iterator<?> it, int length) {
        final Object[] array = new Object[length];
        for (int i = 0; i < length; i++) {
            array[i] = it.next();
        }
        return array;
    }

    @SuppressWarnings("unchecked")
    static <T> T asPrimitives(Class<?> primitiveClass, Iterable<?> values) {
        final Object[] array = Array.ofAll(values).toJavaArray();
        assert (array.length == 0) || (primitiveClass == primitiveType(array[0])) && !primitiveClass.isArray();
        final ArrayType<T> type = of((Class<T>) primitiveClass);
        final Object results = type.newInstance(array.length);
        for (int i = 0; i < array.length; i++) {
            type.setAt(results, i, array[i]);
        }
        return (T) results;
    }

    static <T> Class<?> primitiveType(T element) {
        final Class<?> wrapper = (element == null) ? Object.class : element.getClass();
        if (wrapper == Boolean.class) {
            return boolean.class;
        } else if (wrapper == Byte.class) {
            return byte.class;
        } else if (wrapper == Character.class) {
            return char.class;
        } else if (wrapper == Double.class) {
            return double.class;
        } else if (wrapper == Float.class) {
            return float.class;
        } else if (wrapper == Integer.class) {
            return int.class;
        } else if (wrapper == Long.class) {
            return long.class;
        } else if (wrapper == Short.class) {
            return short.class;
        } else {
            return wrapper;
        }
    }

    final class IntArrayType implements ArrayType<Integer>, Serializable {

        private static final long serialVersionUID = 1L;

        private static final IntArrayType INSTANCE = new IntArrayType();
        private static final int[] EMPTY = new int[0];

        private static int[] cast(Object array) { return (int[]) array; }

        @Override
        public Class<Integer> type() { return int.class; }

        @Override
        public int[] empty() { return EMPTY; }

        @Override
        public int lengthOf(Object array) { return (array == null) ? 0 : cast(array).length; }

        @Override
        public Integer getAt(Object array, int index) { return cast(array)[index]; }

        @Override
        public void setAt(Object array, int index, Object value) { cast(array)[index] = (Integer) value; }

        @Override
        public Object copy(Object array, int arraySize, int sourceFrom, int destinationFrom, int size) {
            if (size == 0) {
                return new int[arraySize];
            } else {
                final int[] result = new int[arraySize];
                System.arraycopy(array, sourceFrom, result, destinationFrom, size); /* has to be near the object allocation to avoid zeroing out the array */
                return result;
            }
        }
    }

}
@danieldietrich

danieldietrich Oct 30, 2016

Member

Please change the ArrayType like follows (example for int only). We make the following changes:

  • also generate ArrayType
  • make ArrayType an interface
  • ArrayType should not extends Serializable. We've done this for other Javaslang types also - only impls implement Serializable (exception: some collections interfaces extends Function1 and are therefore Serializable)
  • make the ArrayType impls static nested classes.

Also I suggest to rename type() to componentType() (see TODO below).

/*     / \____  _    _  ____   ______  / \ ____  __    _______
 *    /  /    \/ \  / \/    \ /  /\__\/  //    \/  \  //  /\__\   JΛVΛSLΛNG
 *  _/  /  /\  \  \/  /  /\  \\__\\  \  //  /\  \ /\\/ \ /__\ \   Copyright 2014-2016 Javaslang, http://javaslang.io
 * /___/\_/  \_/\____/\_/  \_/\__\/__/\__\_/  \_//  \__/\_____/   Licensed under the Apache License, Version 2.0
 */
package javaslang.collection;

import javaslang.collection.*;

import java.io.Serializable;

interface ArrayType<T> {

    @SuppressWarnings("unchecked")
    static <T> ArrayType<T> obj() { return (ArrayType<T>) ObjectArrayType.INSTANCE; }

    @SuppressWarnings("unchecked")
    static <T> ArrayType<T> of(Object array) {
        final Class<?> type = array.getClass().getComponentType();
        if (!type.isPrimitive()) {
            return obj();
        } else if (boolean.class == type) {
            return (ArrayType<T>) BooleanArrayType.INSTANCE;
        } else if (byte.class == type) {
            return (ArrayType<T>) ByteArrayType.INSTANCE;
        } else if (char.class == type) {
            return (ArrayType<T>) CharArrayType.INSTANCE;
        } else if (double.class == type) {
            return (ArrayType<T>) DoubleArrayType.INSTANCE;
        } else if (float.class == type) {
            return (ArrayType<T>) FloatArrayType.INSTANCE;
        } else if (int.class == type) {
            return (ArrayType<T>) IntArrayType.INSTANCE;
        } else if (long.class == type) {
            return (ArrayType<T>) LongArrayType.INSTANCE;
        } else if (short.class == type) {
            return (ArrayType<T>) ShortArrayType.INSTANCE;
        } else {
            throw new IllegalArgumentException("Unknown type: " + type);
        }
    }

    Class<T> type(); // TODO: rename to componentType
    int lengthOf(Object array);
    T getAt(Object array, int index);

    Object empty();
    void setAt(Object array, int index, Object value);
    Object copy(Object array, int arraySize, int sourceFrom, int destinationFrom, int size);

    default Object newInstance(int length) { return copy(empty(), length); }

    /** System.arrayCopy with same source and destination */
    default Object copyRange(Object array, int from, int to) {
        final int length = to - from;
        return copy(array, length, from, 0, length);
    }

    /** Repeatedly group an array into equal sized sub-trees */
    default Object grouped(Object array, int groupSize) {
        final int arrayLength = lengthOf(array);
        assert arrayLength > groupSize;
        final Object results = obj().newInstance(1 + ((arrayLength - 1) / groupSize));
        obj().setAt(results, 0, copyRange(array, 0, groupSize));

        for (int start = groupSize, i = 1; start < arrayLength; i++) {
            final int nextLength = Math.min(groupSize, arrayLength - (i * groupSize));
            obj().setAt(results, i, copyRange(array, start, start + nextLength));
            start += nextLength;
        }

        return results;
    }

    /** clone the source and set the value at the given position */
    default Object copyUpdate(Object array, int index, T element) {
        final Object copy = copy(array, index + 1);
        setAt(copy, index, element);
        return copy;
    }

    default Object copy(Object array, int minLength) {
        final int arrayLength = lengthOf(array);
        final int length = Math.max(arrayLength, minLength);
        return copy(array, length, 0, 0, arrayLength);
    }

    /** clone the source and keep everything after the index (pre-padding the values with null) */
    default Object copyDrop(Object array, int index) {
        final int length = lengthOf(array);
        return copy(array, length, index, index, length - index);
    }

    /** clone the source and keep everything before and including the index */
    default Object copyTake(Object array, int lastIndex) {
        return copyRange(array, 0, lastIndex + 1);
    }

    /** Create a single element array */
    default Object asArray(T element) {
        final Object result = newInstance(1);
        setAt(result, 0, element);
        return result;
    }

    /** Store the content of an iterable in an array */
    static Object[] asArray(java.util.Iterator<?> it, int length) {
        final Object[] array = new Object[length];
        for (int i = 0; i < length; i++) {
            array[i] = it.next();
        }
        return array;
    }

    @SuppressWarnings("unchecked")
    static <T> T asPrimitives(Class<?> primitiveClass, Iterable<?> values) {
        final Object[] array = Array.ofAll(values).toJavaArray();
        assert (array.length == 0) || (primitiveClass == primitiveType(array[0])) && !primitiveClass.isArray();
        final ArrayType<T> type = of((Class<T>) primitiveClass);
        final Object results = type.newInstance(array.length);
        for (int i = 0; i < array.length; i++) {
            type.setAt(results, i, array[i]);
        }
        return (T) results;
    }

    static <T> Class<?> primitiveType(T element) {
        final Class<?> wrapper = (element == null) ? Object.class : element.getClass();
        if (wrapper == Boolean.class) {
            return boolean.class;
        } else if (wrapper == Byte.class) {
            return byte.class;
        } else if (wrapper == Character.class) {
            return char.class;
        } else if (wrapper == Double.class) {
            return double.class;
        } else if (wrapper == Float.class) {
            return float.class;
        } else if (wrapper == Integer.class) {
            return int.class;
        } else if (wrapper == Long.class) {
            return long.class;
        } else if (wrapper == Short.class) {
            return short.class;
        } else {
            return wrapper;
        }
    }

    final class IntArrayType implements ArrayType<Integer>, Serializable {

        private static final long serialVersionUID = 1L;

        private static final IntArrayType INSTANCE = new IntArrayType();
        private static final int[] EMPTY = new int[0];

        private static int[] cast(Object array) { return (int[]) array; }

        @Override
        public Class<Integer> type() { return int.class; }

        @Override
        public int[] empty() { return EMPTY; }

        @Override
        public int lengthOf(Object array) { return (array == null) ? 0 : cast(array).length; }

        @Override
        public Integer getAt(Object array, int index) { return cast(array)[index]; }

        @Override
        public void setAt(Object array, int index, Object value) { cast(array)[index] = (Integer) value; }

        @Override
        public Object copy(Object array, int arraySize, int sourceFrom, int destinationFrom, int size) {
            if (size == 0) {
                return new int[arraySize];
            } else {
                final int[] result = new int[arraySize];
                System.arraycopy(array, sourceFrom, result, destinationFrom, size); /* has to be near the object allocation to avoid zeroing out the array */
                return result;
            }
        }
    }

}

This comment has been minimized.

@paplorinc

paplorinc Oct 30, 2016

Contributor

Mostly done - and pushed -, not sure about embedding them to the same file, though.
What's the advantage exactly, are we trying to simulate a sealed trait in Java this way?

Pushed most of the changes, if it's easier for you to merge-and-modify rather than explain further, I won't mind :)

@paplorinc

paplorinc Oct 30, 2016

Contributor

Mostly done - and pushed -, not sure about embedding them to the same file, though.
What's the advantage exactly, are we trying to simulate a sealed trait in Java this way?

Pushed most of the changes, if it's easier for you to merge-and-modify rather than explain further, I won't mind :)

paplorinc added some commits Oct 21, 2016

Changed all internal `Vector` arrays to Objects
No primitive support yet.
This delegation shown only minor slowdown:
    Operation Ratio                              32768
    Create    slang_persistent/scala_persistent  4.83×
    Head      slang_persistent/scala_persistent  1.11×
    Tail      slang_persistent/scala_persistent  8.35×
    Get       slang_persistent/scala_persistent  0.75×
    Update    slang_persistent/scala_persistent  1.13×
    Map       slang_persistent/scala_persistent  2.45×
    Filter    slang_persistent/scala_persistent  1.84×
    Prepend   slang_persistent/scala_persistent  1.37×
    Append    slang_persistent/scala_persistent  0.88×
    GroupBy   slang_persistent/scala_persistent  0.86×
    Slice     slang_persistent/scala_persistent  7.70×
    Iterate   slang_persistent/scala_persistent  0.84×
Automatic, internal primitive storage support for `Vector`
This PR modifies the optimized `Vector` to store its data internally in the format it was created from, i.e.
```java
Vector.ofAll(3, 1, 4, 1, 5); // stored internally as `int[]`
```
will create a `Vector` that has an `int[]` internally and
```java
Vector.ofAll(Arrays.asList(3, 1, 4, 1, 5)); // boxed to `Integer[]` by `asList`
```
will have an `Object[]` inside.

The user doesn't have to know how it's stored internally (e.g. adding a `null` will automatically box everything in amortized constant time).

Internally many operations can work with primitive arrays without boxing (e.g. `slice`, `iterate`, `filter`, `distinct`), but others have to wrap them to make sense (e.g. `flatMap`).

Memory-wise storing primitives can be up to `~12×` more memory efficient (e.g. `54,120 bytes` for Vector<Byte> compared to `653,672 bytes` for `ArrayList<Byte>` (or `3,408,624 bytes` for Functional Java's `Seq<Byte>`)).
Interestingly many operations are faster also (e.g. `create` and `iterate`), sometimes even faster than the `Java` counterpart - e.g. `filter` or `map`.

The [benchmarks](https://github.com/paplorinc/javaslang/blob/Primitivization/javaslang-benchmark/src/test/java/javaslang/collection/VectorBenchmark.java) are as follows:
```java
Operation Ratio                                                32     1024    32768
Create    slang_persistent/java_mutable                     1.51×    0.92×    1.17×
Create    slang_persistent/java_mutable_boxed              11.10×    8.64×    6.17×
Create    slang_persistent/java_mutable_boxed_stream       16.68×   10.77×    9.57×
Create    slang_persistent/fjava_persistent               229.91×  216.16×  192.30×
Create    slang_persistent/pcollections_persistent        126.90×  253.86×  375.34×
Create    slang_persistent/ecollections_persistent          2.05×    0.95×    1.18×
Create    slang_persistent/clojure_persistent               0.95×   15.40×   12.79×
Create    slang_persistent/scala_persistent                13.96×    9.32×    6.43×

Head      slang_persistent/java_mutable                     0.71×    0.53×    0.40×
Head      slang_persistent/fjava_persistent                 0.85×    0.66×    0.51×
Head      slang_persistent/pcollections_persistent          2.18×    3.75×    4.68×
Head      slang_persistent/ecollections_persistent          0.77×    0.61×    0.47×
Head      slang_persistent/clojure_persistent               0.70×    0.83×    0.90×
Head      slang_persistent/scala_persistent                 0.78×    0.63×    0.49×

Tail      slang_persistent/java_mutable                     0.71×    0.56×    0.39×
Tail      slang_persistent/fjava_persistent                52.84×   50.26×   52.36×
Tail      slang_persistent/pcollections_persistent          5.85×    9.15×   12.06×
Tail      slang_persistent/ecollections_persistent         16.88×  156.82× 4301.60×
Tail      slang_persistent/clojure_persistent               0.93×    0.69×    0.60×
Tail      slang_persistent/scala_persistent                 2.98×    4.87×    5.97×

Get       slang_persistent/java_mutable                     0.71×    0.12×    0.36×
Get       slang_persistent/fjava_persistent                55.92×   28.12×   79.81×
Get       slang_persistent/pcollections_persistent          8.32×    4.20×   17.46×
Get       slang_persistent/ecollections_persistent          0.71×    0.10×    0.34×
Get       slang_persistent/clojure_persistent               0.93×    0.37×    1.45×
Get       slang_persistent/scala_persistent                 1.13×    0.30×    0.95×

Update    slang_persistent/java_mutable                     0.11×    0.05×    0.03×
Update    slang_persistent/fjava_persistent               133.11×  262.66×  222.09×
Update    slang_persistent/pcollections_persistent          2.45×    4.30×    4.97×
Update    slang_persistent/ecollections_persistent         13.88×  162.43× 2589.05×
Update    slang_persistent/clojure_persistent               0.85×    1.23×    1.10×
Update    slang_persistent/scala_persistent                 1.03×    1.07×    1.14×

Map       slang_persistent/java_mutable                     2.19×    2.27×    2.00×
Map       slang_persistent/java_mutable_loop                0.53×    0.67×    0.54×
Map       slang_persistent/ecollections_persistent          1.16×    1.15×    1.06×
Map       slang_persistent/scala_persistent                 1.03×    1.43×    1.73×

Filter    slang_persistent/java_mutable                     1.97×    1.64×    1.15×
Filter    slang_persistent/ecollections_persistent          1.67×    1.14×    0.92×
Filter    slang_persistent/scala_persistent                 1.45×    1.44×    1.42×

Prepend   slang_persistent/java_mutable                     0.38×    0.71×   11.80×
Prepend   slang_persistent/fjava_persistent                 1.43×    1.62×    1.17×
Prepend   slang_persistent/pcollections_persistent          0.94×    2.47×    2.95×
Prepend   slang_persistent/ecollections_persistent          2.53×   27.04×  569.17×
Prepend   slang_persistent/clojure_persistent               5.90×  124.95× 3177.65×
Prepend   slang_persistent/scala_persistent                 0.22×    0.23×    0.15×

Append    slang_persistent/java_mutable                     0.25×    0.05×    0.03×
Append    slang_persistent/fjava_persistent                 5.18×    1.54×    1.02×
Append    slang_persistent/pcollections_persistent          2.71×    1.87×    2.22×
Append    slang_persistent/ecollections_persistent          7.93×   24.56×  560.25×
Append    slang_persistent/clojure_persistent               0.99×    0.23×    0.18×
Append    slang_persistent/scala_persistent                 0.81×    0.22×    0.16×

GroupBy   slang_persistent/java_mutable                     0.43×    0.65×    0.83×
GroupBy   slang_persistent/scala_persistent                 1.44×    1.05×    1.21×

Slice     slang_persistent/java_mutable                     0.54×    0.49×    0.45×
Slice     slang_persistent/pcollections_persistent          0.51×    0.42×    0.40×
Slice     slang_persistent/clojure_persistent               0.54×    0.45×    0.43×
Slice     slang_persistent/scala_persistent                 2.43×    5.61×    8.61×

Iterate   slang_persistent/java_mutable                     0.88×    0.45×    0.39×
Iterate   slang_persistent/fjava_persistent               368.25×  442.89×  289.54×
Iterate   slang_persistent/pcollections_persistent         14.61×   13.76×    9.42×
Iterate   slang_persistent/ecollections_persistent          2.35×    1.72×    1.52×
Iterate   slang_persistent/clojure_persistent               0.79×    1.03×    0.85×
Iterate   slang_persistent/scala_persistent                 1.53×    1.49×    0.90×
```
Note: Apparently `Java 9`  made some array copying optimizations (that made `append` slower than `prepend` in the first place), making `update`, `append` and `groupBy` slightly faster.

primitive:
```java
Operation Ratio                                                32     1024    32768
Create    slang_persistent_int/java_mutable                 3.55×    1.71×    1.35×
Create    slang_persistent_int/java_mutable_boxed          26.16×   16.01×    7.09×
Create    slang_persistent_int/java_mutable_boxed_stream   39.31×   19.96×   11.01×
Create    slang_persistent_int/fjava_persistent           542.00×  400.65×  221.11×
Create    slang_persistent_int/pcollections_persistent    299.17×  470.54×  431.57×
Create    slang_persistent_int/ecollections_persistent      4.84×    1.76×    1.36×
Create    slang_persistent_int/clojure_persistent           2.23×   28.55×   14.71×
Create    slang_persistent_int/scala_persistent            32.91×   17.27×    7.39×

Tail      slang_persistent_int/java_mutable                 0.71×    0.56×    0.45×
Tail      slang_persistent_int/fjava_persistent            52.55×   50.20×   59.62×
Tail      slang_persistent_int/pcollections_persistent      5.82×    9.14×   13.73×
Tail      slang_persistent_int/ecollections_persistent     16.79×  156.62× 4898.01×
Tail      slang_persistent_int/clojure_persistent           0.92×    0.69×    0.68×
Tail      slang_persistent_int/scala_persistent             2.97×    4.87×    6.80×

Update    slang_persistent_int/java_mutable                 0.11×    0.05×    0.03×
Update    slang_persistent_int/fjava_persistent           124.86×  254.21×  215.66×
Update    slang_persistent_int/pcollections_persistent      2.30×    4.16×    4.83×
Update    slang_persistent_int/ecollections_persistent     13.02×  157.21× 2514.05×
Update    slang_persistent_int/clojure_persistent           0.80×    1.19×    1.07×
Update    slang_persistent_int/scala_persistent             0.97×    1.04×    1.11×

Map       slang_persistent_int/java_mutable                 2.01×    1.85×    1.58×
Map       slang_persistent_int/java_mutable_loop            0.48×    0.54×    0.43×
Map       slang_persistent_int/ecollections_persistent      1.07×    0.93×    0.84×
Map       slang_persistent_int/scala_persistent             0.94×    1.16×    1.37×

Filter    slang_persistent_int/java_mutable                 2.35×    1.63×    0.87×
Filter    slang_persistent_int/ecollections_persistent      2.00×    1.13×    0.70×
Filter    slang_persistent_int/scala_persistent             1.73×    1.43×    1.07×

Prepend   slang_persistent_int/java_mutable                 0.84×    0.78×   12.69×
Prepend   slang_persistent_int/fjava_persistent             3.12×    1.78×    1.25×
Prepend   slang_persistent_int/pcollections_persistent      2.05×    2.72×    3.18×
Prepend   slang_persistent_int/ecollections_persistent      5.50×   29.76×  612.04×
Prepend   slang_persistent_int/clojure_persistent          12.83×  137.53× 3416.94×
Prepend   slang_persistent_int/scala_persistent             0.49×    0.26×    0.17×

Append    slang_persistent_int/java_mutable                 0.23×    0.11×    0.03×
Append    slang_persistent_int/fjava_persistent             4.66×    3.00×    1.15×
Append    slang_persistent_int/pcollections_persistent      2.43×    3.64×    2.50×
Append    slang_persistent_int/ecollections_persistent      7.13×   47.83×  631.27×
Append    slang_persistent_int/clojure_persistent           0.89×    0.46×    0.20×
Append    slang_persistent_int/scala_persistent             0.73×    0.42×    0.18×

Slice     slang_persistent_int/java_mutable                 0.56×    0.47×    0.38×
Slice     slang_persistent_int/pcollections_persistent      0.52×    0.40×    0.34×
Slice     slang_persistent_int/clojure_persistent           0.55×    0.44×    0.37×
Slice     slang_persistent_int/scala_persistent             2.50×    5.42×    7.28×

Iterate   slang_persistent_int/java_mutable                 1.76×    0.88×    1.38×
Iterate   slang_persistent_int/fjava_persistent           737.64×  858.07× 1036.08×
Iterate   slang_persistent_int/pcollections_persistent     29.26×   26.65×   33.69×
Iterate   slang_persistent_int/ecollections_persistent      4.71×    3.33×    5.44×
Iterate   slang_persistent_int/clojure_persistent           1.58×    2.01×    3.05×
Iterate   slang_persistent_int/scala_persistent             3.06×    2.89×    3.23×
Iterate   slang_persistent_int/slang_persistent             2.00×    1.94×    3.58×
```
![screenshot from 2016-09-10 14 20 50](https://cloud.githubusercontent.com/assets/1841944/18410161/e09fbfe4-7762-11e6-9524-3eb7e46bd530.png)

and memory usages:
```java
for `32` elements
  Java mutable @ ArrayList                            →   504 bytes
  Functional Java persistent @ Seq                    → 3,064 bytes
  PCollections persistent @ TreePVector               → 1,712 bytes
  Eclipse Collections persistent @ ImmutableArrayList →   496 bytes
  Clojure persistent @ PersistentVector               →   704 bytes
  Scala persistent @ Vector                           →   536 bytes
  Javaslang persistent @ Vector (Integer[])           →   544 bytes
  Javaslang persistent @ Vector (int[])               →   264 bytes
  Javaslang persistent @ Vector (byte[])              →   208 bytes

for `1024` elements
  Java mutable @ ArrayList                            →  19,064 bytes
  Functional Java persistent @ Seq                    → 104,760 bytes
  PCollections persistent @ TreePVector               →  55,984 bytes
  Eclipse Collections persistent @ ImmutableArrayList →  19,056 bytes
  Clojure persistent @ PersistentVector               →  20,504 bytes
  Scala persistent @ Vector                           →  19,736 bytes
  Javaslang persistent @ Vector (Integer[])           →  19,744 bytes
  Javaslang persistent @ Vector (int[])               →   4,816 bytes
  Javaslang persistent @ Vector (byte[])              →   1,896 bytes

for `32768` elements
  Java mutable @ ArrayList                            →   653,672 bytes
  Functional Java persistent @ Seq                    → 3,408,624 bytes
  PCollections persistent @ TreePVector               → 1,833,376 bytes
  Eclipse Collections persistent @ ImmutableArrayList →   653,664 bytes
  Clojure persistent @ PersistentVector               →   700,168 bytes
  Scala persistent @ Vector                           →   674,824 bytes
  Javaslang persistent @ Vector (Integer[])           →   674,832 bytes
  Javaslang persistent @ Vector (int[])               →   152,272 bytes
  Javaslang persistent @ Vector (byte[])              →    54,120 bytes
```
i.e. storing `byte`s in `functional java` would require `~63×` more storage.
Show outdated Hide outdated javaslang/generator/Generator.scala
val cast = if (wrapperType != "Object") s"($wrapperType)" else ""
xs"""
/**

This comment has been minimized.

@paplorinc

paplorinc Oct 30, 2016

Contributor

indeed, thanks

@paplorinc

paplorinc Oct 30, 2016

Contributor

indeed, thanks

Show outdated Hide outdated javaslang/src/main/java/javaslang/collection/BitMappedTrie.java
if (length() == 0) {
return new BitMappedTrie<>(asArray(leading), offset, newSize, depthShift);
if (cannotAdd(leading)) {
return map(identity()).prepend(leading);

This comment has been minimized.

@paplorinc

paplorinc Oct 30, 2016

Contributor

I thought about it, forgot to add it, thanks :)

@paplorinc

paplorinc Oct 30, 2016

Contributor

I thought about it, forgot to add it, thanks :)

if (skippedElements != digit(offset, shift)) {
break;
}
array = getAt(array, skippedElements);
array = obj().getAt(array, skippedElements);

This comment has been minimized.

@paplorinc

paplorinc Oct 30, 2016

Contributor

Valid question: the collapse is working on the parent nodes only, which are always reference arrays, never primitives ones (we wouldn't have anything to collapse if they weren't pointing to other nodes)

@paplorinc

paplorinc Oct 30, 2016

Contributor

Valid question: the collapse is working on the parent nodes only, which are always reference arrays, never primitives ones (we wouldn't have anything to collapse if they weren't pointing to other nodes)

if (skippedElements != digit(offset, shift)) {
break;
}
array = getAt(array, skippedElements);
array = obj().getAt(array, skippedElements);

This comment has been minimized.

@paplorinc

paplorinc Oct 30, 2016

Contributor

So instead of

obj().setAt(array, previousIndex, leaf.apply(obj().getAt(array, previousIndex), lastDigit(index)));

you would prefer

ObjectArrayType.INSTANCE.setAt(array, previousIndex, leaf.apply(ObjectArrayType.INSTANCE.getAt(array, previousIndex), lastDigit(index)));

?

I can't really make the methods in ObjectArrayType static as they would collide with the overrides.
I also can't import ObjectArrayType.INSTANCE.* in Java - would be nice, though.

@paplorinc

paplorinc Oct 30, 2016

Contributor

So instead of

obj().setAt(array, previousIndex, leaf.apply(obj().getAt(array, previousIndex), lastDigit(index)));

you would prefer

ObjectArrayType.INSTANCE.setAt(array, previousIndex, leaf.apply(ObjectArrayType.INSTANCE.getAt(array, previousIndex), lastDigit(index)));

?

I can't really make the methods in ObjectArrayType static as they would collide with the overrides.
I also can't import ObjectArrayType.INSTANCE.* in Java - would be nice, though.

offset -= treeSize(skippedElements, shift);
}
return new BitMappedTrie<>(array, offset, length, shift);
return new BitMappedTrie<>(type, array, offset, length, shift);

This comment has been minimized.

@paplorinc

paplorinc Oct 30, 2016

Contributor

Sure, but it's the type of the Vector also (i.e. the T)

@paplorinc

paplorinc Oct 30, 2016

Contributor

Sure, but it's the type of the Vector also (i.e. the T)

Show outdated Hide outdated javaslang/src-gen/main/java/javaslang/collection/ObjectArrayType.java
Object getAt(Object array, int index) { return cast(array)[index]; }
@Override
void setAt(Object array, int index, Object value) { cast(array)[index] = value; }

This comment has been minimized.

@paplorinc

paplorinc Oct 30, 2016

Contributor

spacing ...

@paplorinc

paplorinc Oct 30, 2016

Contributor

spacing ...

Show outdated Hide outdated javaslang/src/main/java/javaslang/collection/ArrayType.java
import java.io.Serializable;
abstract class ArrayType<T> implements Serializable {

This comment has been minimized.

@paplorinc

paplorinc Oct 30, 2016

Contributor

Uff, I don't understand what you mean by file system in this context.
Also, I can't easily make the methods in ObjectArrayType static :/
Suggestions?

@paplorinc

paplorinc Oct 30, 2016

Contributor

Uff, I don't understand what you mean by file system in this context.
Also, I can't easily make the methods in ObjectArrayType static :/
Suggestions?

Show outdated Hide outdated javaslang/src/main/java/javaslang/collection/ArrayType.java
} else {
throw new IllegalArgumentException("Unknown type: " + type);
}
}

This comment has been minimized.

@paplorinc

paplorinc Oct 30, 2016

Contributor

I wanted to separate the primitive types from the only non-primitive type.
Would you like me to merge the nested ifs instead?

@paplorinc

paplorinc Oct 30, 2016

Contributor

I wanted to separate the primitive types from the only non-primitive type.
Would you like me to merge the nested ifs instead?

Show outdated Hide outdated javaslang/src/main/java/javaslang/collection/ArrayType.java
@SuppressWarnings("unchecked")
static <T> T asPrimitives(Class<?> primitiveClass, Iterable<?> values) {
final Object[] array = Array.ofAll(values).toJavaArray();
assert (array.length == 0) || (primitiveClass == asPrimitive(array[0].getClass())) && !primitiveClass.isArray();

This comment has been minimized.

@paplorinc

paplorinc Oct 30, 2016

Contributor

If you still think I should, I will, just let me know if it's still the case (see my other responses)

@paplorinc

paplorinc Oct 30, 2016

Contributor

If you still think I should, I will, just let me know if it's still the case (see my other responses)

Show outdated Hide outdated javaslang/src/main/java/javaslang/collection/ArrayType.java
/* convert to primitive */
private static final Class<?>[] WRAPPERS = { Boolean.class, Byte.class, Character.class, Double.class, Float.class, Integer.class, Long.class, Short.class, Void.class };
private static final Class<?>[] PRIMITIVES = { boolean.class, byte.class, char.class, double.class, float.class, int.class, long.class, short.class, void.class };

This comment has been minimized.

@paplorinc

paplorinc Oct 30, 2016

Contributor

JMH shows that the if/else version is ~5% faster, thanks :)

@paplorinc

paplorinc Oct 30, 2016

Contributor

JMH shows that the if/else version is ~5% faster, thanks :)

@danieldietrich

Hey @paplorinc,
from what I can see these are my last review comments. After applying them I will take a last look and merge...

if (skippedElements != digit(offset, shift)) {
break;
}
array = getAt(array, skippedElements);
array = obj().getAt(array, skippedElements);

This comment has been minimized.

@danieldietrich

danieldietrich Oct 30, 2016

Member

You're right, of course setAt is not static.

@danieldietrich

danieldietrich Oct 30, 2016

Member

You're right, of course setAt is not static.

offset -= treeSize(skippedElements, shift);
}
return new BitMappedTrie<>(array, offset, length, shift);
return new BitMappedTrie<>(type, array, offset, length, shift);

This comment has been minimized.

@danieldietrich

danieldietrich Oct 30, 2016

Member

Mmhh, I see ArrayType<T>, not T:

private static <T> BitMappedTrie<T> collapsed(ArrayType<T> type, ...

and the constructor which is called looks like this:

    private BitMappedTrie(ArrayType<T> arrayType, Object array, int offset, int length, int depthShift) {
        this.arrayType = arrayType;

Therefore I would name it arrayType.

@danieldietrich

danieldietrich Oct 30, 2016

Member

Mmhh, I see ArrayType<T>, not T:

private static <T> BitMappedTrie<T> collapsed(ArrayType<T> type, ...

and the constructor which is called looks like this:

    private BitMappedTrie(ArrayType<T> arrayType, Object array, int offset, int length, int depthShift) {
        this.arrayType = arrayType;

Therefore I would name it arrayType.

static <T> BitMappedTrie<T> ofAll(Object[] array) {
final int size = array.length;
static <T> BitMappedTrie<T> ofAll(Object array) {
final ArrayType<T> type = ArrayType.of(array);

This comment has been minimized.

@danieldietrich

danieldietrich Oct 30, 2016

Member

Would either name all ArrayType related variables/parameters throughout this class type or arrayType. Your choice.

@danieldietrich

danieldietrich Oct 30, 2016

Member

Would either name all ArrayType related variables/parameters throughout this class type or arrayType. Your choice.

This comment has been minimized.

@paplorinc

paplorinc Oct 30, 2016

Contributor

Renamed all to type for brevity :)

@paplorinc

paplorinc Oct 30, 2016

Contributor

Renamed all to type for brevity :)

Show outdated Hide outdated javaslang-benchmark/src/test/java/javaslang/collection/VectorBenchmark.java
}
@State(Scope.Benchmark)
public static class Base {
@Param({ "32", "1024", "32768" /*, "1048576", "33554432", "1073741824" */ }) /* i.e. depth 1,2,3(,4,5,6) for a branching factor of 32 */
@Param({ "100", "10000" })

This comment has been minimized.

@danieldietrich

danieldietrich Oct 30, 2016

Member

I'm still thinking benchmarks should not run along with our unit tests.

If we need to test the correctness of our types/methods there should exist unit tests. The benchmark have a different purpose. Changing these values in order to make the tests run faster is wrong in my opinion. Benchmarks should measure all relevant scenarios, regardless how long they run. Otherwise they fail to reach their real goal.

To speed up the tests we should

  1. Run benchmarks on their own maven profile again.
  2. Throw out or rewrite the brute-force Vector property tests. It is too much. We need to distill the relevant business cases and write proper unit tests instead. Most other tests run ms these property tests run many seconds.
  3. Pull out the tests of the gwt example. This should also run separately.

I would love to see benchmarks and the gwt example run in their own CI builds, each triggered by a previous (successful) Javaslang build.

@danieldietrich

danieldietrich Oct 30, 2016

Member

I'm still thinking benchmarks should not run along with our unit tests.

If we need to test the correctness of our types/methods there should exist unit tests. The benchmark have a different purpose. Changing these values in order to make the tests run faster is wrong in my opinion. Benchmarks should measure all relevant scenarios, regardless how long they run. Otherwise they fail to reach their real goal.

To speed up the tests we should

  1. Run benchmarks on their own maven profile again.
  2. Throw out or rewrite the brute-force Vector property tests. It is too much. We need to distill the relevant business cases and write proper unit tests instead. Most other tests run ms these property tests run many seconds.
  3. Pull out the tests of the gwt example. This should also run separately.

I would love to see benchmarks and the gwt example run in their own CI builds, each triggered by a previous (successful) Javaslang build.

This comment has been minimized.

@paplorinc

paplorinc Oct 30, 2016

Contributor
  1. I don't mind if the benchmark correctness tests (e.g. that prependAll concats to the start and not the end, i.e. that's I'm testing the speed of the correct thing) run only on the CI, an not locally.
  2. The property based tests are incredibly useful. I have made them quicker now, they all run in 7 seconds :)
  3. same as 1.
  4. we could simply try parallel tests, as we're basically stateless, everything should work out of the box
@paplorinc

paplorinc Oct 30, 2016

Contributor
  1. I don't mind if the benchmark correctness tests (e.g. that prependAll concats to the start and not the end, i.e. that's I'm testing the speed of the correct thing) run only on the CI, an not locally.
  2. The property based tests are incredibly useful. I have made them quicker now, they all run in 7 seconds :)
  3. same as 1.
  4. we could simply try parallel tests, as we're basically stateless, everything should work out of the box

This comment has been minimized.

@danieldietrich

danieldietrich Oct 30, 2016

Member

Looks all good to me. +1 for trying parallel tests - should be a flag in the maven surefire plugin. I created #1652.

@danieldietrich

danieldietrich Oct 30, 2016

Member

Looks all good to me. +1 for trying parallel tests - should be a flag in the maven surefire plugin. I created #1652.

Show outdated Hide outdated javaslang/src/main/java/javaslang/collection/ArrayType.java
import java.io.Serializable;
abstract class ArrayType<T> implements Serializable {

This comment has been minimized.

@danieldietrich

danieldietrich Oct 30, 2016

Member

Please change the ArrayType like follows (example for int only). We make the following changes:

  • also generate ArrayType
  • make ArrayType an interface
  • ArrayType should not extends Serializable. We've done this for other Javaslang types also - only impls implement Serializable (exception: some collections interfaces extends Function1 and are therefore Serializable)
  • make the ArrayType impls static nested classes.

Also I suggest to rename type() to componentType() (see TODO below).

/*     / \____  _    _  ____   ______  / \ ____  __    _______
 *    /  /    \/ \  / \/    \ /  /\__\/  //    \/  \  //  /\__\   JΛVΛSLΛNG
 *  _/  /  /\  \  \/  /  /\  \\__\\  \  //  /\  \ /\\/ \ /__\ \   Copyright 2014-2016 Javaslang, http://javaslang.io
 * /___/\_/  \_/\____/\_/  \_/\__\/__/\__\_/  \_//  \__/\_____/   Licensed under the Apache License, Version 2.0
 */
package javaslang.collection;

import javaslang.collection.*;

import java.io.Serializable;

interface ArrayType<T> {

    @SuppressWarnings("unchecked")
    static <T> ArrayType<T> obj() { return (ArrayType<T>) ObjectArrayType.INSTANCE; }

    @SuppressWarnings("unchecked")
    static <T> ArrayType<T> of(Object array) {
        final Class<?> type = array.getClass().getComponentType();
        if (!type.isPrimitive()) {
            return obj();
        } else if (boolean.class == type) {
            return (ArrayType<T>) BooleanArrayType.INSTANCE;
        } else if (byte.class == type) {
            return (ArrayType<T>) ByteArrayType.INSTANCE;
        } else if (char.class == type) {
            return (ArrayType<T>) CharArrayType.INSTANCE;
        } else if (double.class == type) {
            return (ArrayType<T>) DoubleArrayType.INSTANCE;
        } else if (float.class == type) {
            return (ArrayType<T>) FloatArrayType.INSTANCE;
        } else if (int.class == type) {
            return (ArrayType<T>) IntArrayType.INSTANCE;
        } else if (long.class == type) {
            return (ArrayType<T>) LongArrayType.INSTANCE;
        } else if (short.class == type) {
            return (ArrayType<T>) ShortArrayType.INSTANCE;
        } else {
            throw new IllegalArgumentException("Unknown type: " + type);
        }
    }

    Class<T> type(); // TODO: rename to componentType
    int lengthOf(Object array);
    T getAt(Object array, int index);

    Object empty();
    void setAt(Object array, int index, Object value);
    Object copy(Object array, int arraySize, int sourceFrom, int destinationFrom, int size);

    default Object newInstance(int length) { return copy(empty(), length); }

    /** System.arrayCopy with same source and destination */
    default Object copyRange(Object array, int from, int to) {
        final int length = to - from;
        return copy(array, length, from, 0, length);
    }

    /** Repeatedly group an array into equal sized sub-trees */
    default Object grouped(Object array, int groupSize) {
        final int arrayLength = lengthOf(array);
        assert arrayLength > groupSize;
        final Object results = obj().newInstance(1 + ((arrayLength - 1) / groupSize));
        obj().setAt(results, 0, copyRange(array, 0, groupSize));

        for (int start = groupSize, i = 1; start < arrayLength; i++) {
            final int nextLength = Math.min(groupSize, arrayLength - (i * groupSize));
            obj().setAt(results, i, copyRange(array, start, start + nextLength));
            start += nextLength;
        }

        return results;
    }

    /** clone the source and set the value at the given position */
    default Object copyUpdate(Object array, int index, T element) {
        final Object copy = copy(array, index + 1);
        setAt(copy, index, element);
        return copy;
    }

    default Object copy(Object array, int minLength) {
        final int arrayLength = lengthOf(array);
        final int length = Math.max(arrayLength, minLength);
        return copy(array, length, 0, 0, arrayLength);
    }

    /** clone the source and keep everything after the index (pre-padding the values with null) */
    default Object copyDrop(Object array, int index) {
        final int length = lengthOf(array);
        return copy(array, length, index, index, length - index);
    }

    /** clone the source and keep everything before and including the index */
    default Object copyTake(Object array, int lastIndex) {
        return copyRange(array, 0, lastIndex + 1);
    }

    /** Create a single element array */
    default Object asArray(T element) {
        final Object result = newInstance(1);
        setAt(result, 0, element);
        return result;
    }

    /** Store the content of an iterable in an array */
    static Object[] asArray(java.util.Iterator<?> it, int length) {
        final Object[] array = new Object[length];
        for (int i = 0; i < length; i++) {
            array[i] = it.next();
        }
        return array;
    }

    @SuppressWarnings("unchecked")
    static <T> T asPrimitives(Class<?> primitiveClass, Iterable<?> values) {
        final Object[] array = Array.ofAll(values).toJavaArray();
        assert (array.length == 0) || (primitiveClass == primitiveType(array[0])) && !primitiveClass.isArray();
        final ArrayType<T> type = of((Class<T>) primitiveClass);
        final Object results = type.newInstance(array.length);
        for (int i = 0; i < array.length; i++) {
            type.setAt(results, i, array[i]);
        }
        return (T) results;
    }

    static <T> Class<?> primitiveType(T element) {
        final Class<?> wrapper = (element == null) ? Object.class : element.getClass();
        if (wrapper == Boolean.class) {
            return boolean.class;
        } else if (wrapper == Byte.class) {
            return byte.class;
        } else if (wrapper == Character.class) {
            return char.class;
        } else if (wrapper == Double.class) {
            return double.class;
        } else if (wrapper == Float.class) {
            return float.class;
        } else if (wrapper == Integer.class) {
            return int.class;
        } else if (wrapper == Long.class) {
            return long.class;
        } else if (wrapper == Short.class) {
            return short.class;
        } else {
            return wrapper;
        }
    }

    final class IntArrayType implements ArrayType<Integer>, Serializable {

        private static final long serialVersionUID = 1L;

        private static final IntArrayType INSTANCE = new IntArrayType();
        private static final int[] EMPTY = new int[0];

        private static int[] cast(Object array) { return (int[]) array; }

        @Override
        public Class<Integer> type() { return int.class; }

        @Override
        public int[] empty() { return EMPTY; }

        @Override
        public int lengthOf(Object array) { return (array == null) ? 0 : cast(array).length; }

        @Override
        public Integer getAt(Object array, int index) { return cast(array)[index]; }

        @Override
        public void setAt(Object array, int index, Object value) { cast(array)[index] = (Integer) value; }

        @Override
        public Object copy(Object array, int arraySize, int sourceFrom, int destinationFrom, int size) {
            if (size == 0) {
                return new int[arraySize];
            } else {
                final int[] result = new int[arraySize];
                System.arraycopy(array, sourceFrom, result, destinationFrom, size); /* has to be near the object allocation to avoid zeroing out the array */
                return result;
            }
        }
    }

}
@danieldietrich

danieldietrich Oct 30, 2016

Member

Please change the ArrayType like follows (example for int only). We make the following changes:

  • also generate ArrayType
  • make ArrayType an interface
  • ArrayType should not extends Serializable. We've done this for other Javaslang types also - only impls implement Serializable (exception: some collections interfaces extends Function1 and are therefore Serializable)
  • make the ArrayType impls static nested classes.

Also I suggest to rename type() to componentType() (see TODO below).

/*     / \____  _    _  ____   ______  / \ ____  __    _______
 *    /  /    \/ \  / \/    \ /  /\__\/  //    \/  \  //  /\__\   JΛVΛSLΛNG
 *  _/  /  /\  \  \/  /  /\  \\__\\  \  //  /\  \ /\\/ \ /__\ \   Copyright 2014-2016 Javaslang, http://javaslang.io
 * /___/\_/  \_/\____/\_/  \_/\__\/__/\__\_/  \_//  \__/\_____/   Licensed under the Apache License, Version 2.0
 */
package javaslang.collection;

import javaslang.collection.*;

import java.io.Serializable;

interface ArrayType<T> {

    @SuppressWarnings("unchecked")
    static <T> ArrayType<T> obj() { return (ArrayType<T>) ObjectArrayType.INSTANCE; }

    @SuppressWarnings("unchecked")
    static <T> ArrayType<T> of(Object array) {
        final Class<?> type = array.getClass().getComponentType();
        if (!type.isPrimitive()) {
            return obj();
        } else if (boolean.class == type) {
            return (ArrayType<T>) BooleanArrayType.INSTANCE;
        } else if (byte.class == type) {
            return (ArrayType<T>) ByteArrayType.INSTANCE;
        } else if (char.class == type) {
            return (ArrayType<T>) CharArrayType.INSTANCE;
        } else if (double.class == type) {
            return (ArrayType<T>) DoubleArrayType.INSTANCE;
        } else if (float.class == type) {
            return (ArrayType<T>) FloatArrayType.INSTANCE;
        } else if (int.class == type) {
            return (ArrayType<T>) IntArrayType.INSTANCE;
        } else if (long.class == type) {
            return (ArrayType<T>) LongArrayType.INSTANCE;
        } else if (short.class == type) {
            return (ArrayType<T>) ShortArrayType.INSTANCE;
        } else {
            throw new IllegalArgumentException("Unknown type: " + type);
        }
    }

    Class<T> type(); // TODO: rename to componentType
    int lengthOf(Object array);
    T getAt(Object array, int index);

    Object empty();
    void setAt(Object array, int index, Object value);
    Object copy(Object array, int arraySize, int sourceFrom, int destinationFrom, int size);

    default Object newInstance(int length) { return copy(empty(), length); }

    /** System.arrayCopy with same source and destination */
    default Object copyRange(Object array, int from, int to) {
        final int length = to - from;
        return copy(array, length, from, 0, length);
    }

    /** Repeatedly group an array into equal sized sub-trees */
    default Object grouped(Object array, int groupSize) {
        final int arrayLength = lengthOf(array);
        assert arrayLength > groupSize;
        final Object results = obj().newInstance(1 + ((arrayLength - 1) / groupSize));
        obj().setAt(results, 0, copyRange(array, 0, groupSize));

        for (int start = groupSize, i = 1; start < arrayLength; i++) {
            final int nextLength = Math.min(groupSize, arrayLength - (i * groupSize));
            obj().setAt(results, i, copyRange(array, start, start + nextLength));
            start += nextLength;
        }

        return results;
    }

    /** clone the source and set the value at the given position */
    default Object copyUpdate(Object array, int index, T element) {
        final Object copy = copy(array, index + 1);
        setAt(copy, index, element);
        return copy;
    }

    default Object copy(Object array, int minLength) {
        final int arrayLength = lengthOf(array);
        final int length = Math.max(arrayLength, minLength);
        return copy(array, length, 0, 0, arrayLength);
    }

    /** clone the source and keep everything after the index (pre-padding the values with null) */
    default Object copyDrop(Object array, int index) {
        final int length = lengthOf(array);
        return copy(array, length, index, index, length - index);
    }

    /** clone the source and keep everything before and including the index */
    default Object copyTake(Object array, int lastIndex) {
        return copyRange(array, 0, lastIndex + 1);
    }

    /** Create a single element array */
    default Object asArray(T element) {
        final Object result = newInstance(1);
        setAt(result, 0, element);
        return result;
    }

    /** Store the content of an iterable in an array */
    static Object[] asArray(java.util.Iterator<?> it, int length) {
        final Object[] array = new Object[length];
        for (int i = 0; i < length; i++) {
            array[i] = it.next();
        }
        return array;
    }

    @SuppressWarnings("unchecked")
    static <T> T asPrimitives(Class<?> primitiveClass, Iterable<?> values) {
        final Object[] array = Array.ofAll(values).toJavaArray();
        assert (array.length == 0) || (primitiveClass == primitiveType(array[0])) && !primitiveClass.isArray();
        final ArrayType<T> type = of((Class<T>) primitiveClass);
        final Object results = type.newInstance(array.length);
        for (int i = 0; i < array.length; i++) {
            type.setAt(results, i, array[i]);
        }
        return (T) results;
    }

    static <T> Class<?> primitiveType(T element) {
        final Class<?> wrapper = (element == null) ? Object.class : element.getClass();
        if (wrapper == Boolean.class) {
            return boolean.class;
        } else if (wrapper == Byte.class) {
            return byte.class;
        } else if (wrapper == Character.class) {
            return char.class;
        } else if (wrapper == Double.class) {
            return double.class;
        } else if (wrapper == Float.class) {
            return float.class;
        } else if (wrapper == Integer.class) {
            return int.class;
        } else if (wrapper == Long.class) {
            return long.class;
        } else if (wrapper == Short.class) {
            return short.class;
        } else {
            return wrapper;
        }
    }

    final class IntArrayType implements ArrayType<Integer>, Serializable {

        private static final long serialVersionUID = 1L;

        private static final IntArrayType INSTANCE = new IntArrayType();
        private static final int[] EMPTY = new int[0];

        private static int[] cast(Object array) { return (int[]) array; }

        @Override
        public Class<Integer> type() { return int.class; }

        @Override
        public int[] empty() { return EMPTY; }

        @Override
        public int lengthOf(Object array) { return (array == null) ? 0 : cast(array).length; }

        @Override
        public Integer getAt(Object array, int index) { return cast(array)[index]; }

        @Override
        public void setAt(Object array, int index, Object value) { cast(array)[index] = (Integer) value; }

        @Override
        public Object copy(Object array, int arraySize, int sourceFrom, int destinationFrom, int size) {
            if (size == 0) {
                return new int[arraySize];
            } else {
                final int[] result = new int[arraySize];
                System.arraycopy(array, sourceFrom, result, destinationFrom, size); /* has to be near the object allocation to avoid zeroing out the array */
                return result;
            }
        }
    }

}
Show outdated Hide outdated javaslang/src/main/java/javaslang/collection/ArrayType.java
} else {
throw new IllegalArgumentException("Unknown type: " + type);
}
}

This comment has been minimized.

@danieldietrich

danieldietrich Oct 30, 2016

Member

See suggested code in the comment above.

@danieldietrich

danieldietrich Oct 30, 2016

Member

See suggested code in the comment above.

array = grouped(array, BRANCHING_FACTOR);
for (ArrayType<T> t = type; t.lengthOf(array) > BRANCHING_FACTOR; shift += BRANCHING_BASE) {
array = t.grouped(array, BRANCHING_FACTOR);
t = obj();

This comment has been minimized.

@danieldietrich

danieldietrich Oct 30, 2016

Member

had to look a moment at this for-loop :)

@danieldietrich

danieldietrich Oct 30, 2016

Member

had to look a moment at this for-loop :)

This comment has been minimized.

@paplorinc

paplorinc Oct 30, 2016

Contributor

yeah, trying to achieve a good signal-to-noise ratio in a language that doesn't want me to :)

@paplorinc

paplorinc Oct 30, 2016

Contributor

yeah, trying to achieve a good signal-to-noise ratio in a language that doesn't want me to :)

final Object previous = array[previousIndex];
final Object[] newNode = node.apply(previous, offset);
array[previousIndex] = newNode;
final Object previous = obj().getAt(array, previousIndex);

This comment has been minimized.

@danieldietrich

danieldietrich Oct 30, 2016

Member

The usages of obj() are OK!

@danieldietrich

danieldietrich Oct 30, 2016

Member

The usages of obj() are OK!

@danieldietrich

This comment has been minimized.

Show comment
Hide comment
@danieldietrich

danieldietrich Oct 30, 2016

Member

@paplorinc

Mostly done - and pushed -, not sure about embedding them to the same file, though.
What's the advantage exactly, are we trying to simulate a sealed trait in Java this way?

Yes, it is some kind of sealed trait behavior. It is a kind of coding style I applied in different places of Javaslang. I want to keep together things that belong together. It is some kind of local module. The implementation types are never used directly, like List.Cons & List.Nil. Let's apply this change and align to the other classes.

Member

danieldietrich commented Oct 30, 2016

@paplorinc

Mostly done - and pushed -, not sure about embedding them to the same file, though.
What's the advantage exactly, are we trying to simulate a sealed trait in Java this way?

Yes, it is some kind of sealed trait behavior. It is a kind of coding style I applied in different places of Javaslang. I want to keep together things that belong together. It is some kind of local module. The implementation types are never used directly, like List.Cons & List.Nil. Let's apply this change and align to the other classes.

@paplorinc

This comment has been minimized.

Show comment
Hide comment
@paplorinc

paplorinc Oct 30, 2016

Contributor

Please just nest the *ArrayType into ArrayType as discussed in the original PR.

Is there a non-hacky way for this, I'm not proud of how this code generation part turned out :/

Contributor

paplorinc commented Oct 30, 2016

Please just nest the *ArrayType into ArrayType as discussed in the original PR.

Is there a non-hacky way for this, I'm not proud of how this code generation part turned out :/

@danieldietrich

This comment has been minimized.

Show comment
Hide comment
@danieldietrich

danieldietrich Oct 30, 2016

Member

Is there a non-hacky way for this, I'm not proud of how this code generation part turned out :/

Currently we need to do it the hacky way :-/

When we move to Gradle I hope there is a better Scala integration. We will split the generator in several parts (one for ArrayType, one for Functions, one for Tuple, etc.)

Member

danieldietrich commented Oct 30, 2016

Is there a non-hacky way for this, I'm not proud of how this code generation part turned out :/

Currently we need to do it the hacky way :-/

When we move to Gradle I hope there is a better Scala integration. We will split the generator in several parts (one for ArrayType, one for Functions, one for Tuple, etc.)

@paplorinc

This comment has been minimized.

Show comment
Hide comment
@paplorinc

paplorinc Oct 30, 2016

Contributor

@danieldietrich, I tried, but I can't convince Scala.
Could you please do it after merging this :)?
Is there anything else you would like me to address?

Contributor

paplorinc commented Oct 30, 2016

@danieldietrich, I tried, but I can't convince Scala.
Could you please do it after merging this :)?
Is there anything else you would like me to address?

@danieldietrich

This comment has been minimized.

Show comment
Hide comment
@danieldietrich

danieldietrich Oct 30, 2016

Member

@paplorinc

Could you please do it after merging this :)?

Will do that, Sir. Compared to your great ideas that materialized in this PR this looks like a warm, mild summer breeze to me :)

Member

danieldietrich commented Oct 30, 2016

@paplorinc

Could you please do it after merging this :)?

Will do that, Sir. Compared to your great ideas that materialized in this PR this looks like a warm, mild summer breeze to me :)