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

Dhall.Map: Forget order of keys even harder #1066

Merged
merged 2 commits into from
Jul 7, 2019

Conversation

sjakobi
Copy link
Collaborator

@sjakobi sjakobi commented Jul 4, 2019

So far, this speeds up the "issue 412" benchmark by ~9% and "union performance"
by ~2%.

I expect further speedups when we improve the Map usage in e.g. Dhall.Eval. We could also try ignoring the original key order right from the beginning for the commands where we don't care about it, like normalization.

55e222b is cherrypicked from #1049.

@EggBaconAndSpam
Copy link
Collaborator

EggBaconAndSpam commented Jul 4, 2019

Wouldn't it make more sense to wait for the NbE-elaboration stuff before working on micro-optimisations? See @ocharles's benchmark results (here); he gets a multiple-order-of-magnitude speedup.

@Gabriella439
Copy link
Collaborator

@EggBaconAndSpam: it is definitely still worth doing, for a few reasons:

  • Doing so will make it easier to identify and fix other performance bottlenecks that we might not be aware of yet

  • We will probably need to do this anyway even after we merge the much faster type checker. Users have a tendency to push the language to it's performance limits regardless of how fast we make things

  • Issue 412 is an example of an expression that might not see order-of-magnitude improvement from the faster type-checking branch (it might only be 4x, which is good but still needs improvement)

  • 9% is actually quite a good improvement for such a small change

@sjakobi
Copy link
Collaborator Author

sjakobi commented Jul 4, 2019

Are these all the dhall commands that need to preserve ordering of fields and constructors?

  • resolve
  • diff
  • lint
  • format
  • freeze
  • decode

That would leave the following commands that could forget about the ordering early:

  • type
  • normalize
  • repl
  • hash
  • encode

How early? I'm not sure whether to try messing with the parser to control Dhall.Parser.Combinators.toMap. But already the import resolution might be slightly quicker if doesn't have to traverse maps in their original order.

I guess I don't have a good idea of dhalls overall structure. Do you have an idea where a "forget-key-order" step could fit in @Gabriel439?

@Gabriella439
Copy link
Collaborator

This looks good to me as is. I'm not sure if you still have any additional changes planned for this pull request

@sjakobi sjakobi added merge me and removed merge me labels Jul 6, 2019
@Gabriella439
Copy link
Collaborator

@sjakobi: Sorry, I missed your earlier question. Only dhall {lint,format,freeze} need to preserve field order. Everything else can freely reorder fields if it's more efficient that way

Also add the unorderedSingleton and unorderedFromList functions.

This speeds up the "issue 412" benchmark by ~9% and "union performance"
by ~2%.
@sjakobi sjakobi force-pushed the sjakobi/optional-map-keys branch from 0a53ed3 to 8cfb726 Compare July 6, 2019 23:32
@sjakobi sjakobi changed the title WIP: Dhall.Map: Forget order of keys even harder Dhall.Map: Forget order of keys even harder Jul 6, 2019
@sjakobi
Copy link
Collaborator Author

sjakobi commented Jul 6, 2019

@Gabriel439 Thanks, that's good to know! I'll probably try to hook into Dhall.Parser.Combinators.toMap so it doesn't produce the keys list for the other commands.

@mergify mergify bot merged commit 3b4f826 into master Jul 7, 2019
@sjakobi sjakobi deleted the sjakobi/optional-map-keys branch July 7, 2019 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants