NIO read/write review

[Vlad Rabkin]

Hi,

Could you please review my read/write methods:

    private void cashDatabase()
     throws IOException {
      cache_ = new DBCache();
      String[] recordvalues = null;
      int totalRecords = getFileRecordNumber();
       // cash all existing records
       for (int recordCount = 0; recordCount < totalRecords; recordCount++) {
       ByteBuffer bytes =
          ByteBuffer.allocate(fileHeader_.getRecordLength());
       CharBuffer chars =
          CharBuffer.allocate(fileHeader_.getRecordLength()-1);
       fileChannel_.read(bytes, getFilePos(recordCount));
       bytes.flip();
        byte flag = bytes.get();
         DBRecord record = new DBRecord();
         // check if the record is not deleted
 if (flag == ACTIVE_RECORD) {
          chars = charset.newDecoder().decode(bytes);
          recordvalues = new String[fileHeader_.getColumnsNumber()];
           // fill the data record
           for (int col = 0; col < fileHeader_.getColumnsNumber(); col++) {
              char[] charArray = new char[fileHeader_.getColumns()[col].getLength()];
              chars.get(charArray);
               String value = new String(charArray);
               int nullPos = value.indexOf("\0");
               recordvalues[col] =
                  (nullPos < 0) ? value : value.substring(0, nullPos);
           }
           record.setData(recordvalues);
         }
           // insert record in the cash
           cache_.add(record);
       }
       fileChannel_.force(false);
    }

    private void writeRecord(int recNo, String[] data)
     throws DatabaseSystemException{
     try{
       DBColumnModel[] columns = fileHeader_.getColumns();
       ByteBuffer bytes =
          ByteBuffer.allocate(fileHeader_.getRecordLength());
       CharBuffer chars =
          CharBuffer.allocate(fileHeader_.getRecordLength()-1);
       // write the first byte in the record indicating a valid record
       bytes.put(ACTIVE_RECORD);
       // write all fields in the record
       for (byte col = 0; col < fileHeader_.getColumnsNumber(); col++) {
         String fieldToWrite = (data[col]==null ? "" : data[col]);
           int space =
              columns[col].getLength() - fieldToWrite.length();
          // if length of the field from request is less than database defined,
          // terminate the string with null.
          if (space > 0) {
            char[] cbuf = new char[space];
            Arrays.fill(cbuf, '\0');
            fieldToWrite += new String(cbuf);
          } 
       //fill CharBuffer with record value
       chars.put(fieldToWrite);
      }
         chars.flip();
         //Encodes the CharBuffer containing the record into ByteBuffer.
         charset.newEncoder().encode(chars, bytes, true);
         bytes.flip();

         //write record in the database
         fileChannel_.write(bytes, getFilePos(recNo));
         fileChannel_.force(false);
     } catch (IOException ioe) {
             throw new DatabaseSystemException(DB_EXEPTION, ioe);
     }
    }

Any suggestions?

1) Does it make sense to make encoder and descoder as static private variables, instead of initiating them every time?
2) Should I invoke fileChannel_.force(false) while reading every time a record is read, or it is Ok now?
3) I synchronize on database, not record. Does it make sense allocate Byte/CharBuffer only one time, or ir is Ok now leaving a possibility to switch synchronization to record-level without changing a IO read/write methods?
Tx,
Vlad
[ September 15, 2003: Message edited by: Vlad Rabkin ]

[Jonathan Liu]

I have a question about your naming convention
you use cache_ fileHeader_ these kinds of instance variable
so you don't follow Sun's naming convention ?

[Andrew Monkhouse]

Hi Vlad,

    private void cashDatabase() throws IOException {
      cache_ = new DBCache();

You have the variable 'cache' but your method name is 'cash...'. If you had used cash throughout, then I would ignore it. But in using both cache and cash, my immediate thought is that this is not a typo, but a deliberate coding scheme, so this method is somehow doing some in the database related to money.

    DBRecord record = new DBRecord();
    // check if the record is not deleted
    if (flag == ACTIVE_RECORD) {

Why is the instantiation of the record outside the if block? Unless I miscounted my braces, you are only using the record variable if the flag indicates that the record is not deleted.

Does it make sense to make encoder and descoder as static private variables, instead of initiating them every time?

I think that either case could be argued. I don't think that they are getting that much use that having static instances of them always around is going to gain you much.

Should I invoke fileChannel_.force(false) while reading every time a record is read, or it is Ok now?

I think you only need to call force() when you are writing data. You should not have to force your updates while reading (updates while reading? ).

I synchronize on database, not record. Does it make sense allocate Byte/CharBuffer only one time, or ir is Ok now leaving a possibility to switch synchronization to record-level without changing a IO read/write methods?

Sorry, I don't understand what you are asking for here. I don't see the connection betwen "synchronizing on the database or record" and the "allocation of the Byte/CharBuffer".
Regards, Andrew

[Vlad Rabkin]

Hi,
Tx for your feedbacks.
Jonathan

you use cache_ fileHeader_ these kinds of instance variable

It is very common to use xxx_ for private member variables. I don't need to use this.xxx to differ them for local ones. I was wondering also as I have read some books and saw code sample with private variables having xxx_ signature (e.g. "RMI" from O'Relly). I had a project a year ago, where I met a freelancer with experience over 25 years. He was the best deveoper and architect I have ever seen. He used the same thing. So, I decided to use it also.
Do you think Sun will not approciate it?
Andrew

so this method is somehow doing some in the database related to money.

It is my bad English. I changed name of the variable, but I've forgot to change name of the method. Tx!

Why is the instantiation of the record outside the if block?

If the record is deleted I still have an instance of Record in the List.
Record class has two properties : String[] data and Long cookie. If the the record is deleted data = null, but Record object must be initialized. In this case if a record is deleted it can be first locked if needed.
Example lock method: I lock first the record and then check if it is not deleted.

Sorry, I don't understand what you are asking for here. I don't see the connection betwen "synchronizing on the database or record"

There was a small discussion beteen Jim and Phil.
Phil suggested to allocate Byte/CharBuffer once and reuse it for writing/reading each record. It should be performant.
In Jim's design it not possible since he synchronizes on record. It means if two threads trying to write records in database (Thread 1 record #4, Thread 2 record #5) they will proceed concurrently. If you use ByteBuffer as instance variable, both threads will access it concurrently. Therefore it is neccessary in Jims case have ByteArray as an local variable and iallocate it everytime you read/write.
Since I block whole database while reading/writing it is OK to optimize it. At least in cacheDatabase() method I could allocate Byte/CharBuffer one time before looping over all record and reuse it every time for each record. No I allocate Buffer everytime in the loop.
So, my question is if it makes sense to follow Phil's suggestion...

Tx,
Vlad

[Tony Collins]

I think the xxx_ for member variabes is good practice, if SUN think differentally I don't know why. The reason you use the _ is so instance variables don't clash with local variables, it's good practice.
Instead of having recordNumber and recordNo for example which can only be confusing, you could have recordNumber_ and recordNumber. Good for those of us with no imagination as well.
Tony

[Tony Collins]

This is what SUN says about variable names, so I think you're ok with the underscore, I think it's good practice.

Except for variables, all instance, class, and class constants are in mixed case with a lowercase first letter. Internal words start with capital letters. Variable names should not start with underscore _ or dollar sign $ characters, even though both are allowed.
Variable names should be short yet meaningful. The choice of a variable name should be mnemonic- that is, designed to indicate to the casual observer the intent of its use. One-character variable names should be avoided except for temporary "throwaway" variables. Common names for temporary variables are i, j, k, m, and n for integers; c, d, and e for characters.

Tony
[ September 16, 2003: Message edited by: Tony Collins ]

[Vlad Rabkin]

Hi Tony,

Variable names should not start with underscore _ or dollar sign $ characters, even though both are allowed.

xxx_ is not in the "black list"
Good point.
Tx,
Vlad

[Philippe Maquet]

Hi Vlad,
Vlad:
So, my question is if it makes sense to follow Phil's suggestion...
Of course it makes sense, as following my suggestions in general...
OK I stop kidding. Yes, I would allocate that ByteBuffer only once outside of the loop. Think of the fact that that ByteBuffer is a local variable, in such a way that whatever your design, it will never get accessed by some other thread.
Now, IMO, there is a huge optimization to be done : don't instantiate the additional CharBuffer. You don't need it for two reasons :

You don't need to allocate your CharBuffer chars, because as a few lines later you assign "charset.newDecoder().decode(bytes)" to it, you immediately loose the buffer you pre-allocated

You don't need any CharBuffer variable at all, charset.decode(aByteBuffer).toString() doing the job directly.

Here is how a Field reads its own String value from a ByteBuffer record in my implementation :

    /**
     * Extracts the field value from a <code>ByteBuffer</code> record buffer
     * and converts it into a <code>String</code>.
     *
     * @param recordBuffer a record buffer
     * @return a field value as a <code>String</code>
     */
    protected String getValue(ByteBuffer recordBuffer) {
        recordBuffer.limit(offset + length * metaData.bytesPerChar);
        recordBuffer.position(offset);
        String retVal = metaData.charset.decode(recordBuffer).toString();
        int zeroPos = retVal.indexOf(0);
        if (zeroPos >= 0) {
            retVal = retVal.substring(0, zeroPos);
        }
        return retVal;
    }

One more little possible optimization IMO : you don't need the newDecoder() part in the expression "charset.newDecoder().decode(bytes)". charset.decode() uses an internal decoder. Here is how Charset.decode() is documented :
Excerpt from SUN API doc:
public final CharBuffer decode(ByteBuffer bb)
Convenience method that decodes bytes in this charset into Unicode characters.
An invocation of this method upon a charset cs returns the same result as the expression
cs.newDecoder()
.onMalformedInput(CodingErrorAction.REPLACE)
.onUnmappableCharacter(CodingErrorAction.REPLACE)
.decode(bb);
except that it is potentially more efficient because it can cache decoders between successive invocations.

For the rest, I agree with all what Andrew wrote above as well as with your reply to his post.
Best,
Phil.

[Vlad Rabkin]

Hi Phil,

Yes, I would allocate that ByteBuffer only once outside of the loop. Think of the fact that that ByteBuffer is a local variable, in such a way that whatever your design, it will never get accessed by some other thread.

That is what I thought. Ok, agreed.

You don't need to allocate your CharBuffer chars, because as a few lines later you assign "charset.newDecoder().decode(bytes)" to it

Good idea. I will have to think some time about this. I could probably ask you a question about. decode() internally must anyway allocate CharArray (because it returns CharArray).
What you do is actualling following:
1) you decode for each field separately, allocating implicetely CharArray for each field!?
2) I could decode the whole record in string, but if the record is very large in future (let's say 5 KB) it would be probably better first deciode it in CharBuffer and then fill step by step field String from the Buffer.

One more little possible optimization IMO : you don't need the newDecoder() part in the expression

Agreed.
Tx,
Vlad
[ September 16, 2003: Message edited by: Vlad Rabkin ]

[Philippe Maquet]

Hi Vlad,

1) you decode for each field separately, allocating implicetely CharArray for each field!?

I suppose that by CharArray you mean CharBuffer. Yes, implicitely for each field == multiple smaller ones, once per record. In comparison, you allocate one bigger CharBuffers for each records, but you do it twice (one time explicitely and one time implicitely in decode(), the first one being useless).

2) I could decode the whole record in string, but if the record is very large in future (let's say 5 KB) it would be probably better first deciode it in CharBuffer and then fill step by step field String from the Buffer.

As individual field values are null-terminated, you'd add more complexity IMO to extract individual field values from that record String.
Best,
Phil.

[Max Habibi]

Originally posted by Vlad Rabkin:
Hi,
It is very common to use xxx_ for private member variables. I don't need to use this.xxx to differ them for local ones. I was wondering also as I have read some books and saw code sample with private variables having xxx_ signature (e.g. "RMI" from O'Relly). I had a project a year ago, where I met a freelancer with experience over 25 years. He was the best deveoper and architect I have ever seen. He used the same thing. So, I decided to use it also.
Do you think Sun will not approciate it?
Vlad

I've never seen it in any of Sun's code, and I know I ask the developers on my team to adhere to a common style(which doesn't include the style above). There's nothing wrong with it, IMO, However, it's not(again, IMO) very java-like. It seems like something a C++ programmer would do.
All best,
M

[Vlad Rabkin]

Hi Max,

However, it's not(again, IMO) very java-like.

Ok, I will change it not to play with Sun...
Tx,
Vlad

[Vlad Rabkin]

Hi,
Here is corrected read:

 
    private void cacheDatabase()
     throws IOException {
      cache = new DBCache();
      String[] recordvalues = null;
      int totalRecords = getFileRecordNumber();
       ByteBuffer bytes =
          ByteBuffer.allocate(fileHeader_.getRecordLength());
       // cash all existing records
       for (int recordCount = 0; recordCount < totalRecords; recordCount++) {
       bytes.clear();
       fileChannel.read(bytes, getFilePos(recordCount));
       bytes.flip();
        byte flag = bytes.get();
         DBRecord record = new DBRecord();
         // check if the record is not deleted
 if (flag == ACTIVE_RECORD) {
         int fieldLength = 0;
         int offset = bytes.position();
           recordvalues = new String[fileHeader_.getColumnsNumber()];
           // fill the data record
           for (int col = 0; col < fileHeader_.getColumnsNumber(); col++) {
              fieldLength = fileHeader_.getColumns()[col].getLength();
               bytes.limit(offset + fieldLength);
               bytes.position(offset);
               String value = charset.decode(bytes).toString();
               int nullPos = value.indexOf("\0");
               recordvalues[col] =
                  (nullPos < 0) ? value : value.substring(0, nullPos);
              offset += fieldLength;
;
              }
           }
           record.setData(recordvalues);
         }
           // insert record in the cash
           cache.add(record);
       }
    }

