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.

Sign up for GitHub

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

Jump to bottom

Log histograms with -Dorg.enso.persist.Logger.level=debug #8881

Merged
merged 2 commits into from
Jan 29, 2024
Merged

Log histograms with -Dorg.enso.persist.Logger.level=debug #8881

merged 2 commits into from
Jan 29, 2024

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Jan 28, 2024 •
edited
Loading

Pull Request Description

Fixes #8644 by using slf4j instead of java.util.logging.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • All code follows the
    Scala,
    Java,
  • All code has been tested:
    • Manually verified

@JaroslavTulach JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label Jan 28, 2024
@JaroslavTulach JaroslavTulach self-assigned this Jan 28, 2024
JaroslavTulach
JaroslavTulach commented Jan 28, 2024
View reviewed changes
build.sbt
@@ -1105,6 +1105,7 @@ lazy val `persistance` = (project in file("lib/java/persistance"))
frgaalJavaCompilerSetting,
Compile / javacOptions := ((Compile / javacOptions).value),
libraryDependencies ++= Seq(
"org.slf4j" % "slf4j-api" % slf4jVersion,
Copy link
Member Author

Choose a reason for hiding this comment

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

Bye, bye "no dependencies" of lib/java/persitance project...

JaroslavTulach
JaroslavTulach commented Jan 28, 2024
View reviewed changes
lib/java/persistance/src/main/java/org/enso/persist/PerGenerator.java
" {0} {1} {2}",
new Object[] {elem.getValue()[0], elem.getValue()[1], elem.getKey().getName()});
log.debug(
" " + elem.getValue()[0] + " " + elem.getValue()[1] + " " + elem.getKey().getName());
Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately the {0} {1} {2} format doesn't work with sfl4j. Sad outcome of using two different logging system throughout the project.

Copy link
Member

Choose a reason for hiding this comment

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

I believe the correct way to do this in slf4j is just use empty brackets {} instead of {0}. As an example, look into

enso/engine/language-server/src/main/java/org/enso/languageserver/boot/resource/RepoInitialization.java

Line 160 in c109886

"Failed to delete the database file. Attempt #{}. File does not exist [{}].",

Copy link
Member Author

Choose a reason for hiding this comment

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

Concatenating strings manually relies on standard Java mechanism rather than relying on the logging framework semantics. As it is guaranteed the message is going to be printed, I opted for the generic mechanism known to work.

Copy link
Member

Choose a reason for hiding this comment

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

It is fine for this case. But generally, you should avoid string concatenation for logs. If you do string concatenation, the concatenation is evaluated before log.debug method is called even if the DEBUG is disabled.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is fine for this case.

Yes, it is absolutely fine in this case as we know we are going to log.

@JaroslavTulach
Copy link
Member Author

I can execute Enso with -Dorg.enso.persist.Logger.level=debug and see some output:

enso$ JAVA_OPTS=-Dorg.enso.persist.Logger.level=debug ./built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/bin/enso --run test/Base_Tests/

 ==== Top Bytes & Counts of Classes =====
   39037 844 org.enso.compiler.core.ir.Name$Literal
   11480 2 scala.collection.mutable.HashMap
   8592 716 scala.collection.immutable.Set$Set1
   7464 622 org.enso.compiler.core.ir.IdentifiedLocation
   7336 80 org.enso.compiler.data.BindingsMap$ResolvedImport
   7188 84 org.enso.compiler.core.ir.expression.Application$Prefix
   7155 163 org.enso.compiler.data.BindingsMap$ModuleReference$Abstract
   6844 90 org.enso.compiler.core.ir.CallArgument$Specified
   6376 80 org.enso.compiler.core.ir.module.scope.Import$Module
   4474 167 org.enso.compiler.pass.analyse.AliasAnalysis$Graph$Occurrence$Use
   3135 627 org.enso.compiler.pass.analyse.DataflowAnalysis$DependencyInfo$Type$Static
   2425 164 org.enso.compiler.pass.analyse.DataflowAnalysis$DependencyInfo$Type$Dynamic
   2200 91 org.enso.compiler.pass.analyse.AliasAnalysis$Graph$Scope
   2040 170 org.enso.compiler.pass.analyse.AliasAnalysis$Info$Occurrence
   1640 82 scala.collection.immutable.Set$Set2
   1520 95 org.enso.compiler.pass.analyse.AliasAnalysis$Info$Scope$Child
   1264 158 org.enso.compiler.data.BindingsMap$Resolution
   1264 158 org.enso.compiler.data.BindingsMap$ResolvedModule
   1260 3 scala.collection.immutable.HashSet
   1097 78 org.enso.compiler.data.BindingsMap$ResolvedMethod
   1052 263 org.enso.compiler.pass.analyse.CachePreferenceAnalysis$WeightInfo
   895 3 org.enso.compiler.core.ir.Expression$Block
   684 1 org.enso.compiler.data.BindingsMap
   224 2 org.enso.compiler.core.ir.Expression$Binding
   202 2 org.enso.compiler.core.ir.Function$Lambda
   181 21 java.lang.String
   168 6 scala.collection.immutable.Set$Set3
   154 1 org.enso.compiler.core.ir.module.scope.definition.Method$Explicit
   97 1 org.enso.compiler.core.ir.DefinitionArgument$Specified
   84 3 org.enso.compiler.pass.analyse.AliasAnalysis$Graph$Occurrence$Def

@enso-bot enso-bot bot mentioned this pull request Jan 29, 2024
Closed
@JaroslavTulach JaroslavTulach added the CI: Keep up to date Automatically update this PR to the latest develop. label Jan 29, 2024
@JaroslavTulach JaroslavTulach added CI: Ready to merge This PR is eligible for automatic merge and removed CI: Keep up to date Automatically update this PR to the latest develop. labels Jan 29, 2024
Akirathan
Akirathan approved these changes Jan 29, 2024
View reviewed changes
Copy link
Member

@Akirathan Akirathan left a comment

Choose a reason for hiding this comment

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

Maybe use logger.log("Hello {}", name) instead of string concatenation. Just a minor note though.

@mergify mergify bot merged commit 946f931 into develop Jan 29, 2024
26 of 28 checks passed
@mergify mergify bot deleted the wip/jtulach/Slf4jInPersist_8644 branch January 29, 2024 10:27
JaroslavTulach
JaroslavTulach commented Jan 30, 2024
View reviewed changes
lib/java/persistance/src/main/java/org/enso/persist/PerGenerator.java
@@ -31,7 +30,7 @@ private PerGenerator(
}

static byte[] writeObject(Object obj, Function<Object, Object> writeReplace) throws IOException {
var histogram = PerUtils.LOG.isLoggable(Level.FINE) ? new Histogram() : null;
var histogram = PerUtils.LOG.isDebugEnabled() ? new Histogram() : null;
Copy link
Member Author

Choose a reason for hiding this comment

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

Whether or not we will log is decided up-front before creating new Histogram(). Once the instance is created, it is clear we will log whatever it will have collected at the end.

@JaroslavTulach JaroslavTulach mentioned this pull request Jan 30, 2024
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Akirathan Akirathan Akirathan approved these changes

@hubertp hubertp hubertp approved these changes

@4e6 4e6 Awaiting requested review from 4e6 4e6 is a code owner

Assignees

@JaroslavTulach JaroslavTulach

Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot enable org.enso.persist logger via a property
3 participants
@JaroslavTulach @hubertp @Akirathan