-
Notifications
You must be signed in to change notification settings - Fork 4
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
Performance #8
Comments
CC @Gabriel439 |
The (huge) normalized expression can be found at https://github.com/dhallix/nix-derivation/blob/gcc/nf.dhall |
@ocharles: I can think of two possible ways to solve this: Approach 1: Use lamping's algorithm for normalization in Dhall so that it can internally represent the abstract syntax tree as a graph so that it doesn't need to materialize the entire result except to display to the user. This is a significant amount of work. Approach 2: Use the support for custom type-checking and custom normalization so that |
Approach 2 has the same problems that always come up with this, namely siloing yourself from any other Dhall tool in the field. That does feel like a shame for something like |
@ocharles: Right, the graph representation is the way to avoid repeated work and Lamping's algorithm is the most efficient graph representation that I know of. I also believe it is an algorithm that's amenable to parallelization, too (cc: @MaiaVictor to confirm) Given that this is quite a bit of work to I'll probably have to set up a KickStarter or something similar to fund somebody to do this because I know that I won't have the bandwidth to do this myself |
While that might be the most optimal it is, as you say, a lot of work. My approach would be to do a pass on the input to |
Unfortunately even adding |
@ocharles: That seems unusual. I think you'll have to profile it to see what is the bottleneck |
I'll have a look in the week. I took out the Nix specific stuff and switched to Dhall 1.18. Going from |
Yeah, what makes this weird is that it seems like the AST should be small from what I've skimmed so far, unless it is duplicating calls to |
The normal form is still pretty huge - 6MB formatted: https://github.com/dhallix/nix-derivation/blob/builtin-derivation/normal-form.dhall |
I'm not sure what you mean about |
Ah, okay, now I see why you didn't get a performance speed up. What I meant was to use |
I actually don't think it's normalization that is the problem. If I take |
Actually, I seem to be getting a normal form even if I only do expr' <-
StateT.evalStateT
(Dhall.Import.loadWith expr)
(Dhall.Import.emptyStatus "."
& Dhall.Import.startingContext .~ Dhallix.context)
case Dhall.TypeCheck.typeWith Dhallix.context expr' of
Left err -> Control.Exception.throwIO err
Right _ -> return ()
print (Dhall.Pretty.prettyExpr expr'))) Does importing now do normalization? |
@ocharles: Yes |
Ok, then this is going to be much harder - doable, but harder. That's because normalisation of a call to the builtin I was hoping that I could just plug in the above extra case to I'll proceed in the direction of directly building |
@ocharles: Actually, there is a trick we might be able to apply here. Detecting common subexpressions in general is going to be hard if we consider all possible subexpressions, but in this case we don't need to consider all possible subexpressions. We only need to factor out common subexpressions that are derivations (i.e. a saturated call to the
|
This sounds interesting, but where would this be done? Is this something that would be plugged into |
@ocharles: Yeah, you're right. This would instead require a performance improvement to the normalization engine. At best the trick could be used to minimize the rendered output, but wouldn't improve normalization speed. |
Ok, we now have derivation
{ environment =
[ { name = "dep-1", value = derivation dep-1-args }
, { name = "dep-2", value = derivation dep-2-args }
]
} Here our expression is a derivation, and it has two derivations as children. Let's consider
The best option actually seems to be able to enter my custom normalizer top-down. Whenever I see a Before I go ahead and add an extra call in |
@ocharles: I believe what you want is to use |
@Gabriel439 That requires a top-down traversal rather than bottom up, unless I'm missing something. |
To clarify, listen :: Monad m => WriterT w m a -> WriterT w m (a, w) So |
@ocharles: Oh yeah, you're right. |
Right, but normalizeWithM would need to be changed to have a top down recursive call before anything else. If you're ok with that, I'll add it |
@ocharles: What do you mean by a top-down recursive call? |
I mean that it needs to call the custom The current algorithm only calls the context function at the leaves in a bottom-up fashion. There is no way for me to perform what I'm calling top-down recursion. Does that make sense? |
@ocharles: Ah, yes, I get it now! Yeah, I would be fine with this if it doesn't significantly degrade performance. My intuition is that if we only apply the custom |
Alright, cool! I'll throw something up soon. |
I still don't think this is possible, because I'm unable to use
derivation { builder = ./bash.dhall, ... }
derivation { ... } -- No further imports or derivations If I try and build the expression The rough flow is thus:
Does that make sense? You can witness the problem if you build http://github.com/ocharles/dhall-build and then create a Dhall file that points to, say, I'll double check my reasoning tomorrow, but I'm pretty convinced this is correct. |
@ocharles: Yeah, now that you mention that I think you are going to hit a wall since we don't have a good way for effects to cross import boundaries. Maybe the way to go here is to do what you originally proposed to:
|
That's actually going to be really messy. I would have to scan all arguments to This is doable but feels really unfortunate - the information is right there in a much more structured form! It's really the repeated normalisation that's causing problems, but not sure if that can be changed |
@Gabriel439 did you see my last comment? I've been thinking about this a bit more and I still can't see a good way to do this. The only thing I can think of that wouldn't be awful would be to generalise loading from |
It's that, or maybe we wait until standard normalization is better and the original approach can be used (which is certainly the most conceptually simple from my side). |
Yeah, I think making standard normalization faster is going to be the way to go here. If Dhall is going to replace Nix then it needs to be as fast as the Nix interpreter (and possibly as lazy) |
@ocharles Hi, could you perhaps update the Preliminarily, on this file type checking and normalization is about 100 times faster on the new branch, and now command time is dominated by networking and expression rendering. If you check the branch out, I shall note that basically only |
See the gcc-latest-dhall branch |
@ocharles thanks. I'm happy to tell you that |
Woah, this project just became viable again! |
Can you share your branch? |
Do you mean the link: https://github.com/dhall-lang/dhall-haskell/tree/nbe-elaboration, or is there some git sharing thing which I don't know about (being a git/github dummy)? |
Nope, that's what I was looking for! I had only seen your work that got merged into |
For anyone wondering, on Dhall 1.30.0 this takes like 0.35s. I think the issue can be regarded as fixed. |
Ok, interesting! I thought we never got all the improvements merged. I will revisit before closing. |
IIUC not all the improvements are merged indeed, but a bunch more of them were in the meantime - amongst them, apparently, those that fixed the performance for this case. |
I agree, this is certainly better than minutes. I get 0.3s on Dhall 1.32. I think we can safely close this now. |
The
gcc
branch gives us./example/gcc.dhall
, which is used to build GCC. This has 3 dependencies - GMP, MPFR and MPC. However, just evaluatinggcc.dhall
takes minutes:Applying
dhall freeze
everywhere actually makes things worse:The text was updated successfully, but these errors were encountered: