[VFS] RAM Provider review and questions

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[VFS] RAM Provider review and questions

Bernd Eckenfels
Hello,

I had a look at the VFS2 RAM Provider in 2.0 and have some questions  
comments:


RamFileData.java
----------------
#52 byte[] buffer - this is the actual content of the file, I would name  
it that way. Buffer sounds transient

#62 Collection<RamFileData> - this keeps references to all childer, aka  
the whole tree. When a implementation gets smarter about cache/buffers  
this could harm a partial (off heap) storage and similiar things. Change  
this to String[] basenames? (especially since lookup is rather fast in RAM)

#71 children = Collections.synchronizedCollection(new  
ArrayList<RamFileData>()) - there are multiple contains() lookups on this  
collection. This can be slow on larger directories. Would it make sense to  
have a adaptive data structure here (or provide a general infrastructure  
for small-to-large collections?

#136 this.buffer = new byte[0]; - I would use a "final static byte[]  
EMPTY" to avoid allocation

#186 contains/add - the "if contains throw / add" construct is quite  
readable however it requires two instead of one linear search and since it  
is not synchronized there is a window for racecondition. Using the return  
boolean of the add() method (on a collection which refuses duplicates) is  
more efficient.

#208 contains/remove - same argument first checking then removing has a  
race and is inefficient. remove returns null if it was not in there.

#225 return children; - is it safe to return a modifyable collection? See  
also RamFileSystem#108 which synchronizes across class boundaries.

#244 equals / RamFileData data = (RamFileData) o; - nitpick: i would use  
"that" or "other", "data" looks local

#256 this.getName().hashCode() - is it intentional to access field via  
getter, toString uses the field

#281: System.arraycopy(this.buffer, 0, newBuf, 0, size); - this will fail  
if resize() truncates. Is it guranteed not to happen?

I think the whole resize is generally strange, only using the setBuf() is  
more atomic and does not leak uninitilized bytes.


RamFileObject.java
------------------
#95 this.data.getBuffer().length; - use data.size() or even this.size()?

#125 setBuffer(new byte[0]) - use EMPTY constant

#272 does it make sense to put the whole quota calculation into fs.  
(especially the option builder)

#276 when "newSize - size" is negative the resize() is a truncation, it  
should not be rejected even when the overall size is over maxsize (can  
this actually happen?)

RamFileOutputStream.java
------------------------
#63 introduce "RamFileObject data = this.file.getData()" as it is  
dereferenced 3 times in this method

#75 file.getData().getBuffer() - does not update last update date (if this  
is intended then should comment)

#61 I would change the whole logic to use setBuffer()

#87 write(buffer1) - replace with write(buffer1, 0, 1) this safes one  
method invocation

RamFileProvider.java
--------------------
#34 I think this is an unusal " *" line :)

RamFileSystem.java
------------------
#54 cache - is this actually a cache or is it the actual content store  
(see above RamFileData#62 for parent/child references). If it is intended  
to be the real storage it should be names that way.

#212 the Null argument is a bit missleading as the argument was not null  
by the state was imaginary.

#263 what a creative way to copy an input stream to an output stream.  
There is no need to Buffer the Ramoutput stream and you should never use  
the byte-wise read if not needed. I also wonder if there is no IOUtil to  
do that. Besides in this specific case it would be better to ask the  
source for size, resize the byte[] to it, read it fully and setBuffer() it  
(atomically). (and 512b is also a very small buffer, it should be more  
like 32k)

#273 the flush is not needed, close will flush (especially if you not  
buffer later on the flush is a nop)

#290 getClass/getMessage is seldom/never used anywhere else.  
FileSystemExeption should really have an IOExepction types constructor to  
be used consistently.

#306 int size() limits the maximum size for the Ram cache to 2GB, should  
be long (see also RamFileSystemConfigBuilder#68 which sets default to  
Integer.MAX_VALUE)

#306 fs.size() is actually used very often (especially when writing small  
write()s to the OutputStream. It is a synchronized full-cach-walker, looks  
like a bad bottlenek. Maybe it is better if a outputstream requests the  
size only once and calculates locally or use an atomicLong to account all  
changes instead of walking.

Generally I am not sure how the synchronisation/concurrency model of the  
providers looks like, so I did not comment on those aspects, but it looks  
like some methods exepct to be executed atomic.

Greetings
Bernd
--
http://bernd.eckenfels.net

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [VFS] RAM Provider review and questions

garydgregory
I created

https://issues.apache.org/jira/browse/VFS-483

to track all of this. See the link for updates and status.

Gary


On Wed, Aug 7, 2013 at 5:36 PM, Bernd Eckenfels <[hidden email]>wrote:

> Hello,
>
> I had a look at the VFS2 RAM Provider in 2.0 and have some questions
> comments:
>
>
> RamFileData.java
> ----------------
> #52 byte[] buffer - this is the actual content of the file, I would name
> it that way. Buffer sounds transient
>
> #62 Collection<RamFileData> - this keeps references to all childer, aka
> the whole tree. When a implementation gets smarter about cache/buffers this
> could harm a partial (off heap) storage and similiar things. Change this to
> String[] basenames? (especially since lookup is rather fast in RAM)
>
> #71 children = Collections.**synchronizedCollection(new
> ArrayList<RamFileData>()) - there are multiple contains() lookups on this
> collection. This can be slow on larger directories. Would it make sense to
> have a adaptive data structure here (or provide a general infrastructure
> for small-to-large collections?
>
> #136 this.buffer = new byte[0]; - I would use a "final static byte[]
> EMPTY" to avoid allocation
>
> #186 contains/add - the "if contains throw / add" construct is quite
> readable however it requires two instead of one linear search and since it
> is not synchronized there is a window for racecondition. Using the return
> boolean of the add() method (on a collection which refuses duplicates) is
> more efficient.
>
> #208 contains/remove - same argument first checking then removing has a
> race and is inefficient. remove returns null if it was not in there.
>
> #225 return children; - is it safe to return a modifyable collection? See
> also RamFileSystem#108 which synchronizes across class boundaries.
>
> #244 equals / RamFileData data = (RamFileData) o; - nitpick: i would use
> "that" or "other", "data" looks local
>
> #256 this.getName().hashCode() - is it intentional to access field via
> getter, toString uses the field
>
> #281: System.arraycopy(this.buffer, 0, newBuf, 0, size); - this will fail
> if resize() truncates. Is it guranteed not to happen?
>
> I think the whole resize is generally strange, only using the setBuf() is
> more atomic and does not leak uninitilized bytes.
>
>
> RamFileObject.java
> ------------------
> #95 this.data.getBuffer().length; - use data.size() or even this.size()?
>
> #125 setBuffer(new byte[0]) - use EMPTY constant
>
> #272 does it make sense to put the whole quota calculation into fs.
> (especially the option builder)
>
> #276 when "newSize - size" is negative the resize() is a truncation, it
> should not be rejected even when the overall size is over maxsize (can this
> actually happen?)
>
> RamFileOutputStream.java
> ------------------------
> #63 introduce "RamFileObject data = this.file.getData()" as it is
> dereferenced 3 times in this method
>
> #75 file.getData().getBuffer() - does not update last update date (if this
> is intended then should comment)
>
> #61 I would change the whole logic to use setBuffer()
>
> #87 write(buffer1) - replace with write(buffer1, 0, 1) this safes one
> method invocation
>
> RamFileProvider.java
> --------------------
> #34 I think this is an unusal " *" line :)
>
> RamFileSystem.java
> ------------------
> #54 cache - is this actually a cache or is it the actual content store
> (see above RamFileData#62 for parent/child references). If it is intended
> to be the real storage it should be names that way.
>
> #212 the Null argument is a bit missleading as the argument was not null
> by the state was imaginary.
>
> #263 what a creative way to copy an input stream to an output stream.
> There is no need to Buffer the Ramoutput stream and you should never use
> the byte-wise read if not needed. I also wonder if there is no IOUtil to do
> that. Besides in this specific case it would be better to ask the source
> for size, resize the byte[] to it, read it fully and setBuffer() it
> (atomically). (and 512b is also a very small buffer, it should be more like
> 32k)
>
> #273 the flush is not needed, close will flush (especially if you not
> buffer later on the flush is a nop)
>
> #290 getClass/getMessage is seldom/never used anywhere else.
> FileSystemExeption should really have an IOExepction types constructor to
> be used consistently.
>
> #306 int size() limits the maximum size for the Ram cache to 2GB, should
> be long (see also RamFileSystemConfigBuilder#68 which sets default to
> Integer.MAX_VALUE)
>
> #306 fs.size() is actually used very often (especially when writing small
> write()s to the OutputStream. It is a synchronized full-cach-walker, looks
> like a bad bottlenek. Maybe it is better if a outputstream requests the
> size only once and calculates locally or use an atomicLong to account all
> changes instead of walking.
>
> Generally I am not sure how the synchronisation/concurrency model of the
> providers looks like, so I did not comment on those aspects, but it looks
> like some methods exepct to be executed atomic.
>
> Greetings
> Bernd
> --
> http://bernd.eckenfels.net
>
> ------------------------------**------------------------------**---------
> To unsubscribe, e-mail: dev-unsubscribe@commons.**apache.org<[hidden email]>
> For additional commands, e-mail: [hidden email]
>
>


