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

Map performance - use pattern match on size to reduce recursive function calls #1069

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

Conversation

HuwCampbell
Copy link

@HuwCampbell HuwCampbell commented Nov 13, 2024

This change incorporates the idea already used for foldMap and traverse, where we
pattern match on the size of "leaf nodes". If the size is 1, we already know both sides
are tips, and we don't need to recurse.

Of particular interest is mapMaybe, where we can also omit the extra function calls to link.

Benchmarks show improvements around 10 to 20% for most operations affected,
though foldl' seems closer to 30%.

❯ ./map-benchmarks-with-these-changes
All
  map:             OK
    53.6 μs ± 3.8 μs
  map really:      OK
    128  μs ± 7.5 μs
  mapWithKey:      OK
    66.4 μs ± 4.1 μs
  foldlWithKey:    OK
    796  μs ±  36 μs
  foldlWithKey':   OK
    17.7 μs ± 909 ns
  foldrWithKey:    OK
    61.1 ns ± 4.7 ns
  foldrWithKey':   OK
    34.9 μs ± 3.4 μs
  mapMaybe:        OK
    112  μs ± 7.9 μs
  mapMaybeWithKey: OK
    111  μs ± 2.4 μs
  filter:        OK
    49.5 μs ± 4.3 μs
  filter really: OK
    70.5 μs ± 4.1 μs

❯ ./map-benchmarks-as-per-master    
All
  map:             OK
    65.0 μs ± 5.5 μs
  map really:      OK
    122  μs ±  10 μs
  mapWithKey:      OK
    76.1 μs ± 6.6 μs
  foldlWithKey:    OK
    821  μs ±  30 μs
  foldlWithKey':   OK
    24.9 μs ± 1.5 μs
  foldrWithKey:    OK
    64.5 ns ± 4.0 ns
  foldrWithKey':   OK
    37.2 μs ± 1.8 μs
  mapMaybe:        OK
    134  μs ± 8.1 μs
  mapMaybeWithKey: OK
    131  μs ± 7.3 μs
  filter:        OK
    62.0 μs ± 5.7 μs
  filter really: OK
    81.0 μs ± 4.7 μs

Some (very) cursory analysis says that about 1/3 of Bin nodes in a tree should have no
children, hence changes up to 30% do seem plausible.

Pattern matching on a strict and unboxed Int should be very fast.

@meooow25
Copy link
Contributor

Thanks for testing this out. The improvements seem decent but I've always found this undocumented special case strange and I'm not sure we want to proliferate this pattern. If we really want to avoid visiting Tips why not do it properly? For foldMap this can look like

foldMap :: Monoid m => (a -> m) -> Set a -> m
foldMap _ Tip = mempty
foldMap f (Set _ x l r) = go x l r
  where
    go x l r = case (l, r) of
      (Bin _ lx ll lr, Bin _ rx rl rr) -> go lx ll lr <> f x <> go rx rl rr
      (Bin _ lx ll lr, Tip) -> go lx ll lr <> f x
      (Tip, Bin rx rl rr) -> f x <> go rx rl rr
      (Tip, Tip) -> f x

But I haven't measured the effects of such a change.

@HuwCampbell
Copy link
Author

HuwCampbell commented Nov 14, 2024

The version you posted is about 10 to 15% slower than what's there already. See this commit which adds the change it to foldMap.

It's worth asking why it's slower – it seems that pattern matching on the strict packed integer should be very fast, as there's no pointer chasing or indirection involved, pattern matching on the tips does require some indirection.

From the comment in the link above, eliding 2/3 of the recursive calls to Tip is a big deal, because there are a lot of Tips, effectively about the same number as the number of items in the map.

Maybe we should add a GHC style note about this optimisation.

Edit: I've written one.

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

Successfully merging this pull request may close these issues.

2 participants