Skip to content

KAFKA-8991: Enable scalac optimizer #7452

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

Merged
ijuma merged 4 commits into from
Oct 9, 2019
Merged

KAFKA-8991: Enable scalac optimizer #7452

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0fe0bcd
KAFKA-8991: Enable scalac optimizer
ijuma Oct 6, 2019
b09430c
Avoid boxing by using `java.lang.Long.compare`
ijuma Oct 7, 2019
2e05b1a
spotBugs exclusions
ijuma Oct 7, 2019
80bb3ab
Add exclusions for Scala 2.12
ijuma Oct 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,22 @@ subprojects {
"-Xlint:constant",
"-Xlint:unused"
]

// Inline more aggressively when compiling the `core` jar since it's not meant to be used as a library.
// More specifically, inline classes from the Scala library so that we can inline methods like `Option.exists`
// and avoid lambda allocations. This is only safe if the Scala library version is the same at compile time
// and runtime. We cannot guarantee this for libraries like kafka streams, so only inline classes from the
// Kafka project in that case.
List<String> inlineFrom
if (project.name.equals('core'))
inlineFrom = ["-opt-inline-from:scala.**", "-opt-inline-from:kafka.**", "-opt-inline-from:org.apache.kafka.**"]
Copy link
Copy Markdown

@olafurpg olafurpg Nov 3, 2020

Choose a reason for hiding this comment

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

@ijuma I apologize if this the wrong place to ask a question about this PR. What would be the best place to start a discussion on removing "-opt-inline-from:scala.**" or reducing the scope of the glob to only include relevant methods like Option.exists? I'm not so familiar with the Kafka development process.

At Twitter, we are currently unable to upgrade to 2.5.x because kafka_2.12:2.5.1 inlines anonymous classes from the 2.12.11 library that no longer exist in the 2.12.12 library, resulting in crashes like this

 java.lang.NoClassDefFoundError: scala/math/Ordering$$anon$7

We can work around this issue by publishing a custom build of Kafka but that's not a sustainable solution.

These inlining settings are causing issues for a lot of users based on my search online. Even members of the Scala compiler team who worked on the inliner have suggested that the inliner settings in the Kafka build are based on false assumptions akka/alpakka-kafka#1212 (comment) cc/ @lrytz

Copy link
Copy Markdown
Member Author

@ijuma ijuma Nov 3, 2020

Choose a reason for hiding this comment

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

You can submit a PR. :)

Copy link
Copy Markdown

@olafurpg olafurpg Nov 3, 2020

Choose a reason for hiding this comment

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

I opened #9548 :)

Copy link
Copy Markdown
Member Author

@ijuma ijuma Nov 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Let's continue the discussion in #9548. Note the comment from @lrytz is regarding the claim that the core module should not be used as a library. This is the recommendation from the Kafka project though, so we can discuss in #9548 why users are depending on this module as a library (i.e. his expertise in the scala compiler/inliner is not relevant to this particular comment).

else
inlineFrom = ["-opt-inline-from:org.apache.kafka.**"]

// Somewhat confusingly, `-opt:l:inline` enables all optimizations. `inlineFrom` configures what can be inlined.
// See https://www.lightbend.com/blog/scala-inliner-optimizer for more information about the optimizer.
scalaCompileOptions.additionalParameters += ["-opt:l:inline"]
scalaCompileOptions.additionalParameters += inlineFrom
}

