Skip to content
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.

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

Remove LZO global lock "hack" that was for an old JVM bug #128

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

skwslide
Copy link

LzoCompressor and LzoDecompressor have a global lock protecting all GetDirectBufferAddress calls. This PR removes the global lock.

Apparently, the global lock's origin is HADOOP-3604, to work around JDK-6852404, but which has been fixed for several years in all Java versions.

The global lock imposes a severe performance penalty in a highly multithreaded environment. I have a Java process with 64 threads on a 64-core machine to decompress LZO files concurrently, and around half of the threads are blocked on the global lock at any moment. The problem is not observed in a 16-way concurrency setup.

@sjlee
Copy link
Collaborator

sjlee commented Aug 22, 2017

Thanks for reporting the issue and also providing the fix. It's a good find, and the fix LGTM.

I am no longer at Twitter, and ideally a Twitter engineer should also approve this and merge it.

cc @jrottinghuis

@jrottinghuis
Copy link
Contributor

Looks reasonable indeed. Builds locally. I'll try to test this on a cluster and see if I can tell any performance difference, report back and merge (assuming positive results).

@CLAassistant
Copy link

CLAassistant commented Jul 18, 2019

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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