Skip to content

alter could be much more efficient #392

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

Open
sjakobi opened this issue Mar 23, 2022 · 2 comments · Fixed by #404
Open

alter could be much more efficient #392

sjakobi opened this issue Mar 23, 2022 · 2 comments · Fixed by #404

Comments

@sjakobi
Copy link
Member

sjakobi commented Mar 23, 2022

alter :: (Eq k, Hashable k) => (Maybe v -> Maybe v) -> k -> HashMap k v -> HashMap k v
-- TODO(m-renaud): Consider using specialized insert and delete for alter.
alter f k m =
case f (lookup k m) of
Nothing -> delete k m
Just v -> insert k v m
{-# INLINABLE alter #-}

Currently this code will attempt to delete keys that aren't present in the first place. Inserts could be more efficient too, compare alterFEager:

alterFEager :: (Functor f, Eq k, Hashable k)
=> (Maybe v -> f (Maybe v)) -> k -> HashMap k v -> f (HashMap k v)
alterFEager f !k m = (<$> f mv) $ \case
------------------------------
-- Delete the key from the map.
Nothing -> case lookupRes of
-- Key did not exist in the map to begin with, no-op
Absent -> m
-- Key did exist
Present _ collPos -> deleteKeyExists collPos h k m
------------------------------
-- Update value
Just v' -> case lookupRes of
-- Key did not exist before, insert v' under a new key
Absent -> insertNewKey h k v' m
-- Key existed before
Present v collPos ->
if v `ptrEq` v'
-- If the value is identical, no-op
then m
-- If the value changed, update the value.
else insertKeyExists collPos h k v' m
where !h = hash k
!lookupRes = lookupRecordCollision h k m
!mv = case lookupRes of
Absent -> Nothing
Present v _ -> Just v
{-# INLINABLE alterFEager #-}

The Strict version has the same problems.

@sjakobi
Copy link
Member Author

sjakobi commented Apr 12, 2022

Even with the optimizations added in #404, we end up walking the tree twice. It would certainly be more efficient to walk the tree once, similarly to insert or delete.

@sjakobi
Copy link
Member Author

sjakobi commented May 10, 2022

It would certainly be more efficient to walk the tree once, similarly to insert or delete.

Re-opening to keep track of this idea.

@sjakobi sjakobi reopened this May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant