ACCUMULO-4153: Update the getCodec method to no longer be synchronize… #73
Conversation
…d and to use an atomic reference.
| } | ||
|
|
||
| return codec; | ||
| return codec.get(); |
There was a problem hiding this comment.
whether or not resultCodec is null, I'm returning the reference as the code requires someone to have set it based on the simple logic. Could get rid of resultCodec if there is heartburn about not using it above.
There was a problem hiding this comment.
Seems like return resultCodec at the end would be more straightforward, no? Goes a bit to my other comment here.
There was a problem hiding this comment.
Seems like return resultCodec at the end would be more straightforward, no?
Would be nice to avoid two reads of codec.
There was a problem hiding this comment.
I was doing too many things at once and tried to account for the case where we didn't win the CAS operation. I could have checked the result of the compareAndSet method and returned codec.get() in that case and avoided the funny logic to account for that ( which is why I did what I did ). Now that I'm home that's all obvious; however, according to what Brian wrote above I may just take a different approach when I have time again..hopefully tomorrow. I think his tact is simpler and was hoping the PR would give me some insight for such a method.
|
I apparently clicked through to the commit and left a bunch of comments instead of commenting on the PR itself. Sorry about that. Overall, this looks pretty good. Some nit-picky things on the test case (yay, tests!) and a similar recommendation to Marc's earlier note. |
| CompressionCodec getCodec() { | ||
| DefaultCodec resultCodec = codec.get(); | ||
| if (null == resultCodec) { | ||
| DefaultCodec newCodec = new DefaultCodec(); |
There was a problem hiding this comment.
If a bunch of threads all got to this point at the same time, they would all create a new DefaultCodec. Only one will succeed on the compareAndSet below, but the code is still potentially creating many DefaultCodec instances which will get garbage collected.
Since isSupported returns true, I'm wondering if it's possible to simply initialize the member variable at construction time, and then the AtomicReference wouldn't be needed either? But maybe I'm missing something obvious.
There was a problem hiding this comment.
The merit of creating it at construction time depends on how costly the initialization is. I'm under the impression that it's not that bad, so eager initialization is probably ok here and we don't have to worry about concurrency at all. Good point, @brianloss.
There was a problem hiding this comment.
Why not just share a singleton codec instance for each kind of codec? Looking at the implementations (Default, Gzip, Bzip2), they're all effectively stateless (sans the Configuration object you can set on them) because they're factories for streams.
There was a problem hiding this comment.
I had tunnel vision when I looked at the change and didn't even spend much time looking or thinking about anything else. You guys are absolutely right and that negates the need for the AtomicReference. I'll try and apply some time tomorrow to make those changes.
6 participants
…d and to use an atomic reference.
This is based only on JSTACKs across a running system. I've seen improvement as a result with local testing and will test at a larger scale as time permits. I can provide performance numbers if needed for the PR.
I'm basing this against 1.6 as that is where we will test it and use it. It should merge cleanly up to 1.7 and 1.8.
This is a very simple change. I added a unit test ( the last of which shows that we won't run into the problem of creating many )...though if it fails for you let me know!