History | Log In     View a printable version of the current page.  
Issue Details (XML | Word | Printable)

Key: CASTOR-1029
Type: Improvement Improvement
Status: Open Open
Priority: Minor Minor
Assignee: Unassigned
Reporter: Martin Fuchs
Votes: 0
Watchers: 1
Operations

If you were logged in you would be able to see more operations.
castor

allow to forget read only objects in long batch processes

Created: 27/Feb/05 08:59 AM   Updated: 19/Dec/05 04:59 PM
Component/s: JDO
Affects Version/s: 0.9.6
Fix Version/s: None

Time Tracking:
Not Specified

File Attachments: 1. Text File castor-1892-forget-ro-objects.patch (6 kb)
2. Text File patch-1892.txt (26 kb)

Environment:
Operating System: Windows XP
Platform: PC

Bugzilla Id: 1892


 Description  « Hide
When using Castor to process data in a long read only batch process the fetched
objects are all stored in TransactionContext._readOnlyObjects. This hash table
is not cleared until the end of the transaction. So there may arise problems
running out of heap memory.
Well, one way to overcome this would be to use shorter transactions. But I
think it's better to provide a simple additional method to disable usage of the
read-only-objects hash table. It avoids reconnecting to the database very
frequently and increases the performance of the batch process.
Attached patch implements the new function Database.setRememberReadOnlyObjects
() to deactivate the read only hash.

 All   Comments   Work Log   Change History      Sort Order: Ascending order - Click to sort in descending order
Martin Fuchs - 27/Feb/05 09:00 AM
Created an attachment (id=943)
proposed patch

Martin Fuchs - 05/Mar/05 05:57 AM
Well, what does everybody think about this extension?
We already used this successfully in our application.

Martin Fuchs - 06/Mar/05 02:59 AM
      • Bug 1450 has been marked as a duplicate of this bug. ***

Werner Guttmann - 08/Mar/05 07:38 AM
Martin, I think your patch is quite clear and precise. I wonder, though,
whether we should actually overload Databaseload() to provide this
functionality, and not add a property to Database(Impl) ?

Werner Guttmann - 15/Mar/05 07:02 AM
Just a question: I think that somewhere in the PAI, Castor JDO allows you to
elevate a lock from 'read' to something higher, e.g. 'write'. How would this
interfer with your ideas ?

Werner Guttmann - 16/Mar/05 12:47 AM
Btw, if you wondered about what PAI stands for, don't worry .. . I meant to
say 'API', of course.

I'm of two minds on this.

Either: Read only objects are critical to keep, in which case the hashtable is a correct implementation
and a flag either on load or on the database object is correct behavior...

Or: Keeping these things around forever is pointless, and moving to a weak reference is the right
solution; if the application didn't want it, and we're not referencing it, it can go away.

The weak hash ref is probably the easiest transparent change to make, and will likely behave correctly
in all normal circumstances; it also provides a mechanism for knowing when something used to be
loaded but is no longer present, as the key is still in the weak hash reference, and debug output can
inform the user that memory pressure led to the application clearing that content.

A flag, instead of setting whether or not read only objects should be remembered, could then allow you
to select reference type: Soft (eliminate when the JVM catches an OOM exception and wants to purge
unnecessary objects from the system), weak (eliminate whenever a GC pass detects an unreferenced
object), phantom (eliminate when all other references are gone, if the application uses weak
references itself).

Implementing a switch that allows you to select the referent style - strong, weak, soft, or phantom -
then provides for today's behavior (a 'strong' reference, which just holds an ordinary link to the object
in the value), and provide for three situations: Discard unreferenced on GC (soft), discard unreferenced
under memory pressure (weak), and discard unused on GC (phantom).

Drawback: it requires you to create reference objects, which increases the number of objects we have
to handle.


Werner Guttmann - 16/Mar/05 02:00 AM
What's a good article on weak/phantom references ? It looks like I need to
educate myself a bit .. .

Ralf Joachim - 16/Mar/05 02:21 AM
The requested feature only relates to access mode ReadOnly and can not be used
with other access modes. As suggested by Martin someone that wants to use the
new feature would have to call setRememberReadOnlyObjects() and call load() or
execute() with access mode ReadOnly. A drawback of the suggested solution is
that it will not be possible to load objects with access mode ReadOnly that are
remembert in the transaction with others that should not been remembert.