--
E-Mail: [hidden email] | [hidden email]
Java Persistence with Hibernate, Second Edition<http://www.manning.com/bauer3/>
JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
Spring Batch in Action <http://www.manning.com/templier/>
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory
Reply | Threaded
Open this post in threaded view
|

Re: [VFS] RAM Provider review and questions

Bernd Eckenfels
Am 08.08.2013, 03:49 Uhr, schrieb Gary Gregory <[hidden email]>:
> https://issues.apache.org/jira/browse/VFS-483
>
> to track all of this. See the link for updates and status.

Very cool thanks, I would also have files some, after discussion where a  
change makes sense. I filed this one here for the test package;  
https://issues.apache.org/jira/browse/VFS-482

Gruss
Bernd

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [VFS] RAM Provider review and questions

garydgregory
On Wed, Aug 7, 2013 at 10:02 PM, Bernd Eckenfels <[hidden email]>wrote:

> Am 08.08.2013, 03:49 Uhr, schrieb Gary Gregory <[hidden email]>:
>
>  https://issues.apache.org/**jira/browse/VFS-483<https://issues.apache.org/jira/browse/VFS-483>
>>
>> to track all of this. See the link for updates and status.
>>
>
> Very cool thanks, I would also have files some, after discussion where a
> change makes sense. I filed this one here for the test package;
> https://issues.apache.org/**jira/browse/VFS-482<https://issues.apache.org/jira/browse/VFS-482>
>

Bernd,

The test fixes are in, thank you. For the rest [1], I picked the low
hanging fruits, and the rest will need more study, later, I am done for
tonight ;)

Hopefully, others will comment...

Patches are always welcome, just make sure you look at the HEAD of trunk,
not the 2.0 source.

Please do not truncate the email thread, it makes harder for some of us to
follow what's going on from message to message.

Thank you for your input!

Gary

[1] https://issues.apache.org/jira/browse/VFS-483


>
> Gruss
> Bernd
>
>
> ------------------------------**------------------------------**---------
> To unsubscribe, e-mail: dev-unsubscribe@commons.**apache.org<[hidden email]>
> For additional commands, e-mail: [hidden email]
>
>


--
E-Mail: [hidden email] | [hidden email]
Java Persistence with Hibernate, Second Edition<http://www.manning.com/bauer3/>
JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
Spring Batch in Action <http://www.manning.com/templier/>
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory