[SPARK-16131] initialize internal logger lazily in Scala preferred way #13842

infynyxx · 2016-06-22T12:43:41Z

What changes were proposed in this pull request?

Initialize logger instance lazily in Scala preferred way

How was this patch tested?

By running ./build/mvn clean test locally

srowen · 2016-06-22T12:47:37Z

core/src/main/scala/org/apache/spark/internal/Logging.scala

-    }
-    log_
-  }
+  protected def log: Logger = log_


Do we even need log_ separately from log? log can now be the lazy val.

In class DriverEndpoint under CoarseGrainedSchedulerBackend.scala,
log() seems to be overridden[1]

Not sure, what's the logic behind that but if we can remove that, log()
can also be removed from Logging trait.

[1]
https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala#L103

On Wed, Jun 22, 2016 at 12:48 PM, Sean Owen notifications@github.com
wrote:

In core/src/main/scala/org/apache/spark/internal/Logging.scala
#13842 (comment):

@@ -41,13 +44,7 @@ private[spark] trait Logging {
}

// Method to get or create the logger for this object

protected def log: Logger = {

if (log_ == null) {

initializeLogIfNecessary(false)

log_ = LoggerFactory.getLogger(logName)

}

log_

}

protected def log: Logger = log_

Do we even need log_ separately from log? log can now be the lazy val.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/apache/spark/pull/13842/files/88ffc87c670deb675f443891a4b2e3315467098e#r68046045,
or mute the thread
https://github.com/notifications/unsubscribe/AACdKruY8xLoqhQEkqS167NIyEM-v4Qtks5qOS8lgaJpZM4I7vOl
.

Cheers,
Praj

Good point. I think that can be removed. I don't see the point of it, as neither it nor its subclass even logs anything directly?

Fixed via
infynyxx@986e61f

On Wed, Jun 22, 2016 at 2:17 PM, Sean Owen notifications@github.com wrote:

In core/src/main/scala/org/apache/spark/internal/Logging.scala
#13842 (comment):

@@ -41,13 +44,7 @@ private[spark] trait Logging {
}

// Method to get or create the logger for this object

protected def log: Logger = {

if (log_ == null) {

initializeLogIfNecessary(false)

log_ = LoggerFactory.getLogger(logName)

}

log_

}

protected def log: Logger = log_

Good point. I think that can be removed. I don't see the point of it, as
neither it nor its subclass even logs anything directly?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/apache/spark/pull/13842/files/88ffc87c670deb675f443891a4b2e3315467098e#r68060626,
or mute the thread
https://github.com/notifications/unsubscribe/AACdKnkKjiFYLhaEMT3wJslNc4olwNSKks5qOUPtgaJpZM4I7vOl
.

Cheers,
Praj

srowen · 2016-06-22T15:14:32Z

core/src/main/scala/org/apache/spark/internal/Logging.scala

  // Make the log field transient so that objects with Logging can
  // be serialized and used on another machine
