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.

Sign up for GitHub

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

Jump to bottom

Patch maps better #328

Closed
wants to merge 3 commits into from
Closed

Patch maps better #328

wants to merge 3 commits into from

Conversation

treeowl
Copy link
Contributor

@treeowl treeowl commented Jul 16, 2019

Use the containers merge interface to improve efficiency.

Make strictness consistent.

treeowl added 2 commits July 16, 2019 15:29
@treeowl
Patch maps better
f78211a 
* Use the `merge` API from `containers` to perform each merge
  in one go.

* Use lazy merge operations for both; this is more consistent.
@treeowl
Add useless-deletion detection for Map
42517f7 
If we decide to include this, we can use it for `IntMap` too.
@treeowl
Copy link
Contributor Author

treeowl commented Jul 16, 2019

I did a fancy useless-deletion detection thing for Map; I can do the same for IntMap if that's how we want to go. It would also be possible to try to detect useless updates, but it'll be pretty unreliable for many important cases. We could add a PatchMap type that insists on its values being Eq, but would that be useful?

3noch
3noch reviewed Jul 16, 2019
View reviewed changes
src/Reflex/Patch/Map.hs
data Changed a
= Unchanged a
| Changed a
deriving (Functor)
Copy link
Member

Choose a reason for hiding this comment

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

Are there any speed benefits to making this newtype Changed a = Changed (ChangedTag, a)? Or does that, as I imagine, compile down to the same thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See my comment from a minute ago.

Copy link
Contributor

Choose a reason for hiding this comment

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

Lazy pairs are kind of challenging to compile, so yeah, it's really between what @treeowl suggested and what's already written. (But I don't think this issue should block anything on this MR.)

treeowl
treeowl commented Jul 16, 2019
View reviewed changes
src/Reflex/Patch/Map.hs
data Changed a
= Unchanged a
| Changed a
deriving (Functor)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I change this to data Changed a = Changed !Bool a or similar? I'll have to look at Core to see if GHC will unbox it nicely in that case. Might be an improvement.

@cgibbard
Copy link
Collaborator

Haha, this kind of thing is exactly why I got you to make Data.Map.Merge in the first place, it's kind of funny that you'd also end up helping us replace our old awkward merges with it. :D

@lpsmith lpsmith added clean up Removes stale code, improves read-ability, or small refactors enhancement non-breaking change labels Dec 20, 2019
@Ericson2314 Ericson2314 mentioned this pull request Jan 8, 2020
Open
@Ericson2314
Copy link
Member

Moved to reflex-frp/patch#3

@Ericson2314 Ericson2314 closed this Jan 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@3noch 3noch 3noch left review comments

@lpsmith lpsmith lpsmith left review comments

Assignees
No one assigned
Labels
clean up Removes stale code, improves read-ability, or small refactors enhancement non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@treeowl @cgibbard @Ericson2314 @lpsmith @3noch