// these options are valid for Scala versions < 2.13 only
Expand Down
4 changes: 2 additions & 2 deletions core/src/main/scala/kafka/log/AbstractIndex.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,8 +405,8 @@ abstract class AbstractIndex[K, V](@volatile var file: File, val baseOffset: Lon

private def compareIndexEntry(indexEntry: IndexEntry, target: Long, searchEntity: IndexSearchEntity): Int = {
searchEntity match {
case IndexSearchType.KEY => indexEntry.indexKey.compareTo(target)
case IndexSearchType.VALUE => indexEntry.indexValue.compareTo(target)
case IndexSearchType.KEY => java.lang.Long.compare(indexEntry.indexKey, target)
Copy link
Copy Markdown
Contributor

@guozhangwang guozhangwang Oct 8, 2019

Choose a reason for hiding this comment

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

Curious why are these piggy-backs added here?

Copy link
Copy Markdown
Member Author

@ijuma ijuma Oct 8, 2019

Choose a reason for hiding this comment

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

It was causing a spotBugs failure after the inlining.

case IndexSearchType.VALUE => java.lang.Long.compare(indexEntry.indexValue, target)
}
}

Expand Down
59 changes: 55 additions & 4 deletions gradle/spotbugs-exclude.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,10 +104,8 @@ For a detailed description of spotbugs bug categories, see https://spotbugs.read
</Match>

<Match>
<!-- Suppression for the equals() for extensiom methods. -->
<Or>
<Class name="kafka.api.package$ElectLeadersRequestOps"/>
</Or>
<!-- Suppression for the equals() for extension methods. -->
<Class name="kafka.api.package$ElectLeadersRequestOps"/>
<Bug pattern="EQ_UNUSUAL"/>
</Match>

Expand All @@ -118,6 +116,59 @@ For a detailed description of spotbugs bug categories, see https://spotbugs.read
<Bug pattern="BC_VACUOUS_INSTANCEOF"/>
</Match>

<Match>
<!-- A spurious null check after inlining by the scalac optimizer confuses spotBugs -->
<Class name="kafka.log.Log"/>
<Bug pattern="NP_NULL_ON_SOME_PATH_EXCEPTION"/>
</Match>

<Match>
<!-- A spurious null check after inlining by the scalac optimizer confuses spotBugs -->
<Class name="kafka.tools.StateChangeLogMerger$"/>
<Bug pattern="NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE"/>
</Match>

<Match>
<!-- Unboxing to Int to make scalac happy makes spotBugs unhappy -->
<Class name="kafka.tools.ConsoleConsumer$ConsumerConfig"/>
<Bug pattern="BX_UNBOXING_IMMEDIATELY_REBOXED"/>
</Match>

<Match>
<!-- Uncallable anonymous methods are left behind after inlining by scalac 2.12, fixed in 2.13 -->
<Source name="AdminUtils.scala"/>
<Package name="kafka.admin"/>
<Bug pattern="UMAC_UNCALLABLE_METHOD_OF_ANONYMOUS_CLASS"/>
</Match>

<Match>
<!-- Uncallable anonymous methods are left behind after inlining by scalac 2.12, fixed in 2.13 -->
<Source name="ControllerContext.scala"/>
<Package name="kafka.controller"/>
<Bug pattern="UMAC_UNCALLABLE_METHOD_OF_ANONYMOUS_CLASS"/>
</Match>

<Match>
<!-- Uncallable anonymous methods are left behind after inlining by scalac 2.12, fixed in 2.13 -->
<Source name="LogManager.scala"/>
<Package name="kafka.log"/>
<Bug pattern="UMAC_UNCALLABLE_METHOD_OF_ANONYMOUS_CLASS"/>
</Match>

<Match>
<!-- Uncallable anonymous methods are left behind after inlining by scalac 2.12, fixed in 2.13 -->
<Source name="DelayedElectLeader.scala"/>
<Package name="kafka.server"/>
<Bug pattern="UMAC_UNCALLABLE_METHOD_OF_ANONYMOUS_CLASS"/>
</Match>

<Match>
<!-- Uncallable anonymous methods are left behind after inlining by scalac 2.12, fixed in 2.13 -->
<Source name="AdminZkClient.scala"/>
<Package name="kafka.zk"/>
<Bug pattern="UMAC_UNCALLABLE_METHOD_OF_ANONYMOUS_CLASS"/>
</Match>

<Match>
<!-- Suppress a warning about some static initializers in Schema using instances of a
subclass. -->
Expand Down