-  @transient private var log_ : Logger = null
+  @transient lazy val log : Logger = {


I think this will fail style checks because of the space before the colon

Yup, my bad. Fixed via
infynyxx@18ae8c5

On Wed, Jun 22, 2016 at 3:15 PM, Sean Owen notifications@github.com wrote:

In core/src/main/scala/org/apache/spark/internal/Logging.scala
#13842 (comment):

@@ -32,23 +32,17 @@ private[spark] trait Logging {

// Make the log field transient so that objects with Logging can
// be serialized and used on another machine

@transient private var log_ : Logger = null

@transient lazy val log : Logger = {

I think this will fail style checks because of the space before the colon

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/apache/spark/pull/13842/files/986e61f96ece6d7f71ff760384cc7553fe3828f4#r68072052,
or mute the thread
https://github.com/notifications/unsubscribe/AACdKtAjTKSCyAI2oyyH7fFUatd4xsCmks5qOVGUgaJpZM4I7vOl
.

Cheers,
Praj

srowen · 2016-06-22T15:14:40Z

Jenkins test this please

SparkQA · 2016-06-22T16:39:51Z

Test build #61040 has finished for PR 13842 at commit 986e61f.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

srowen · 2016-06-22T16:52:03Z

Jenkins retest this please

SparkQA · 2016-06-22T17:17:58Z

Test build #61047 has finished for PR 13842 at commit 18ae8c5.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

zsxwing · 2016-06-22T18:17:30Z

core/src/main/scala/org/apache/spark/internal/Logging.scala

  // Make the log field transient so that objects with Logging can
  // be serialized and used on another machine
-  @transient private var log_ : Logger = null
+  @transient lazy val log: Logger = {


I think lazy will add a volatile flag to log and prevents some JIT optimizations?

zsxwing · 2016-06-22T18:26:23Z

So the pros is only calling initializeLogIfNecessary(false) and LoggerFactory.getLogger(logName) once, the cons is marking a field used frequently volatile. I don't think it's worth.

srowen · 2016-06-22T19:26:16Z

Well, it simplifies the code too and avoids a method call. The current variable really ought to be volatile but isn't, so I don't know if that's a downside. I think the performance issue is negligible, since the cost of dereferencing a volatile variable is nothing compared to the time to construct and log the message to disk. It's really minor but I think there are only upsides here.

zsxwing · 2016-06-22T19:52:51Z

I was thinking about logTrace and logDebug. They are not enabled so the cost of them are cheaper comparing dereferencing a volatile variable. Anyway, in a second thought, we should remove a a debug/trace log if we are concerned about the performance instead of counting on logs are cheap.

So 👍

zsxwing · 2016-06-22T19:53:01Z

retest this please

SparkQA · 2016-06-22T22:03:03Z

Test build #61065 has finished for PR 13842 at commit 18ae8c5.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

zsxwing · 2016-06-22T23:29:15Z

Thanks. Merging to master and 2.0!

## What changes were proposed in this pull request? Initialize logger instance lazily in Scala preferred way ## How was this patch tested? By running `./build/mvn clean test` locally Author: Prajwal Tuladhar <praj@infynyxx.com> Closes #13842 from infynyxx/spark_internal_logger. (cherry picked from commit 044971e) Signed-off-by: Shixiong Zhu <shixiong@databricks.com>

initialize internal logger lazily in Scala preferred way

88ffc87

srowen reviewed Jun 22, 2016
View reviewed changes

no need for log getter method

986e61f

srowen reviewed Jun 22, 2016
View reviewed changes

fix style typo

18ae8c5

zsxwing reviewed Jun 22, 2016
View reviewed changes

asfgit closed this in 044971e Jun 22, 2016

[SPARK-16131] initialize internal logger lazily in Scala preferred way #13842

[SPARK-16131] initialize internal logger lazily in Scala preferred way #13842

Uh oh!

Conversation

infynyxx commented Jun 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

srowen Jun 22, 2016

Choose a reason for hiding this comment

Uh oh!

infynyxx Jun 22, 2016

Choose a reason for hiding this comment

Uh oh!

srowen Jun 22, 2016

Choose a reason for hiding this comment

Uh oh!

infynyxx Jun 22, 2016

Choose a reason for hiding this comment

Uh oh!

srowen Jun 22, 2016

Choose a reason for hiding this comment

Uh oh!

infynyxx Jun 22, 2016

Choose a reason for hiding this comment

Uh oh!

srowen commented Jun 22, 2016

Uh oh!

SparkQA commented Jun 22, 2016

Uh oh!

srowen commented Jun 22, 2016

Uh oh!

SparkQA commented Jun 22, 2016

Uh oh!

zsxwing Jun 22, 2016

Choose a reason for hiding this comment

Uh oh!

zsxwing commented Jun 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srowen commented Jun 22, 2016

Uh oh!

zsxwing commented Jun 22, 2016

Uh oh!

zsxwing commented Jun 22, 2016

Uh oh!

SparkQA commented Jun 22, 2016

Uh oh!

zsxwing commented Jun 22, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

infynyxx commented Jun 22, 2016 •

edited

Loading

zsxwing commented Jun 22, 2016 •

edited

Loading