Skip to content
This repository has been archived by the owner on Feb 8, 2022. It is now read-only.

Improve the performance of Map's algebras. #157

Merged
merged 2 commits into from
Aug 21, 2016
Merged

Conversation

non
Copy link
Contributor

@non non commented Aug 21, 2016

We noticed an issue in cats-kernel where operations over Maps were
incredibly slow. This commit ports those fixes over here, where our
additive/multiplicative monoids experienced the same issues.

Review by @johnynek

We noticed an issue in cats-kernel where operations over Maps were
incredibly slow. This commit ports those fixes over here, where our
additive/multiplicative monoids experienced the same issues.
@non non mentioned this pull request Aug 21, 2016
else if (n == 0) zero
else throw new IllegalArgumentException("Illegal negative exponent to sumN: %s" format n)

override def sum(as: TraversableOnce[Map[K, V]]): Map[K, V] = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't have trySum here:

https://github.com/non/algebra/blob/master/ring/src/main/scala/algebra/ring/Additive.scala#L32

can we do the same override on AdditiveMonoid or here so that trySum calls sum?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

@johnynek
Copy link
Contributor

minor comment about trySum.

This mirrors a similar change in cats-kernel. The basic idea is that
if someone goes to the trouble of writing an optimized .sum method
we'd like for .trySum to use it as well.
if (xs.size <= ys.size) {
timesMap(xs, ys)((x, y) => V.times(x, y))
xs.foldLeft(Map.empty[K, V]) { case (m, (k, x)) =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if Map.empty makes sense here or if we should use the mutable approach? Like, we could allocate an empty MMap about the size of the smallest entry (an upperbound).

I think this will be faster, if we care, but we can come back to it later if you prefer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do this now, and then make that change with a benchmark later.

@johnynek
Copy link
Contributor

👍

@johnynek johnynek merged commit 2c7e5d1 into master Aug 21, 2016
@johnynek johnynek deleted the topic/speed-up-map branch August 21, 2016 22:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants