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

Improve performance of type-checking record literals #1048

Merged
merged 2 commits into from
Jul 2, 2019

Conversation

Gabriella439
Copy link
Collaborator

While benchmarking the example from #769 I saw that a significant
amount of time was spent benchmarking record literals. When I looked
at the code more closely I saw that the first key in the record literal
was being type-checked twice (once to figure out the record's associated
type-checking constant and once as part of the process loop).

This change fixes that, which speeds up interpretation of the large
example by 9%:

Before:

time                 18.13 s    (18.11 s .. 18.16 s)
                     1.000 R²   (1.000 R² .. 1.000 R²)
mean                 18.09 s    (18.07 s .. 18.11 s)
std dev              21.92 ms   (10.66 ms .. 29.76 ms)
variance introduced by outliers: 19% (moderately inflated)

After:

time                 16.53 s    (16.49 s .. 16.60 s)
                     1.000 R²   (1.000 R² .. 1.000 R²)
mean                 16.59 s    (16.56 s .. 16.64 s)
std dev              43.65 ms   (6.227 ms .. 56.35 ms)
variance introduced by outliers: 19% (moderately inflated)

This also includes a small change to use a new unorderedTraverseWithKey
utility instead of traverseWithKey. This appears to have no effect on
performance, but I did this anyway so that I could be sure that any
slowness in this code is not related to any Dhall.Map-specific weirdness.

While benchmarking the example from #769 I saw that a significant
amount of time was spent benchmarking record literals.  When I looked
at the code more closely I saw that the first key in the record literal
was being type-checked twice (once to figure out the record's associated
type-checking constant and once as part of the `process` loop).

This change fixes that, which speeds up interpretation of the large
example by 9%:

Before:

```
time                 18.13 s    (18.11 s .. 18.16 s)
                     1.000 R²   (1.000 R² .. 1.000 R²)
mean                 18.09 s    (18.07 s .. 18.11 s)
std dev              21.92 ms   (10.66 ms .. 29.76 ms)
variance introduced by outliers: 19% (moderately inflated)
```

After:

```
time                 16.53 s    (16.49 s .. 16.60 s)
                     1.000 R²   (1.000 R² .. 1.000 R²)
mean                 16.59 s    (16.56 s .. 16.64 s)
std dev              43.65 ms   (6.227 ms .. 56.35 ms)
variance introduced by outliers: 19% (moderately inflated)
```
@sjakobi
Copy link
Collaborator

sjakobi commented Jul 2, 2019

Nice! I'm wondering whether we should try to do the same with Union:

loop ctx e@(Union kts ) = do
let nonEmpty k mt = First (fmap (\t -> (k, t)) mt)
case getFirst (Dhall.Map.foldMapWithKey nonEmpty kts) of
Nothing -> do
return (Const Type)
Just (k0, t0) -> do
s0 <- fmap Dhall.Core.normalize (loop ctx t0)
c0 <- case s0 of
Const c0 -> do
return c0
_ -> do
Left (TypeError ctx e (InvalidAlternativeType k0 t0))
let process _ Nothing = do
return ()
process k (Just t) = do
s <- fmap Dhall.Core.normalize (loop ctx t)
c <- case s of
Const c -> do
return c
_ -> do
Left (TypeError ctx e (InvalidAlternativeType k t))
if c0 == c
then return ()
else Left (TypeError ctx e (AlternativeAnnotationMismatch k t c k0 t0 c0))
Dhall.Map.unorderedTraverseWithKey_ process kts

It's a bit more complicated though since k0 isn't necessarily the first key of kts. So we'd have to find k0's position and re-insert it at the same position again. Or we could do something terrible like deleting it only from the Data.Map but leaving it in the list of keys… 😈

@Gabriella439
Copy link
Collaborator Author

@sjakobi: I'm going to do that separately, because I want to make sure that I have an example benchmark that shows that the change is an improvement before making the change.

@sjakobi
Copy link
Collaborator

sjakobi commented Jul 2, 2019

@Gabriel439 No worries. I doubt that it offers similar performance gains as the RecordLit case. Should I open an issue to track the idea?

@Gabriella439
Copy link
Collaborator Author

@sjakobi: Yeah, we can track that in a separate issue

@sjakobi sjakobi merged commit f5819dd into master Jul 2, 2019
@sjakobi sjakobi deleted the gabriel/type_record_lit branch July 2, 2019 16:42
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.

None yet

2 participants