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

Add cost centres in decodeExpressionInternal #1808

Closed
wants to merge 1 commit into from

Conversation

sjakobi
Copy link
Collaborator

@sjakobi sjakobi commented May 23, 2020

…to aid profiling.

Context: #1804.

@sjakobi sjakobi mentioned this pull request May 23, 2020
4 tasks
@sjakobi
Copy link
Collaborator Author

sjakobi commented May 23, 2020

Hydra reports:

      ./dhall-lang/tests/type-inference/success/CacheImportsCanonicalize:                                                      FAIL (5.02s)
        tests/Dhall/Test/TypeInference.hs:73:

        Error: Remote host not found

        URL: https://test.dhall-lang.org/random-string

Should I simply try again?

…to aid profiling.

Context: #1804.
@sjakobi sjakobi force-pushed the sjakobi/profile-decoding branch from 1dabc0a to 3f10c07 Compare May 24, 2020 09:12
@sjakobi
Copy link
Collaborator Author

sjakobi commented May 24, 2020

@Gabriel439 How do I convince Hydra to give my patch another chance? Or does the error indicate some underlying problem that needs to be addressed?

@Gabriella439
Copy link
Collaborator

@sjakobi: If you have a GPG key I can create a Hydra account for you so that you can restart things on your own. In the interim, I'll go ahead and restart the build myself. I'd also be fine with disabling that test.

@Gabriella439
Copy link
Collaborator

@sjakobi: Also, I generally don't recommend checking SCC annotations into version control. The reason why is that if you're not interested in profiling that specific code at a given time they will distort the profile to make that code look more expensive compared to the rest of the code base. This is because SCC annotations will interfere with optimizations / inlining.

@sjakobi
Copy link
Collaborator Author

sjakobi commented May 24, 2020

I don't currently have GPG set up, but you make a good point that these cost centers might distort other profiling results, so I'm fine with not merging this. I'll keep the branch though for future use.

Regarding the test failure – is that an error after which we might want to retry the request? Otherwise I also wouldn't mind disabling it.

@sjakobi sjakobi closed this May 24, 2020
@Gabriella439
Copy link
Collaborator

@sjakobi: Rather than retry at the test level, we should consider if we should have the interpreter automatically retry all imports as discussed in #1794

@sjakobi
Copy link
Collaborator Author

sjakobi commented May 24, 2020

Yeah, that's what I meant. :)

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