Skip to content

Conversation

@andreiburdusa
Copy link
Contributor


Fixes #1988

Reviewer checklist
  • Test coverage: stack test --coverage
  • Public API documentation: stack haddock

@andreiburdusa andreiburdusa marked this pull request as ready for review July 22, 2020 11:47
@ttuegel ttuegel requested a review from ana-pantilie July 22, 2020 14:04
Copy link
Contributor

@ana-pantilie ana-pantilie left a comment

Choose a reason for hiding this comment

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

Is it possible to use the logger to issue a warning for this?

@andreiburdusa
Copy link
Contributor Author

andreiburdusa commented Jul 22, 2020

Is it possible to use the logger to issue a warning for this?

That would require IO to be an instance of MonadLog and by looking at the class definition I think it was intended to instantiate only transformers 🤔, but I don't know.
Also, I thought the logger is for user-relevant information

@ana-pantilie
Copy link
Contributor

LoggerT is MonadIO so you could get the stats and log them inside the logger, but it might be that you would want to access this information only at the very end of execution. Would that be the case here?

@andreiburdusa
Copy link
Contributor Author

LoggerT is MonadIO so you could get the stats and log them inside the logger, but it might be that you would want to access this information only at the very end of execution. Would that be the case here?

Oh, I thought by "logger" you mean the entire logging mechanism.

@ttuegel Can the warning be moved as Ana suggested?

warnIfLowProductivity = do
Stats { gc_cpu_ns, cpu_ns } <- liftIO getStats
when (gc_cpu_ns * 10 > cpu_ns) $
logWarning
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason we don't want to add a separate entry type, WarnIfLowProductivity, instead of logging this as a LogMessage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

parsimony :))
I'll add a new entry type 👍

Copy link
Contributor

@ana-pantilie ana-pantilie left a comment

Choose a reason for hiding this comment

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

Approved and added a refactoring commit, please take a look when you have time.

@rv-jenkins rv-jenkins merged commit c3e3728 into runtimeverification:master Jul 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Warn about poor performance when productivity is low

3 participants