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

Drop non-profiling inputs to profiling build #892

Merged
merged 1 commit into from
May 24, 2019
Merged

Conversation

aherrmann
Copy link
Member

The profiling builds took the non-profiling object file outputs as an additional input with a comment stating that these are required for template Haskell. However, it seems these inputs are unnecessary as removing them does not seem to negatively affect bazel test //tests/... -c dbg. As I understand, this is because we are passing -fexternal-interpreter on profiling builds. Removing that flag would indeed cause errors due to missing object files.

@aherrmann aherrmann requested review from mrkkrp and guibou May 23, 2019 14:07
@guibou
Copy link
Contributor

guibou commented May 24, 2019

I fully agree with theses changes, however have a look at #438

There is two options for profiling:

  • building twice (with and without profiling)
  • building with -fexternal-interpreter

The current situation is that, starting from the first solution, the flag for the second solution was introduced in a totally unrelated commit, see #438 for all the details, and progressively the code evolved such that the first solution was not working anymore and that's what your are removing now (which is a good thing).

But considering that -fexternal-interpreter is ~ 15x times slower than the other approach when it involves template haskell, see https://gitlab.haskell.org/ghc/ghc/issues/16256 I have concerns.

That's a tradeoff between simplifying rules_haskell by fully switching to -fexternal-interpreter or removing the flag and fixing the previous approach. Pro and cons of approach:

  • Fixing the first will be more work than the second
  • The first generates more code in rules_haskell
  • The second is slower
  • The second will be faster if the bug if fixed upstream.

Copy link
Contributor

@guibou guibou left a comment

Choose a reason for hiding this comment

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

This can be extended by removing the objets generation code at all. But I think that bazel won't generate them because they are not part of the dependency graph anymore.

See my other comment before merging.

@aherrmann
Copy link
Member Author

@guibou Thanks for the pointer. It's a tricky tradeoff. As you point out this PR doesn't change the current state of affairs.

This can be extended by removing the objets generation code at all.

I'm working on this for a separate PR. I'll continue the discussion there.

@aherrmann aherrmann merged commit ad7e2af into master May 24, 2019
@aherrmann aherrmann deleted the profiling_objects branch May 24, 2019 09:56
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.

2 participants