Skip to content

[SPARK-34573][SQL] Avoid global locking in SQLConf object for sqlConfEntries map #31689

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
Closed
sparkyengine wants to merge 3 commits into from
Closed

[SPARK-34573][SQL] Avoid global locking in SQLConf object for sqlConfEntries map #31689

sparkyengine wants to merge 3 commits into from

Conversation

@sparkyengine
Copy link
Contributor

@sparkyengine sparkyengine commented Mar 1, 2021 •
edited
Loading

What changes were proposed in this pull request?

In the SQLConf object, the sqlConfEntries map is globally synchronized (it is a Java Collections.synchronizedMap): any operation, including a get, will need to acquire the lock.

An example of this is calling the DatatType.sameType method. This will trigger a check on SQLConf.get.caseSensitiveAnalysis. So every time we compare two datatypes with sameType, we hit a lock.

To avoid having multiple tasks locking on this, a better approach would be to use a map that does not lock on read (like a ConcurrentHashMap). This map implementation does not lock on read, and on write it only locks the map partially. The only lock that happens is on write on the same map key.

Why are the changes needed?

Multiple tasks performing any operation that directly or indirectly trigger a query to the SQLConf.sqlConfEntries map, will require acquiring a global lock on that map. Something as easy as calling DataType.sameType(...) would be locking on the global sqlConfEntries lock of the Collections.synchronizedMap.

Does this PR introduce any user-facing change?

No

How was this patch tested?

No functionality change. Existing unit tests run normally.

@github-actions github-actions bot added the SQL label Mar 1, 2021
@sparkyengine sparkyengine changed the title [SPARK-34573][SQL][CORE] avoid global locking in SQLConf object for sqlConfEntries map [WIP][SPARK-34573][SQL][CORE] avoid global locking in SQLConf object for sqlConfEntries map Mar 1, 2021
@sparkyengine sparkyengine changed the title [WIP][SPARK-34573][SQL][CORE] avoid global locking in SQLConf object for sqlConfEntries map [WIP][SPARK-34573][SQL] avoid global locking in SQLConf object for sqlConfEntries map Mar 1, 2021
@sparkyengine
replace synchronized block in SQLConf.register method with single row…
4ffa257 
… lock

Note that merge function is only called if old value is not null, so it will always fail the require check (as intended):

 V newValue = (oldValue == null) ? value :
              remappingFunction.apply(oldValue, value);
@sparkyengine sparkyengine changed the title [WIP][SPARK-34573][SQL] avoid global locking in SQLConf object for sqlConfEntries map [SPARK-34573][SQL] avoid global locking in SQLConf object for sqlConfEntries map Mar 1, 2021
@maropu
Copy link
Member

maropu commented Mar 2, 2021

ok to test

@maropu
Copy link
Member

maropu commented Mar 2, 2021

This change could get away the lock wait you mentioned in the jira? If so, could you describe more in the PR description?

@maropu maropu changed the title [SPARK-34573][SQL] avoid global locking in SQLConf object for sqlConfEntries map [SPARK-34573][SQL] Avoid global locking in SQLConf object for sqlConfEntries map Mar 2, 2021
@sparkyengine
Copy link
Contributor Author

sparkyengine commented Mar 2, 2021

This change could get away the lock wait you mentioned in the jira? If so, could you describe more in the PR description?

Added more descriptive PR text.

@SparkQA
Copy link

SparkQA commented Mar 2, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40196/

@SparkQA
Copy link

SparkQA commented Mar 2, 2021

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40196/

maropu
maropu reviewed Mar 2, 2021
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala
private[sql] val sqlConfEntries = java.util.Collections.synchronizedMap(
new java.util.HashMap[String, ConfigEntry[_]]())
private[sql] val sqlConfEntries =
new ConcurrentHashMap[String, ConfigEntry[_]]()
Copy link
Member

@maropu maropu Mar 2, 2021

Choose a reason for hiding this comment

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

Seems a reasonable fix cc: @srowen @cloud-fan @viirya

srowen
srowen approved these changes Mar 2, 2021
View reviewed changes
Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

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

OK pending tests

viirya
viirya reviewed Mar 2, 2021
View reviewed changes
Copy link
Member

@viirya viirya left a comment

Choose a reason for hiding this comment

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

looks reasonable. pending test.

@sparkyengine
Copy link
Contributor Author

sparkyengine commented Mar 2, 2021

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40196/

@srowen what does this mean? in jenkins the failure is:

Setting status of 4ffa2576782e410cef4d5cb3d8865362679ff11c to FAILURE with url https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40196/ and message: 'Build finished. '
FileNotFoundException means that the credentials Jenkins is using is probably wrong. Or the user account does not have write access to the repo.
org.kohsuke.github.GHFileNotFoundException: https://api.github.com/repos/apache/spark/statuses/4ffa2576782e410cef4d5cb3d8865362679ff11c {"message":"Not Found","documentation_url":"https://docs.github.com/rest/reference/repos#create-a-commit-status"}

@SparkQA
Copy link

SparkQA commented Mar 2, 2021

Test build #135617 has finished for PR 31689 at commit 4ffa257.

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

@maropu
Copy link
Member

maropu commented Mar 2, 2021

@srowen what does this mean? in jenkins the failure is:

The failure is not related to this PR, so it's okay just to ignore it.

@maropu maropu closed this in b13a4b8 Mar 2, 2021
@maropu
Copy link
Member

maropu commented Mar 2, 2021

Thanks! Merged to master.

@HyukjinKwon
Copy link
Member

HyukjinKwon commented Mar 2, 2021

Yeah seems good

@cloud-fan
Copy link
Contributor

cloud-fan commented Mar 2, 2021

late LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@viirya viirya viirya left review comments

@maropu maropu maropu approved these changes

@srowen srowen srowen approved these changes

Assignees

No one assigned

Labels

SQL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@sparkyengine @maropu @SparkQA @HyukjinKwon @cloud-fan @viirya @srowen