Skip to content

KYLIN-3716 FastThreadLocal replaces ThreadLocal#436

Closed
SteNicholas wants to merge 2 commits intoapache:masterfrom
SteNicholas:KYLIN-3716
Closed

KYLIN-3716 FastThreadLocal replaces ThreadLocal#436
SteNicholas wants to merge 2 commits intoapache:masterfrom
SteNicholas:KYLIN-3716

Conversation

@SteNicholas
Copy link
Copy Markdown
Member

In kylin query engine,QuerySevice acquires OLAPContext through ThreadLocal.In certain research,the development that FastThreadLocal replaces ThreadLocal to store thread OLAPContext is substantial performance improvement.

@asfgit
Copy link
Copy Markdown

asfgit commented Jan 19, 2019

Can one of the admins verify this patch?

Copy link
Copy Markdown
Contributor

@shaofengshi shaofengshi left a comment

Choose a reason for hiding this comment

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

This PR doesn't pass the "mvn test", please double check.

I did a quick check, found two issues:

  1. The ThreadLocal class couldn't be initialized; the root cause it:
    java.lang.NullPointerException
    at org.apache.kylin.common.threadlocal.ThreadLocalMap.nextVariableIndex(ThreadLocalMap.java:68)

After moving "nextIndex = new AtomicInteger()" before "slowThreadLocalMap = new ThreadLocal();", this error can be avoided;

  1. Dead loop:

    at org.apache.kylin.common.threadlocal.ThreadLocalMap.get(ThreadLocalMap.java:51)
    at org.apache.kylin.common.threadlocal.ThreadLocal.get(ThreadLocal.java:120)
    at org.apache.kylin.common.threadlocal.ThreadLocalMap.slowGet(ThreadLocalMap.java:144)
    at org.apache.kylin.common.threadlocal.ThreadLocalMap.get(ThreadLocalMap.java:51)
    at org.apache.kylin.common.threadlocal.ThreadLocal.get(ThreadLocal.java:120)
    at org.apache.kylin.common.threadlocal.ThreadLocalMap.slowGet(ThreadLocalMap.java:144)

For 2), I don't know how to handle it, I suggest you take a look and do test for it. Thanks!

@coveralls
Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 4086

  • 105 of 187 (56.15%) changed or added relevant lines in 24 files are covered.
  • 460 unchanged lines in 12 files lost coverage.
  • Overall coverage increased (+0.07%) to 26.908%

Changes Missing Coverage Covered Lines Changed/Added Lines %
core-common/src/main/java/org/apache/kylin/common/QueryContextFacade.java 0 1 0.0%
core-common/src/main/java/org/apache/kylin/common/debug/BackdoorToggles.java 0 1 0.0%
core-common/src/main/java/org/apache/kylin/common/metrics/metrics2/CodahaleMetrics.java 0 1 0.0%
core-common/src/main/java/org/apache/kylin/common/metrics/perflog/PerfLoggerFactory.java 0 1 0.0%
core-job/src/main/java/org/apache/kylin/engine/EngineFactory.java 0 1 0.0%
core-job/src/main/java/org/apache/kylin/job/SchedulerFactory.java 0 1 0.0%
core-metadata/src/main/java/org/apache/kylin/metadata/datatype/DataTypeSerializer.java 1 2 50.0%
core-metadata/src/main/java/org/apache/kylin/metadata/filter/function/BuiltInMethod.java 0 1 0.0%
core-metadata/src/main/java/org/apache/kylin/util/KryoUtils.java 0 1 0.0%
core-metrics/src/main/java/org/apache/kylin/metrics/lib/impl/RecordEvent.java 0 1 0.0%
Files with Coverage Reduction New Missed Lines %
core-cube/src/main/java/org/apache/kylin/cube/cuboid/TreeCuboidScheduler.java 2 68.46%
core-job/src/main/java/org/apache/kylin/job/dao/ExecutableOutputPO.java 2 84.62%
core-cube/src/main/java/org/apache/kylin/cube/inmemcubing/MemDiskStore.java 4 77.81%
engine-mr/src/main/java/org/apache/kylin/engine/mr/common/CuboidRecommenderUtil.java 13 0.0%
core-job/src/main/java/org/apache/kylin/job/JoinedFlatTable.java 14 0.0%
core-job/src/main/java/org/apache/kylin/job/execution/DefaultChainedExecutable.java 18 73.79%
tool/src/main/java/org/apache/kylin/tool/JobInstanceExtractor.java 21 0.0%
engine-mr/src/main/java/org/apache/kylin/engine/mr/common/JobInfoConverter.java 22 22.4%
core-cube/src/main/java/org/apache/kylin/cube/cuboid/algorithm/CuboidRecommender.java 39 0.0%
core-job/src/main/java/org/apache/kylin/job/execution/AbstractExecutable.java 47 58.26%
Totals Coverage Status
Change from base Build 4062: 0.07%
Covered Lines: 18711
Relevant Lines: 69537

💛 - Coveralls

@SteNicholas
Copy link
Copy Markdown
Member Author

@shaofengshi I did maven test and rename the classes of threadlocal package that causes some exception you referred.I have already return to the origin class name and checked maven test passed.

Copy link
Copy Markdown
Contributor

@shaofengshi shaofengshi left a comment

Choose a reason for hiding this comment

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

Hi Nicholas, thanks for the patch. Is it possible to add the dependency on netty and then use netty's "FastThreadLocal" class in Kylin? instead of copying the source code into Kylin?

@SteNicholas
Copy link
Copy Markdown
Member Author

@shaofengshi Yeah,it is possible to add the dependency on netty and then use netty's "FastThreadLocal" class in Kylin.I simplify FastThreadLocal by using InternalThreadLocal and avoid more dependency in Kylin.Therefore, there is two ways to improve ThreadLocal performance by different aspects.

@shaofengshi
Copy link
Copy Markdown
Contributor

Hi Nicholas, I merge the change by merging the two commits into one, and modified the commit message to follow Apache rule. Thanks for your patience!

@shaofengshi shaofengshi closed this Mar 4, 2019
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.

4 participants