Phil,
1) Honestly saying I don't really understand why it should be much faster since CharSet.decode() must now allocate internally an CharBuffer() for every field, not a record!?
2) I didn't change write method for CharSet.newEncoder().encode()-> CharSet.encode(), because CharBuffer is one byte less, than ByteBuffer, and it is impossible then to add this byte (status flag) without creating a new ByteBuffer. Of course, I could allocate CharBuffer with the same number of byte, but what for: CharBuffer doesn't have status flag (active/deleted)?
Tx,
Vlad

[Philippe Maquet]

Hi Vlad,

1) Honestly saying I don't really understand why it should be much faster since CharSet.decode() must now allocate internally an CharBuffer() for every field, not a record!?

If you compare your new code cacheDatabase() method with the previous one :
You had :

For each record, a ByteBuffer allocated for the whole record

For each record, a useless explicit allocation of a CharBuffer for the whole record

For each record, an implicit allocation of a CharBuffer for the whole record

For each field, a char[] charArray allocated

You now have :

Only once, , a ByteBuffer allocated for all records

For each field, an implicit allocation of a CharBuffer

I am pretty sure the second solution is more efficient. Now if you really want to know, you may test it by reading 1000000 times the same record with solution 1 and solution 2 and compare run times. I usually use System.currentTimeMillis() in such tests.

2) I didn't change write method for CharSet.newEncoder().encode()-> CharSet.encode(), because CharBuffer is one byte less, than ByteBuffer, and it is impossible then to add this byte (status flag) without creating a new ByteBuffer. Of course, I could allocate CharBuffer with the same number of byte, but what for: CharBuffer doesn't have status flag (active/deleted)?

Well if your ByteBuffer size includes the deleted flag, if you get its array() to fill it with nulls (byte) 0 and set its first byte with the "not-deleted" byte value, all your fields may write themselves in the ByteBuffer without any need for an intermediate CharBuffer :
In MetaData (called from data by updateRecord() and createRecord()) :

    protected void setValues(ByteBuffer recordBuffer, String[] values) {
        if (values.length != fieldsCount()) {
            throw new IllegalArgumentException(
                "Invalid field values array length");
        }
        byte[] b = recordBuffer.array();
        Arrays.fill(b, (byte) 0);
        b[0] = NOT_DELETED_FLAG_VALUE;
        for (short i = 0; i < fieldsCount(); i++) {
            ( (Field) fields.get(i)).setValue(recordBuffer, values[i]);
        }
        recordBuffer.rewind();
    }

