-
Notifications
You must be signed in to change notification settings - Fork 1
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
Undo background thread execution #1
base: wait-free-MemoryMappedFileManager
Are you sure you want to change the base?
Undo background thread execution #1
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SandishKumarHN thank you for preparing a commit against my branch.
@@ -918,41 +909,10 @@ private void scheduleWriteTouch(final long bufferWatermarkOffsetInFile, final lo | |||
// If we really have to cancel write touch tasks, it means that the logging rate is so high that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The containing method, scheduleWriteTouch()
should be removed completely. Generally, the new version of the class shouldn't even mention "touch", "pretouch". All related methods should be removed.
@@ -496,13 +496,6 @@ private void doCloseAndSwitchRegionExclusively() { | |||
// it's return it is guaranteed that all concurrent writers has completed. | |||
if (manager.closing) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unmapMappedBuffer()
shouldn't be wrapped in if (manager.closing)
. Try to understand why the code with background execution used to have this if (manager.closing)
condition. When you understand it, you'll realize why if (manager.closing)
is not needed without background threading.
@@ -972,15 +932,10 @@ private void drainWriteTouchTasks() { | |||
} | |||
|
|||
private void asyncCreateNextRegion(final Region currentRegion, final long newMappingOffsetInFile) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is no longer "async". You cannot just rename it to "createNextRegion" because there is already a method with this name. So I think this method should be now inlined at its sole usage site.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I Will name it as syncCreateNextRegion for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks OK for testing.
@leventov below are the charts hosted on Github pages |
|
Also, did you benchmark log4j master back then, or the current master? |
Undo background thread execution