[FLINK-16245] Decoupling user classloader from context classloader. #13027

AHeise · 2020-07-30T08:20:03Z

This is a follow-up PR of #11303 after #11611 disabled it because of performance regression in blink e2e test.

What is the purpose of the change

Decoupling user class loader from context classloader.

Thus, user classloader can be unloaded even though a reference on the context classloader outlives the user code.

Brief change log

Adding SafetyNetWrapperClassLoader
Use it for FlinkUserCodeClassLoaders
Make sure LocalExecutor closes user classloader

Compared to the original PR, it includes the following changes

Use the parent classloader of the FlinkUserCodeClassLoader in SafetyNetWrapperClassLoader to address the performance concerns.
Further, improve performance by enabling parallel class loading in all involved classloaders.

Verifying this change

Added unit test.

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (yes / no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (yes / no)
The serializers: (yes / no / don't know)
The runtime per-record code paths (performance sensitive): (yes / no / don't know)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn/Mesos, ZooKeeper: (yes / no / don't know)
The S3 file system connector: (yes / no / don't know)

Documentation

Does this pull request introduce a new feature? (yes / no)
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

flinkbot · 2020-07-30T08:23:01Z

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit 4301bfa (Thu Jul 30 08:23:01 UTC 2020)

Warnings:

No documentation files were touched! Remember to keep the Flink docs up to date!

_{Mention the bot in a comment to re-run the automated checks.}

Review Progress

❓ 1. The [description] looks good.
❓ 2. There is [consensus] that the contribution should go into to Flink.
❓ 3. Needs [attention] from.
❓ 4. The change fits into the overall [architecture].
❓ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.

The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
@flinkbot approve all to approve all aspects
@flinkbot approve-until architecture to approve everything until architecture
@flinkbot attention @username1 [@username2 ..] to require somebody's attention
@flinkbot disapprove architecture to remove an approval you gave earlier

flinkbot · 2020-07-30T08:30:19Z

CI report:

74857e7 Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run travis re-run the last Travis build
@flinkbot run azure re-run the last Azure build

zentol · 2020-07-30T10:02:07Z

Could you summarize what the problem was that caused the performance regression?

Allows user classloader can be unloaded even if a reference on the context classloader outlives the user code.

Because the classloader is now closed when the task fails the UDF only has access to the bootstrap classloader, which doesn't contain our own test classes.

AHeise · 2020-08-04T09:54:26Z

Could you summarize what the problem was that caused the performance regression?

For some reason, not setting a parent classloader in the wrapper is much slower. I cannot see an obvious reason and added a couple of measurements.

First, I wanted to know if the performance difference was caused by slower class loading. So I measured the total time spent in ClassLoader#loadClass for the previous version and this version. I found no difference. There is also no difference by the amount of classes loaded.

Second, a profiler revealed that much time in the slow version was spent in Class#newInstance. I added the time measurements to org.apache.flink.table.runtime.generated.GeneratedClass. Interestingly, there is again no difference in compiling the class. There is a huge difference in calling newInstance on the generated class though that can easily account for the time difference.

Without the parent, creating an instance of the 4 specific blink operators BatchNestedLoopJoin, LongHashJoinOperator, LocalHashAggregateWithKeys, and HashAggregateWithKeys slows down over time taking up over 10s to finish at the end of the.

Interestingly, they use only the instantiation method with explicit arguments GeneratedClass#newInstance(ClassLoader classLoader, Object... args). However, there are also operators that use that method that are "well-behaved".

I have also forced class resolution through reflection on the generated classes, but resolution is fast in all cases. It's really just about creating the instances.

edit: log of the timed measurements without parent: https://gist.github.com/AHeise/50375144fb6d6da7acb324544722e10b ; I can also provide the full logs.

AHeise · 2020-08-04T09:58:03Z

There was also a classloader leak in ORC, for which I added a workaround.

See https://issues.apache.org/jira/browse/ORC-653 OrcFile#getStaticMemoryManager caches initial configuration and leaks classloader in it. Thus, new Flink jobs use the classloader of the first job implicitly. By adding ThreadLocalClassLoaderConfiguration, which forces the use of thread-local classloader over the initial classloader, Flink jobs use the appropriate classloader on higher runtime costs (no caches). This commit should be reverted, once the bug in ORC is fixed.

zentol

One question for clarity, otherwise this looks good.

zentol · 2020-08-07T07:25:03Z

...ctors/flink-connector-hive/src/main/java/org/apache/flink/connectors/hive/HiveTableSink.java

 			return Optional.of(ParquetRowDataBuilder.createWriterFactory(
 					formatType, formatConf, hiveVersion.startsWith("3.")));
 		} else if (serLib.contains("orc")) {
+			Configuration formatConf = new ThreadLocalClassLoaderConfiguration(jobConf);


If we were to use a normal configuration, what would happen? Subsequent jobs would not be able to run?

Yes. That makes me believe that we should probably make the safety net configurable (on by default). Or would you think we can catch up and fix leaks much quicker?

An opt-out switch is a good idea; also for cases where things don't behave as expected close to the next release, and we don't want to revert commits at that point.

zentol · 2020-08-12T08:06:45Z

flink-core/src/main/java/org/apache/flink/configuration/CoreOptions.java

@@ -124,6 +124,15 @@
 		return parseParentFirstLoaderPatterns(base, append);
 	}

+	@Documentation.Section(Documentation.Sections.EXPERT_CLASS_LOADING)
+	public static final ConfigOption<Boolean> CHECK_LEAKED_CLASSLOADER = ConfigOptions
+		.key("classloader.check_leaked_classloader")


we don't use underscores.

zentol · 2020-08-12T08:09:25Z

...src/main/java/org/apache/flink/runtime/execution/librarycache/FlinkUserCodeClassLoaders.java

+				throw new IllegalStateException("Trying to access closed classloader. Please check if you store " +
+					"classloaders directly or indirectly in static fields. If the stacktrace suggests that the leak " +
+					"occurs in a third party library and cannot be fixed immediately, you can disable this check " +
+					"with the configuration 'classloader.check_leaked_classloader'.");


Retrieve the key from the actual configuration object instead.

zentol · 2020-08-12T08:47:23Z

flink-core/src/main/java/org/apache/flink/configuration/CoreOptions.java

+		.withDescription("Fails if a user classloader is used in another job after a job has been finished. This " +
+			"check should only be disabled if the class leak occurs in an external library cannot be fixed " +
+			"immediately.");


Fails attempts at loading classes if the user classloader of a job is used after it has terminated. This is usually caused by the classloader being leaked by lingering threads or misbehaving libraries, which may also result in the classloader being used by other jobs. This check should only be disabled if such a leak prevents further jobs from running.

…oader. If users experience class loader leaks in libraries not under his control, they can force old behavior with the config option.

rmetzger added the review=description? label Jul 30, 2020

rmetzger added the component=Runtime/Task label Jul 30, 2020

AHeise added 3 commits July 30, 2020 15:32

[FLINK-16245][runtime] Decouple user from context classloader

6447ae8

Allows user classloader can be unloaded even if a reference on the context classloader outlives the user code.

[FLINK-16245][table] Close user classloader

4a85022

[FLINK-16245][tests] Adjust BatchFineGrainedRecoveryITCase

47ea974

Because the classloader is now closed when the task fails the UDF only has access to the bootstrap classloader, which doesn't contain our own test classes.

zentol self-assigned this Aug 4, 2020

zentol approved these changes Aug 7, 2020

View reviewed changes

zentol requested changes Aug 12, 2020

View reviewed changes

[FLINK-16917][runtime] Added option to disable safety net user classl…

74857e7

…oader. If users experience class loader leaks in libraries not under his control, they can force old behavior with the config option.

zentol merged commit fce82d7 into apache:master Aug 16, 2020

AHeise deleted the FLINK-16917 branch August 17, 2020 06:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-16245] Decoupling user classloader from context classloader. #13027

[FLINK-16245] Decoupling user classloader from context classloader. #13027

AHeise commented Jul 30, 2020

flinkbot commented Jul 30, 2020

flinkbot commented Jul 30, 2020 •

edited

zentol commented Jul 30, 2020

AHeise commented Aug 4, 2020 •

edited

AHeise commented Aug 4, 2020

zentol left a comment

zentol Aug 7, 2020

AHeise Aug 7, 2020

zentol Aug 7, 2020

zentol Aug 12, 2020

zentol Aug 12, 2020

zentol Aug 12, 2020

[FLINK-16245] Decoupling user classloader from context classloader. #13027

[FLINK-16245] Decoupling user classloader from context classloader. #13027

Conversation

AHeise commented Jul 30, 2020

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

flinkbot commented Jul 30, 2020

Automated Checks

Review Progress

flinkbot commented Jul 30, 2020 • edited

CI report:

zentol commented Jul 30, 2020

AHeise commented Aug 4, 2020 • edited

AHeise commented Aug 4, 2020

zentol left a comment

Choose a reason for hiding this comment

zentol Aug 7, 2020

Choose a reason for hiding this comment

AHeise Aug 7, 2020

Choose a reason for hiding this comment

zentol Aug 7, 2020

Choose a reason for hiding this comment

zentol Aug 12, 2020

Choose a reason for hiding this comment

zentol Aug 12, 2020

Choose a reason for hiding this comment

zentol Aug 12, 2020

Choose a reason for hiding this comment

flinkbot commented Jul 30, 2020 •

edited

AHeise commented Aug 4, 2020 •

edited