- User Since
- Jun 8 2014, 1:59 AM (158 w, 4 d)
Ah, I think this will only work with the non-threaded RTS, and we can't easily test that because we'd have to relink GHCi. Sorry about that! Just remove the test and we're good to go.
Mon, Jun 19
@AaronFriel the issue is that type error messages will display the transformed code, and not the original code that the user wrote. You'll see this if you construct an example that (a) is subject to the new transformation introduced by your patch, and (b) contains a type error that includes the transformed code. ApplicativeDo as currently implemented avoids this problem because when pretty-printed, ApplicativeStmt looks exactly like the original source code. If we turn a BodyStmt into a BindStmt, then we lose that property. Does that help?
So I agree there's a bug here, but I'm afraid I don't understand the fix!
Fri, Jun 16
We're good to go here. I talked to @simonpj and we agreed that this direction is fine for now, in the future we can think about redesigning hoopl and once again extracting it from GHC.
Thu, Jun 15
The title says WIP, but I'm not sure what's still IP here. Seems reasonable so I'll accept anyway.
Hmm. I'm not really keen on the design here, particularly the way that ModuleGraph contains both a [ModSummary] and also the ModuleEnvs. Can we make ModuleGraph not keep the [ModSummary]? Perhaps topSortModuleGraph could return [ModSummary], not ModuleGraph. Or perhaps it should really contain a graph, so that you can get the top-sorted order any time you like.
- Do we want to do this? I think we should, being careful to provide a smooth migration path for existing users. Let's discuss how exactly to do this before any changes take place.
Tue, Jun 13
I think you need to adjust the bounds on the perf tests again. Did T13379 regress?
@bgamari please merge this to the 8.2 branch if you'd be so kind!
Mon, Jun 12
Sun, Jun 11
Thu, Jun 8
The naming looks much better.
Wed, Jun 7
indentation & lint
Thu, Jun 1
Are the test failures relevant?
This looks OK in general, but somehow I feel like there's a more general solution trying to get out.
Sure, looks good to me. Does it make any difference to the perf tests?
Tue, May 30
I won't object, other than to note that this is typically the opposite direction that we normally want to go in, that is to use third-party libraries via their normal APIs rather than to fork them. We're trying to go in the other direction with pretty, for example. Having said that, I know it's difficult to reconcile some of the divergence without losing performance, particularly the manual specialisation. But some of the other impedence mismatch could be worked around, by either unifying APIs or centralising a renaming layer.
Looks right to me, just need to fix the copy/pasto pointed out by @alexbiehl
Wed, May 24
Nice, I'll leave it for @simonpj to accept, just nits and style suggestions from me.
May 23 2017
-pie seems reasonable to me. But couldn't -shared be used for this? There's an arguable consistency to it: -shared makes a dynamically-loadable library, it would be nice if it did the same thing for an executable.
May 22 2017
May 18 2017
Ok, passing back to you for refactoring. I'll take another look when you update the diff.
May 17 2017
Ok, you're using a different test case now. It looks fine, but you didn't show the output of a type error message, which I think will demonstrate the problem.
May 16 2017
May 15 2017
We don't have a good way to virtualize the file system (or IO in general). My concern is that whatever mechanisms we put in place here won't be enough.
@simonmar, what do you think about this?
Nice observation about the unnecessary reloads, and the results look good. I presume with -O the unnecessary reloads will be eliminated by CmmSink, but eliminating them earlier saves compile time and helps with -O0.
If it works on Windows, go for it.
May 11 2017
I think in your test case ApplicativeDo is not transforming the statement. You need to test it with an example that gets transformed after your diff.
I really don't like this, let's try to find alternative solutions.
May 10 2017
To be clear I don't object to a stop-gap approach that allows BodyStmt to work with ApplicativeDo without going all the way to Trac #10892, but the first point in my previous comment still applies: I think this will break error messages. I'm not sure what the best way to fix this is so I'll let you experiment, but we have to retain the information somehow that the original statement was a BodyStmt so that it can be printed as such in an error message.
May 9 2017
Ok, so there are a couple of problems with this approach
May 8 2017
@kavon should have a look at this too
May 5 2017
Well ok, if you can demonstrate a perf improvement then I won't object. GHC does use unsafeInterleaveIO, but as far as I know we've never identified it as a significant factor for performance (that could be because our profiling tools aren't good enough, of course). So my concerns are really around whether this is the most productive area to focus effort, rather than whether it's a good idea in isolation.
May 4 2017
I think I benchmarked this when I was working on the backend and found that it was worse. What effect did it have on the perf tests?
May 3 2017
@dfeuer what's this aiming at? Is there a problem with the old implementation, or are you trying to optimise it? If so, what's the benchmark?
Looks like it should be ok, although this is an unforced change so I feel slightly uneasy about it. Do we have good enough tests to be confident in this do you think?
Apr 28 2017
Apr 27 2017
I'm dubious too: there's no good way to test it, and we'll inevitably break it very quickly. Is there another way you could keep track of this file, perhaps in a separate github repo?
Apr 26 2017
Oh wow, that's a good point. You should be able to change it to an unsafeDupableInterleaveIO if you also change the takeMVar to be readMVar.
Apr 25 2017
Looks good to me. You could also clarify the docs a little by saying "i.e. it must be a pointer, but it doesn't matter if it's lifted or not"
This is probably a lot more expensive than the MVar version. Recall that noDuplicate# walks the stack; it's an O(n) operation.
Apr 24 2017
fix test for Windows
Looks great, thanks for doing this!
Apr 22 2017
Wow, you folks are really dragging this code out of the 90s :)
Apr 20 2017
Yep, looks right to me. It won't be interruptible which is a shame, but at least that's not a regression. Thanks for doing this!
Apr 17 2017
I haven't done a complete review here, there's a lot I don't understand. Just a few questions below. My main concern is that we keep the elft/x86_64 parts of the linker working and don't introduce any perf regressions.
Just a few nits...
Ok when you've got clean builds on OS X and Windows.
Yes, it's time we removed this.
Apr 15 2017
Thanks for doing this!
Apr 13 2017
Apr 12 2017
Seems like there are two independent changes here, shouldn't they be separate diffs?
Mar 31 2017
Let's call that a separate problem, I don't think I've made it worse here. Arguably I've made it better because :set won't put you in an inconsistent state after you change your environment file.
Wow. I didn't do anything about point (1), I don't think this is the right place to take into account environment files, and I'm not sure what you mean by "obvious that we failed to pick up a package environment file because of the caching mechanism".
Make a data type for package DB flags
We should already have touch on Windows as part of the msys toolchain.
The test is maybe OS-specific, but I was optimistically hoping it might work on Windows. I don't have a Windows box to test on though. (can anyone help?)
Mar 30 2017
- packageFlagsChanged: check package DB flags too
- setLogAction: set log_finaliser too