[
https://issues.apache.org/jira/browse/HBASE-10015?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13830857#comment-13830857
]
Lars Hofhansl commented on HBASE-10015:
---------------------------------------
No longer sure that patch is good. The problem was that StoreScanner.peek() is
synchronized.
With the patch peek() will use the old scanner stack, looking at
MemstoreScanner and StoreFileScanner it *is* correct (both scanners just return
the current KV and StoreFileScanner does not touch its reader during peek), but
it is fragile, it also requires StoreScanner to hang on to all
StoreFileScanners and the MemstoreScanner, until checkReseek is called or the
scanner is closed. This in turn means that we keep references to the readers
open, etc.
I keep doing that... Filing jiras with patches and then finding that the fix is
a bad idea.
Well, at least I learned about modern CPU and that synchronized in tight loops
is very expensive.
I will keep thinking about this. Unscheduling for now.
> Major performance improvement: Avoid synchronization in StoreScanner
> --------------------------------------------------------------------
>
> Key: HBASE-10015
> URL: https://issues.apache.org/jira/browse/HBASE-10015
> Project: HBase
> Issue Type: Bug
> Reporter: Lars Hofhansl
> Assignee: Lars Hofhansl
> Attachments: 10015-0.94-v2.txt, 10015-0.94-v3.txt, 10015-0.94-v4.txt,
> 10015-0.94-withtest.txt, 10015-0.94.txt, 10015-trunk-v2.txt,
> 10015-trunk-v3.txt, 10015-trunk-v4.txt, 10015-trunk-v4.txt,
> 10015-trunk-v4.txt, 10015-trunk.txt, TestLoad.java
>
>
> Did some more profiling (this time with a sampling profiler) and
> StoreScanner.peek() showed up a lot in the samples. At first that was
> surprising, but peek is synchronized, so it seems a lot of the sync'ing cost
> is eaten there.
> It seems the only reason we have to synchronize all these methods is because
> a concurrent flush or compaction can change the scanner stack, other than
> that only a single thread should access a StoreScanner at any given time.
> So replaced updateReaders() with some code that just indicates to the scanner
> that the readers should be updated and then make it the using thread's
> responsibility to do the work.
> The perf improvement from this is staggering. I am seeing somewhere around 3x
> scan performance improvement across all scenarios.
> Now, the hard part is to reason about whether this is 100% correct. I ran
> TestAtomicOperation and TestAcidGuarantees a few times in a loop, all still
> pass.
> Will attach a sample patch.
--
This message was sent by Atlassian JIRA
(v6.1#6144)
