db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anders Morken <ander...@stud.ntnu.no>
Subject Re: [jira] Commented: (DERBY-801) Allow parallel access to data files.
Date Mon, 18 Sep 2006 21:00:47 GMT
Suresh Thalamati (JIRA):
>     [ http://issues.apache.org/jira/browse/DERBY-801?page=comments#action_12434613 ]

>             
> Suresh Thalamati commented on DERBY-801:
> ----------------------------------------
> 
> Thanks for working on this issue, Anders. I really like your solution to solve
> this issue. Patch is very good, I have only few minor comments/questions.
>  I am really sorry  for not reviewing it sooner. 

Heh, my progress hasn't exactly been fast either. =)

> RAFContainerFactory.java
> ------------------------
> 
> Logic in this new class seems to be deciding whether to load RafContainer.java or
> the RafContainer4.java based on the JVM.  I am not sure, if  this logic is
> necessary here. Did you consider using basic services to load the java classes 
> specific to a JVM ?

Can you do that? Never occured to me, I'll check it out. Thanks for the
tip. Any pointers and tips would be welcome, but given enough time and
source code perusal I'll probably figure it out on my own as well. From
a quick look at modules.properties it looks pretty straightforward, at
least if adding "J4" to a module name means "load this in Java 1.4 and
up". =)

> I think basic services has support to boot a specific factory implementation
> based on the JVM using modules.properties. For example in the current
> scenario, one can extend BaseDataFileFactory.java class to  
> implement newContainerObject(), which will return the RafContainer4( ..).  add
> the new class to modules.properties to boot only on versions >=jdk14.
> 
> 
> 
> In RafContainer4.java :
> ---------------------
> 
> 1) I think following import is not needed. 
> 
> +import java.io.*; 

I've narrowed it down to java.io.IOException. =)

> 
> 2) Is it really necessary to rewind() the buffers in readFull/writeFull ? From what I
understood, 
>    there is a new ByteBuffer object being created on  both read/write page
>    methods.  
> 
> +        dstBuffer.rewind(); // Reset buffer position before we start read
> and 
> +        srcBuffer.rewind(); // Reset buffer position before we start writing.

Naah, not really. Just unnecessary paranoia. =)

> 3) do we really need the following method ? 
> 
> +    final protected FileChannel getChannel() {
> +        return ourChannel;
> +    }

Naah, not really. It acted as a synchronization point at one time in the
life of the patch, but the synchronization has been made excplicit in
readPage() and writePage() instead at a later stage. =)

> 4) I noticed, there is new encryption buffer created on every writePage() call,
>    if the database is encrypted. This may cause jvm peak memory usage increase,
>    when there is a checkpoint, if there are lot of dirty pages in the cache and
>    if garbage collection is not happening fast enough.  I hope this does not
>    lead to out of memory errors!
> 
>    We may need to implement some kind of scheme, that will help in reuse of
>    the encryption buffers. 

This is a (classical) choice between concurrency and resource
consumption.  On one hand, object creation and garbage collection from
the eden space is pretty cheap on 1.4 and newer VMs, but we'll force
additional GC runs... This is a complex subject, with many variables. =)

I suppose it could be interesting to do a little throughput testing with
this patch and an encrypted database and see if there is any impact. I
can't really say that I have any idea if recycling a single buffer or
creating lots of buffers is the cheaper way...

> 5) I am ok with readPage() and writePage() routines in RafContainer4.java. 
>    just curious , if you considered implementing  new read/write..etc  calls in 
>    the RafContainer4.java using file channel and just wrapper methods in the
>    RafContainer.java using the existing random access file,  instead of 
>    overriding readPage()/writePage() ...etc. 

Uhm, I'm sorry, I'm not sure I understand what you're thinking of here?
Could you elaborate a bit? =)

When I started work on this patch, readPage() and writePage() just
seemed like the smallest pieces of RAFContainer that could be replaced
easily to retrofit NIO support. I agree if you think the current
solution is a bit of a "bolt-on" fix, but it was relatively non-invasive
in the original code (which appealed to me as a newbie to Derby) and
hopefully efficient enough on new JVMs anyway.

If we wanted to go even further we could consider using memory mapped IO
- then the parts of Derby that modify data pages could access the memory
mapped file directly and save some overhead, but that probably requires
a bit of a rework of some interfaces and the code that modifies data
pages. And then we have the case of large databases and 32-bit address
spaces... =)

> 6) Please file a JIRA  to enhance StorageFactory interfaces to support NIO. 

There's already DERBY-262 in Jira, but it's a bit unspecific... Anything
in particular you think needs to be added to the interfaces?

Thanks for your advice,
-- 
Anders Morken

My opinions may have changed, but not the fact that I am right!

Mime
View raw message