Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

use a Vector as accumulator of map.traverse (#1633) #1671

Merged
merged 1 commit into from
Nov 11, 2016
Merged

use a Vector as accumulator of map.traverse (#1633) #1671

merged 1 commit into from
Nov 11, 2016

Conversation

mduesterhoeft
Copy link
Contributor

fixes #1633
was it really that easy or did I overlook something?!

The measurement looks a lot better now:

Created list with 100 entries in 111 ms
	116	1.16	0.012
Traverse after 129 ms
	129	1.29	0.013
Sum is 333300 after 131 ms
	131	1.31	0.013
Created list with 400 entries in 2 ms
	2	0.005	0
Traverse after 5 ms
	5	0.013	0
Sum is 21333200 after 6 ms
	6	0.015	0
Created list with 1600 entries in 3 ms
	3	0.002	0
Traverse after 10 ms
	10	0.006	0
Sum is 1365332800 after 12 ms
	12	0.007	0
Created list with 6400 entries in 9 ms
	9	0.001	0
Traverse after 19 ms
	19	0.003	0
Sum is 87381331200 after 21 ms
	21	0.003	0
Created list with 25600 entries in 25 ms
	25	0.001	0
Traverse after 47 ms
	47	0.002	0
Sum is 5592405324800 after 52 ms
	53	0.002	0
Created list with 102400 entries in 90 ms
	90	0.001	0
Traverse after 131 ms
	131	0.001	0
Sum is 22073268555776 after 137 ms
	137	0.001	0
Created list with 409600 entries in 314 ms
	314	0.001	0
Traverse after 516 ms
	516	0.001	0
Sum is 16266472669184 after 576 ms
	576	0.001	0

@ruslansennov
Copy link
Member

ruslansennov commented Nov 11, 2016

Could you make measurements with another implementation of traverse (Daniel pointed at it in issue comments) when we use List.prepend() on every element and List.reverse() at the end?
Vector is awesome structure, but it requires more memory than List

@codecov-io
Copy link

Current coverage is 96.45% (diff: 100%)

Merging #1671 into master will not change coverage

@@             master      #1671   diff @@
==========================================
  Files            89         89          
  Lines         11187      11187          
  Methods           0          0          
  Messages          0          0          
  Branches       1881       1881          
==========================================
  Hits          10790      10790          
  Misses          243        243          
  Partials        154        154          

Powered by Codecov. Last update b9f2263...662cdd7

@mduesterhoeft
Copy link
Contributor Author

mduesterhoeft commented Nov 11, 2016

@ruslansennov you are right it is interesting to also look at these results. With a list and the following traversal I get similar results:

List<U> result = foldLeft(List.empty(), (acc, entry) -> acc.prepend(mapper.apply(entry._1, entry._2)));
return result.reverse();
Created list with 100 entries in 129 ms
    138 1.38    0.014
Traverse after 145 ms
    145 1.45    0.015
Sum is 333300 after 145 ms
    146 1.46    0.015
Created list with 400 entries in 2 ms
    2   0.005   0
Traverse after 3 ms
    3   0.007   0
Sum is 21333200 after 4 ms
    4   0.01    0
Created list with 1600 entries in 3 ms
    4   0.003   0
Traverse after 8 ms
    8   0.005   0
Sum is 1365332800 after 8 ms
    9   0.006   0
Created list with 6400 entries in 9 ms
    10  0.002   0
Traverse after 14 ms
    14  0.002   0
Sum is 87381331200 after 15 ms
    15  0.002   0
Created list with 25600 entries in 24 ms
    24  0.001   0
Traverse after 34 ms
    34  0.001   0
Sum is 5592405324800 after 35 ms
    35  0.001   0
Created list with 102400 entries in 94 ms
    94  0.001   0
Traverse after 108 ms
    108 0.001   0
Sum is 22073268555776 after 110 ms
    110 0.001   0
Created list with 409600 entries in 452 ms
    452 0.001   0
Traverse after 528 ms
    528 0.001   0
Sum is 16266472669184 after 541 ms
    542 0.001   0

So which way would you recommend @ruslansennov @danieldietrich

@danieldietrich danieldietrich added this to the 2.0.5 milestone Nov 11, 2016
@danieldietrich
Copy link
Contributor

Hi @mduesterhoeft, thank you for the fast impl! If you have room (and appetite) for more, than you could take a look at #1600.

Hi @ruslansennov, that's right, List has a smaller mem footprint. In the end it is the decision what should be the default Seq impl for Javaslang - a LinearSeq or an IndexedSeq. Alvin Alexander, the author of Scala Cookbook suggests to use Vector as default Seq (in Scala). We should apply that suggestion to Javaslang. Vector is general purpose. As this 'bug' shows, the time complexity highly depends on the type of the collection. We reduced the complexity from n^2 to n by just switching to Vector. Having a reliable time complexity is more important that saving bytes.

I will pull this one now, many thanks!

@danieldietrich danieldietrich merged commit a73d49f into vavr-io:master Nov 11, 2016
@mduesterhoeft
Copy link
Contributor Author

@danieldietrich thanks for the suggestion and the help - I would like look at something more complex. So I am definitely eager to help with #1600

@ruslansennov
Copy link
Member

@danieldietrich this code has complexity O(n)

List<U> result = foldLeft(List.empty(), (acc, entry) -> acc.prepend(mapper.apply(entry._1, entry._2)));
return result.reverse();

And @mduesterhoeft shows similar results.

But OK

@danieldietrich
Copy link
Contributor

@ruslansennov yes, that's right. For the user it is better if the returned Seq is eff. O(1) for operations like append. Therefor we should choose Vector as default general purpose impl for Seq return types. (Nevertheless this is no dogma, we have to decide it from case to case.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Map.traverse performance is non-linear.
4 participants