I suggest to introduce a new access mode ReadOffTransaction to implement the
requested feature. This would not have the disadvantages from above.

In addition we need to think about what will happen if the requested object is
already part of the transaction. I think it have to remain in the transaction
and the load() or execute() with access mode ReadOffTransaction behaves as with
access mode ReadOnly for this object only.

I don't think we can use soft-, weak- or phantom-references in
TransactionContext because we have to take care to commit or rollback all
changes done during the transaction and also all locks have to be released at
the end of the transaction.


Ralf Joachim - 16/Mar/05 02:25 AM
The javadoc is not to bad but I can also search for some articles in magazines
if you like.

Werner Guttmann - 16/Mar/05 02:28 AM
Don't worry, Ralf. Unless a particular article springs to your mind immediately,
I'll be happy to browse myself.

Werner Guttmann - 16/Mar/05 02:33 AM
> In addition we need to think about what will happen if the requested
> object is already part of the transaction. I think it have to remain
> in the transaction and the load() or execute() with access mode
> ReadOffTransaction behaves as with access mode ReadOnly for this object
> only.
Without commenting upon the feasability of this suggested approach, this looks
to me like we need to add even more if-else-ifs to an already quite convoluted
code base.


Werner Guttmann - 16/Mar/05 02:23 PM
> ... and it's under the assumption that anything in the read-only section
> can safely go away.
As mentioned above, a read lock for an object can be upgraded. If we 'did away'
with that object, this could be a problmem.

Martin Fuchs - 20/Mar/05 06:10 AM
> I suggest to introduce a new access mode ReadOffTransaction to implement the
> requested feature. This would not have the disadvantages from above.

Would be fine with me. But could you please explain what the
name "ReadOffTransaction" stands for? I could think about a new constant with a
name like "PlainRead" or "BatchRead" in the Database interface.

> In addition we need to think about what will happen if the requested object is
> already part of the transaction. I think it have to remain in the transaction
> and the load() or execute() with access mode ReadOffTransaction behaves as
with
> access mode ReadOnly for this object only.

Perhaps someone can explain me why the accessMode is translated into an
AccessMode object in DatabaseImpl.load() ?
Dropping this mapping and just using the enumeration values could perhaps make
the code a bit more readable.


Ralf Joachim - 20/Mar/05 07:08 AM
>Would be fine with me. But could you please explain what the
>name "ReadOffTransaction" stands for? I could think about a new constant with a
>name like "PlainRead" or "BatchRead" in the Database interface.

I used 'ReadOffTransaction' as it is a read only without adding the objects read
to the transaction but I also didn't like that name. Maybe your ideas are
better. Let's hear suggestions from our contibutors speaking native english

>Perhaps someone can explain me why the accessMode is translated into an
>AccessMode object in DatabaseImpl.load() ?
>Dropping this mapping and just using the enumeration values could perhaps make
>the code a bit more readable.
I guess that this has historical reasons and that the one that introduced
AccessMode don't wanted to change Database interface. If we want to do that now
we first need to introduce new db.load(... AccessMode) methods and declare the
old ones as deprecated until we remove them some versions later. We also can
make code a little easier to read by delegating the short->AccessMode
translation to the AccessMode class.


Martin Fuchs - 24/Mar/05 01:36 AM
Well, I though of keeping the Database interface as it's now (just adding a new
constant for the read-only-dont-remember mode) and dropping the AccessMode
class.

Ralf Joachim - 24/Mar/05 06:44 AM
If you search where the AccessMode class is used you may recognize that it will
be rather complicated to remove it (211 references). In addition I prefer
enumerations like AccessMode over unrelated short primitives.

I'll prepare a patch about my ideas to modify AccessMode and clean up Database
and DatabaseImpl.


Ralf Joachim - 26/Mar/05 02:32 PM
Created an attachment (id=989)
Patch to make usage of AccessMode public

Ralf Joachim - 12/Apr/05 03:22 PM
Moved refactoring of AccessMode to bug 1933