Then in Field :

    protected void setValue(ByteBuffer recordBuffer, String value) {
        ByteBuffer fieldByteBuffer =
            metaData.charset.encode(value);
        recordBuffer.limit(offset + length * metaData.bytesPerChar);
        recordBuffer.position(offset);
        recordBuffer.put(fieldByteBuffer);

As you can see, I use much "delegation" in my design because I find it easy to do so, but sometimes I wonder if it is good practice and easy to be understood by others :
Data knows nothing about records structure, so it delegate convertions between String[] records and bytes[] records to its MetaData ... which knows about the charset to be used, but nothing about its fields individual lengths and offsets, so it delegates in turn final encoding/decoding to the fields themselves. Any comment from anybody would be welcome about that repartition of responsabilities among the three classes.
Best,
Phil.

[Vlad Rabkin]

Hi Phil,
Tx for your reply.

I am pretty sure the second solution is more efficient. Now if you really want to know, you may test

I won't do it, I trust you Well, in my old solution I had anyway to initialze array of chars every time. So, I am sure you are right.

all your fields may write themselves in the ByteBuffer without any need for an intermediate CharBuffer

Very elegant! Seems that you just don't like CharBuffers I have used them, becuase I rewrite code from IO. I used Streams (IO) earlier and just took CharBuffer instead of StreamReader/Writer. If you don't mind I will
use idea. Tx!

As you can see, I use much "delegation" in my design because I find it easy to do so, but sometimes I wonder if it is good practice and easy to be understood by others :

I am trying alway to break down big classes to small ones. I like your idea. I had something the same: I had separate Request/Response Parser classes, separate LockManager and so on, but I decided to insert most of these classes as methods in Data, just to follow Max advices.
I like the way you have it, moreover I respect you that you didn't give up your principles of programming in opposite to me.
Best,
Vlad

[Vlad Rabkin]

Hi Phil,
I have implemented write method the way you suggested. It works great. Thanx.

      ByteBuffer bytes =
          ByteBuffer.allocate(fileHeader_.getRecordLength());
       bytes.put(ACTIVE_RECORD);
//       byte[] bArray = bytes.array();
//       Arrays.fill(bArray, (byte) 0);
//       bArray[0] = ACTIVE_RECORD;
       int offset = 1;
       int fieldLength = 0;
       // write all fields in the record
       for (byte col = 0; col < fileHeader_.getColumnsNumber(); col++) {
         fieldLength = columns[col].getLength();
         String fieldToWrite = (data[col]==null ? "" : data[col]);
          int space = fieldLength - fieldToWrite.length();
          if (space < 0)
           // if length of the field from request is greater
           // than database defined, throw an Exception
           throw new IllegalArgumentException(
                    "Database: <" + columns[col].getName()
                            + "> field in request is too long");
         ByteBuffer fieldByteBuffer = charset.encode(fieldToWrite);
         bytes.limit(offset + fieldLength);
         bytes.position(offset);
         bytes.put(fieldByteBuffer);
         offset += fieldLength;
      }
         bytes.rewind();
         //write record in the database
         fileChannel.write(bytes, getFilePos(recNo));
         fileChannel.force(false);

I have 1 suggestion and one question to you:
1. Arrays.fill(b, (byte) 0); is not needed, because when an ByteBuffer is allocated it has all his bytes set to 0. Probably you allocate it only one time, not by each write, but in this case it would easer just to clear() method on ByteBuffer().
2. metaData.charset.encode(value); I don't know if encode() method is synchrone. Specification says Encoder will be cached for optimization.
If you we want to allow in future concurrent writes on different records:
ByteBuffer is Ok, since it will be a local variable, but Encoder will be the same object used by all threads, since charset is static. It is not then thread safe in case of record based synchronization. Isnt't it?
Best,
Vlad
[ September 17, 2003: Message edited by: Vlad Rabkin ]
[ September 17, 2003: Message edited by: Vlad Rabkin ]

[Philippe Maquet]

Hi Vlad,
Your code looks correct to me.

I have 1 suggestion and one question to you:
1. Arrays.fill(b, (byte) 0); is not needed, because when an ByteBuffer is allocated it has all his bytes set to 0. Probably you allocate it only one time, not by each write, but in this case it would easer just to clear() method on ByteBuffer().

Good point. I checked it and it's allocated for each write. Notice that as MetaData receives it as a parameter, I had to check all its calls to verify if my Arrays.fill(b, (byte) 0) was needed or not. So, you may see it as just defensive from the MetaData class point of view. But you are 100% right. Where you are not right, it's about the ByteBuffer.clear() method. The latter actually is badly named :

public final Buffer clear()
Clears this buffer. The position is set to zero, the limit is set to the capacity, and the mark is discarded.
Invoke this method before using a sequence of channel-read or put operations to fill this buffer. For example:
buf.clear(); // Prepare buffer for reading
in.read(buf); // Read data
This method does not actually erase the data in the buffer, but it is named as if it did because it will most often be used in situations in which that might as well be the case.

2. metaData.charset.encode(value); I don't know if encode() method is synchrone. Specification says Encoder will be cached for optimization.
If you we want to allow in future concurrent writes on different records:
ByteBuffer is Ok, since it will be a local variable, but Encoder will be the same object used by all threads, since charset is static. It is not then thread safe in case of record based synchronization. Isnt't it?

One more good point !
About CharsetEncoder, I read that "Instances of this class are not safe for use by multiple concurrent threads." and the like for class CharsetDecoder. We don't allow concurrent writes, but I do allow concurrent reads ! So I should adapt my Field.getValues() method. Thanks again.
Good team work, isn't it ?
Best,
Phil.

[Vlad Rabkin]

Hi Phil,

Buffer clear()
This method does not actually erase the data in the buffer

Tx 1001 times! As you can see I used clear() for reading. I thought it works, because every read has just overwritten the ByteBuffer.
But you are right! I just didn't carefully read API. Still it would work,
because old ByteBuffer will be anyway overwritten by a new record. Is it correct?

We don't allow concurrent writes, but I do allow concurrent reads !

How do you want to fix it? I don't have good ideas I have to think about it.
May be:
- instead of Charset.encode -> charset.newDecoder.encode? If so, are you sure that newDecoder() is thread-safe?
- make you own methods, wrapping charset.newDecoder, which would be synchronized?

Good team work, isn't it ?

Of course! I hope that others read this topic, because everybody can do the same misjudgement.
Best,
Vlad
P.S. After writing this mail I realized that you gave up the idea to cache partially the database. Didn't you? E.g. I cache database only one time. SO all client reads work only with cache, not with file. So, it should be a problem with decoder. Still, it is interesting problem.
[ September 17, 2003: Message edited by: Vlad Rabkin ]
[ September 17, 2003: Message edited by: Vlad Rabkin ]
[ September 17, 2003: Message edited by: Vlad Rabkin ]

[Philippe Maquet]

Hi Vlad,
Vlad:
Tx 1001 times! As you can see I used clear() for reading. I thought it works, because every read has just overwritten the ByteBuffer.
Sorry, I didn't notice that in your code...
Vlad:
How do you want to fix it? I don't have good ideas I have to think about it.
May be:
- instead of Charset.encode -> charset.newDecoder.encode?
Well I do exactly what SUN does in Charset.decode() (as documented in the doc and as I saw in the JDK sources).
Unfortunately instead of this elegant line :

    String retVal = metaData.charset.decode(recordBuffer).toString();

I have now these ones :

    String retVal = null;
    try {
        retVal = metaData.charset.newDecoder()
               .onMalformedInput(CodingErrorAction.REPLACE)
               .onUnmappableCharacter(CodingErrorAction.REPLACE)
               .decode(recordBuffer)
               .toString();
        }
    catch (CharacterCodingException e) {
        throw new Error(e);// Can't happen
    }

Here are the sources of Charset.decode() :

    public final CharBuffer decode(ByteBuffer bb) {
try {
    return ThreadLocalCoders.decoderFor(this)
.onMalformedInput(CodingErrorAction.REPLACE)
.onUnmappableCharacter(CodingErrorAction.REPLACE)
.decode(bb);
} catch (CharacterCodingException x) {
    throw new Error(x);// Can't happen
}
    }

Vlad:
If so, are you sure that newDecoder() is thread-safe?
Well, I hope so, but I cannot prove it... the CharsetEncoder got for sure as it will be locally instantiated, but newEncode() itself ?!
Anyway I hate that solution, it's really ugly to read. For encoding, as only one write may occur at any given time, I may safely come back to my previous simple solution, which is faster BTW as it uses the catched CharsetDecoders.
The problem only concerns reads. I think I'll come back to the previous solution too and synchronize on the charset. Reads would still be concurrent, except during the decoding process. As many reads are done in cache (no decoding) and as when there are done in file decoding is only a very short part of the job, it should not be that much slower IMO. And I would benefit again of the decoders cache which I lost here.
Vlad:
E.g. I cache database only one time. SO all client reads work only with cache, not with file.
Yes, you are not concerned by this issue : no concurrent writes and reads from file performed only once by only one thread. So it's pure altruism of you to interest yourself in it !
Best,
Phil.

[Jim Yingst]

Vlad asked me to look at this thread. Unfortunately I don't have time right now to read everything above really carefully, so I may miss some subtleties. But here's how I handled these issues in my own design:
Just as concurrent reads & writes forced me to allocate a new, local ByteBuffer for each read or write, they also prevent me from relying on any shared CharsetEncoder or CharsetDecoder. These are not thread-safe, so each thread needs its own. However Charset itself is thread-safe. I just use Charset.decode() or Charset.encode() to encode/decode the whole ByteBuffer at once (excluding the initial isDeleted flag). I considered replacing the single encode()/decode() with a few more lines as Philippe did, but decided it wasn't necessary, as I felt no need to override the default options in this case, and as PM noted, it does make the code a bit uglier. I experimented with different options here to see if I could notice any effect on performance, and I didn't. Then I moved to full caching and the performance impact of read was even less significant, so I haven't really given the matter any more thought since then - I just made the code as simple as I could using NIO, which meant using Charset's decode() and encode().

[Philippe Maquet]

Hi Jim,
Thanks for joining in.
However Charset itself is thread-safe.
You're right. It is explicitely stated in Charset doc.
I just use Charset.decode() or Charset.encode() to encode/decode the whole ByteBuffer at once.
But here came the question :
Charset.decode() doc (similar for encode) states this :

An invocation of this method upon a charset cs returns the same result as the expression
cs.newDecoder()
.onMalformedInput(CodingErrorAction.REPLACE)
.onUnmappableCharacter(CodingErrorAction.REPLACE)
.decode(bb);
except that it is potentially more efficient because it can cache decoders between successive invocations.

It can cache decoders which, as you mention yourself, are not thread-safe. Unfortunately, I couldn't access the sources of ThreadLocalCoders.decoderFor() which returns the decoder. Reading you, I suppose now that as Charset is said thread-safe ThreadLocalCoders.decoderFor() called from Charset.decode() is thread-safe too. But is just a guess, right ?
I considered replacing the single encode()/decode() with a few more lines as Philippe did, but decided it wasn't necessary, as I felt no need to override the default options in this case, and as PM noted, it does make the code a bit uglier.
Yes they are ugly, but they don't "override the default options" of decode(). I will not keep them anyway.
Best,
Phil.
[ September 17, 2003: Message edited by: Philippe Maquet ]

[Vlad Rabkin]

Hi Phil,
Jim:

...encode/decode the whole ByteBuffer at once.

We don't do it. We feel encode/decode it by blocks.
Phil:

but they don't "override the default options" of decode().

If don't, why shouldn't we then use encode()/decode() convinience methods as Jim mentioned?
Vlad

[Philippe Maquet]

Hi Vlad,

If don't, why shouldn't we then use encode()/decode() convinience methods as Jim mentioned?

Well, it seems that we could use them now that we consider (or could consider) them as thread-safe despite the questions posted above. Now, if we want to emulate Charset.decode() with a CharsetDecoder, we should use the latter in the same way as Charset.decode() does. Hence the additional lines (notice that I posted the full source of SUN's Charset.decode() in the posted time-stamped "posted September 17, 2003 09:00 AM" under "Here are the sources of Charset.decode()"). By reading them, you'll see what I mean.
Best,
Phil.

[Vlad Rabkin]

Hi Phil,
Sorry, but I have lost idea what you mean:
Your version:

 retVal = metaData.charset.newDecoder()... 

Sun version:

 return ThreadLocalCoders.decoderFor(this)... 

By doing this you try to open explicetly a new decoder to guarantee thread-safety.
First, it doesn't bring much because .newDecoder() must be synchronized
Second, Sun says about charset (As Jim mentioned also):

All of the methods defined in this class are safe for use by multiple concurrent threads.

So, whatever Sun caches there to optimize performance, we can be guaranteed, that it is thread-safe. So, there is no any problem to call just charset.decode(). Right?
If not, and if you want to use this ugly code, could you explain what for?
Sorry, my question can be stupid, but really don't see any reason to make this:

 retVal = metaData.charset.newDecoder()... 

since we found out that metaData.charset.decode() is thread safe!?

Tx,
Vlad

[Philippe Maquet]

Hi Vlad,
It seems that you are explaining to me things that I already understood and ... explained myself before.
My decision is this one :
As Charset is stated thread-safe in its doc (as Jim noticed), and despite the facts that :

CharsetDecoder / CharsetEncoder are not (as you noticed, do you remember)

Charset.decode() and Charset.encode() use a cache of decoders/encoders with no visible syncronization

I'll use Charset.decode() / encode() as I did before.
Best,
Phil.

[Vlad Rabkin]

Hi Phil,
Phil

It seems that you are explaining to me things that I already understood and ... explained myself before

No way! I was asking, not explaining.
Phil

Yes they are ugly.... . I will not keep them anyway.

That's why I concluded that you wanted to avoid using Chatset.encode()/decode() directly and use your trick with this ugly call. That is why I was just wondering. Sorry for misunderstanding.
Phil

I'll use Charset.decode() / encode() as I did before

Now it is clear.
Best,
Vlad
P.S. I am preparing now a new topic about MetaData and GUI. I know, you have great ideas there. I would be glad if you join.

[Jim Yingst]

Yes they are ugly, but they don't "override the default options" of decode().
What I meant was, by changing some of this ugly code you do have the option of exerting more control over these options, if needed. E.g. if we wanted to replace onMalformedInput(CodingErrorAction.REPLACE) with onMalformedInput(CodingErrorAction.REPORT), we could do so. That would be the main reaon why I might have wanted to use this extra code - to allow finer control. But since I didn't really need to override the default behaviors (REPLACE for onMalformedInput() and onUnmappableCharacter()), I didn't. I wasn't making any statement about what Philippe was doing. Even if you do use REPLACE, like the defaul, it may still amke sense to write that explicitly in your code so that future programmers realize that they have the option to change it if they don't like it. That's not what I chose to do, but it's justifible if someone else wants to, IMO.

[Vlad Rabkin]

Hi Jim,
Ok. Agreed.
Hi Phil,
Do you think it's Ok to hard code some things while reading a header like instead of raf.readInt() (we can change the constants indication length of value in header) raf.readFully(new byte[LENGTH_OF_COOKIE]) :

int recordLenth = raf.readInt();
         int columnCount = raf.readShort();
          // Column Metadata
          DBColumn columns[] =
            new DBColumn[columnCount];
            short nameLength = 0;
            String columnName = null;
            short columnLength = 0;
          // Schema description section.
          // read and fill description for each column
          for (int i = 0; i < columnCount; i++) {
            nameLength = raf.readShort();
          bbuf = new byte[nameLength];
          raf.read(bbuf);
          columnName = new String(bbuf, ENCODING);
          columnLength = raf.readShort(); 

}
Tx,
Vlad
[ September 18, 2003: Message edited by: Vlad Rabkin ]

[Philippe Maquet]

Hi Vlad,
Yes, I think it's OK and I do the same.
Best,
Phil.

[Vlad Rabkin]

Hi Phil,
Tx